Make Query Builder `->get()` optional (automatically call on array access)

[calebporzio]

This proposal is for aesthetics and code brevity.

Personally, ->get() often taints the readability of a line of code for me. I recognize this may just be some sort of fetish, but entertain me.

By making Builder.php (Query or Eloquent) implement ArrayAccess and IteratorAggregate (similar to Collection.php), we could automatically call ->get() internally and allow the Builder to be used in an Array context. Let me give you an examples:

Post::unpublished()->get(); // current syntax (unpublished is a scope)

foreach (Post::unpublished() as $post) {...

All the docs can remain the same. It could just be a quiet feature for the purpose of code aesthetics. Similar to: Post::where('title'... and Post::whereTitle(...

What do we think?

[Dylan-DPC-zz]

What's the gain?

[joshmanders]

@Dylan-DPC Developer happiness. I often find myself wondering why my stuff don't work, and realize that I forgot ->get() at the end.

[calebporzio]

@Dylan-DPC - valid question. The gain is definitely not easily justifiable.

Well put @joshmanders - Developer Happiness. I know it would mean MY developer happiness, hopefully others feel the same way and the dangers aren't too great.

To me ->get() feels like a query implementation detail.

In this example: Post::unpublished()->get(), Post and unpublished are descriptive domain terms and get brings me back to the unfortunate reality that I am actually just building a query.

I liken it to Collections. You would really hate it if you have to type ->all() or toArray() before you "used" a collection. It removes the ugly implementation reality and feels more seamless. Everyone knows toArray() exists, and sometime need to use it, but developers have the option of operating a level above it if they choose.

I hope the makes sense.

Thanks for caring.

[ConnorVG]

This is a pointless loss in readability purely for the satisfaction of adding crappy generic magic.

[byCedric]

How would you perform a count to the unpublished posts without fetching it all? I feel this adds something that will hit performance without notice:

// old
Post::unpublished()->get(); // fetches it all
Post::unpublished()->count(); // only fetches the count (aggregate method)

// new
Post::unpublished(); // fetches it all
Post::unpublished()->count(); // fetches it all, and then count it in PHP

It does work, but I do think this is a no-go due to performance alone...

[calebporzio]

So, get doesn't get called UNTIL you access Builder as an array. So there would be no performance hit.

Post::unpublished(); // does not fetch any thing foreach (Post::unpublished() as $post) // does fetch

make sense @byCedric?

[byCedric]

@calebporzio I don't think it makes sense to put such an important and heavy weight action together with a simple array access... Personally I don't think that's a great developer experience...

[ConnorVG]

I wasn't going to post this originally but I might as-well post it now.

Consider this generic scenario that would occur if this was fact, in a controller:

$posts = Post::unpublished();

foreach ($posts as $post) ...

return view('...')->with('posts', $posts);

In the view:

@foreach ($posts as $post)
   ...
@endforeach

The query would be executed multiple times because it wasn't explicitly executed. If people see that this feature is available, they simply won't be explicit and this will occur a lot.

Either way, that's my remaining input.

[mattstauffer]

I'm not sure whether this should or shouldn't be merged.

But.

This is not the first time in Laravel that an action would be taken in response to a certain type of call.

Responses are converted to strings when they're accessed in a string context. Collections are converted to arrays when they're accessed in an array context. Collections are converted to JSON when they're accessed in a string context.

It's not exactly the same because those conversions are more temporary--they don't make a database call happen. So maybe this is a bad idea--like @ConnorVG mentioned, there are potential ways that this would lead people to accidentally make a call multiple times (there may be a way to get around this, may not be). But to suggest that this is bad just because it's magic is ignoring the fact that it's in line with a lot of other similar magic in the framework.

This feedback is not helpful:

This is a pointless loss in readability purely for the satisfaction of adding crappy generic magic.

This feedback is helpful:

https://github.com/laravel/internals/issues/563#issuecomment-299212815

[byCedric]

@mattstauffer Yes, its the same for all casting properties of Eloquent. Type casting or type conversions are often invoked like this. But as you mentioned, a database query execution is a lot heavier. I still feel like when something like that needs to happen, I want an explicit method to invoke it. Also I think that many junior or starting developers would fall for the example provided by @ConnorVG.

[ConnorVG]

@mattstauffer consider this: Comparing a collection to an array, a collection is (internally) an array; Comparing a response to a string, a response is (bar headers + status code) a string.

So, in real life let's say: Comparing a bottle of juice to the juice it contains; Comparing a piece of paper to the ink that is on it.

Finally: Comparing an empty fuel tank, ready to be filled, to the petrol that is going to fill it; Comparing cake mix to the cake it's not baked into yet.

This is how I see this, it's a completely different type of data flow. It's not currently available data, it's data that needs to be retrieved via some sort of middleman-delivery.

When I apply this sort of logic to a query, array access to it should either return some sort of binding data or it should return a constraint. Not the data it's preparing to retrieve.

[mattstauffer]

@ConnorVG I hear you, but I would more say this:

Comparing a collection to an array, a collection is (internally) an array; Comparing a response to a string, a response is (bar headers + status code) a string. Comparing a query builder instance to an executed query (aka results), a query builder is (in its essence) a query executed against a database which, inherently, returns results.

A query exists for very few reasons than to be executed. get() just means execute() in the majority of cases. Obviously this is reductionist, but that doesn't make it right or wrong.

Yes. A not-yet-get()-ed query can be counted. It can be modified. But the core essence of what it represents is a query made against a database whose logical output is database results.

I agree it's a different data flow, which is why I'm saying I'm not sure whether this new-but-similar way of thinking about it is right. Every point that this feels different is right! I'm on board there.

Like I said earlier, I don't think this is definitely good or bad. I'm really unsure.

But. I think arguments that state this is pointless, crappy, generic, or out of sync with the rest of Laravel are invalid, and I want us to discard them as we consider this proposal's merit. I also think there are super valid arguments against it--for example, your concern that people would use this more than once in a row, introducing potential performance issues--which we should absolutely consider.

[ConnorVG]

@mattstauffer to be fair, I didn't bring that back up – no one has since you last did (in response to: "I think arguments that state this is pointless, crappy, generic, or out of sync with the rest of Laravel are invalid, and I want us to discard them as we consider this proposal's merit.").

I don't know of anywhere in Laravel that has this style of accessor on not-yet-known data. Like I made clear in my last comment, I understand your sentiment on the other parts (collections, responses etc) – I don't understand it on queries.

The significant difference here is "accessing" data vs "retrieving then accessing" data.

[joshmanders]

your concern that people would use this more than once in a row, introducing potential performance issues--which we should absolutely consider.

This concern can be an issue right now too, I can do the same call twice in my controller not thinking otherwise, the idea is that I was warned about it in docs, etc... If I know that if I omit ->get() from the call, and pass it off to a foreach loop or even to a response, it'll get called internally, then everything should be fine.

Hand holding is fine, but there comes a time when you have to take the training wheels off and put a level of responsibility on the developer themselves.

[ConnorVG]

@joshmanders "This concern can be an issue right now too"

If by "I can do the same call twice in my controller not thinking otherwise" you mean literally typing out the query, in full, twice then that is not even remotely similar. I may be mistaken by your point here, though.

Can you provide a viable example of this, please?

[calebporzio]

$unpublishedPosts = Post::unpublished();

foreach ($unpublishedPosts as $post)

foreach($unpublishedPosts->withinTheLastDay as $post)

It's a feature! :)

We could of course do some caching if accessed as array after the first time, I could see that being pretty simple. Or we could embrace the new way of treating these and come up with cool stuff?

Ya, I like it - let's keep talking examples. Both pro and con

[joshmanders]

@ConnorVG using your example:

$posts = Post::unpublished();

foreach ($posts->get() as $post) ...

return view('...')->with('posts', $posts->get());

There is nothing protecting me from calling ->get() multiple times. I just know better not to do it.

[ConnorVG]

@joshmanders that's not an example of what I stated? Your example for that, from what you said, would be:

$posts = Post::unpublished();

foreach ($posts->get() as $post) ...

return view('...')->with('posts', $posts->get());

Which is a fundamental misunderstanding, I'm unsure on how you could mean this is like "I can do the same call twice in my controller not thinking otherwise". There is an immediately available top-down view of duplication here.

[joshmanders]

@ConnorVG my point is, education. If you know what's going on, there should be no surprises. Instead of adding boilerplate that can be gotten rid of because someone might not know is a bunk argument to me.

[ConnorVG]

@joshmanders it's not boilerplate, it's fluent. The whole idea of the "Eloquent query builder" is that we have a readable flow of data from left to right. We know what we're retrieving, how it needs to be constrained and when we retrieve it.

This functionality drops the latter, it's like building a sentence and not fi-

[Dylan-DPC-zz]

@calebporzio

How will you differenciate between:

$usingEloquentCount = Post::unpublished()->count();

$usingCollectionCount = Post::unpublished()->get()->count();

by using your proposal?

Both will be:

$usingIDontKnowWhat = Post::unpublished()->count();

[joshmanders]

It doesn't drop it. You can still do it the way you do now, but you can also omit ->get() and just pass the builder to whatever, say a foreach, or whatever and it'll do it for you.

[joshmanders]

@Dylan-DPC $usingIDontKnowWhat uses eloquent count. Because you didn't pass it off to something that calls ->get() for you.

[ConnorVG]

OK, I think we've all made some valid points here and there is a lot of info here to help base a decision off of. This is now a very well described feature request 😬

[Riari]

Not a fan of this. Automatic casting/conversion of data is one thing; automatic querying of a data source is something else. That kind of thing should always be explicit IMO.

[franzliedke]

If I remember correctly, Rails has a similar concept for this in their ActiveRecord implementation. And it works well. There are a few differences that make this possible:

• Queries being built are basically proxies for collections: Once they are executed (by calling ->get or iterating over them), they cache their result. This solves the problem of queries being executed twice accidentally.
• More importantly, this would require query objects to be immutable. Otherwise, executing one query would automatically create a cached result for a different query that is built by extending the (already cached) query object, e.g. with another where() condition. At least, immutability is how Rails deals with this.

[BrandonShar]

I really like this idea.

I don't think query builders would necessarily have to immutable, we would just have to be smart about cache invalidation (easier said then done).

I would also expect this behavior to extend to calling collection methods on a query builder. Could be as bold as automatically trying to cache and call the method on the resulting collection if an unknown method is called on the builder.

Something like

public function __call($method, $args)
{
  // query builder's current logic

  return $this->get()->$method(...$args);
}

[BrandonShar]

A friend of mine just correctly pointed out that collections and query builders share some methods (namely where) so if this were as simple as my example it would be fairly confusing. Maybe just methods like map and each should on query builders to act similarly to the arrayaccess features..