Skip to content

[9.x] Add collect key parameter to replicate the behavior of Response::collect method #44665

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[9.x] Add collect key parameter to replicate the behavior of Response::collect method #44665

wants to merge 2 commits into from

Conversation

brenjt
Copy link

@brenjt brenjt commented Oct 19, 2022

The Illuminate\Http\Client\Response::collect method makes it really easy to convert a json object to a collection object. It would be very convenient to have a similar functionality in the collection class itself however the collect method does not accept an argument.

Given this data structure

{
    "results": [1,2,3],
    "meta": [1,2,3]
}

To access the sub keys as a collection you'd need to do this:

$data = Http::get()->collect();
$results = collect($data->get('results'));
$meta = collect($data->get('meta'));

But with this PR you can simply do this:

$data = Http::get()->collect();
$results = $data->collect('results');

I recognize that you could do this

$response = Http::get();
$results = $response->collect('results');

However when used in this context it makes it not so convenient since you can't cache a response object

public function templates(): Collection
{
    return collect($this->getTemplates()->get('templates'))->mapInto(Template::class);
}

public function upsells(): Collection
{
    return collect($this->getTemplates()->get('upsells'))->mapInto(Upsell::class);
}

protected function getTemplates(): Collection
{
    return cache()->remember(
        key:'templates',
        ttl: now()->addMinutes(10),
        callback: fn () => Http::asJson()->get('endpoint')->collect()
    );
}

@brenjt brenjt changed the title Add collect key parameter to replicate the behavior of Response::collect method [9.x] Add collect key parameter to replicate the behavior of Response::collect method Oct 19, 2022
@JosephSilber
Copy link
Contributor

JosephSilber commented Oct 24, 2022 •
edited
Loading

I've always wished for a method that takes a key and returns that value as a collection. I think it's a great idea 👍

I'm just not so keen on overloading the existing collect() method. I think it would make more sense to come up with a name for a different method instead.

The problem with using collect() is that it's conflating two separate concerns into a single method.

The collect() method was originally added to the lazy collection to convert it into an eager collection. It was later added to the regular collection for consistency, so that when you have an instance of Enumerable you could always call collect() and be sure you have an eager collection.

Adding collect($key) to the lazy collection does not make sense IMO. Lazy collections do not have random access. Doing collect($key) on them enumerates the whole collection first, which is unexpected.

@brenjt
Copy link
Author

brenjt commented Oct 25, 2022

That is a good point, I hadn't thought of the LazyCollection in this scenario. Let me give it some thought on a better name and implementation.

@driesvints driesvints marked this pull request as draft October 25, 2022 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brenjt @JosephSilber @taylorotwell