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.

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

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

Closed
calebporzio opened this Issue May 4, 2017 · 28 comments

Comments

Projects
None yet
10 participants
@calebporzio

calebporzio commented May 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Dylan-DPC

Dylan-DPC May 4, 2017

What's the gain?

Dylan-DPC commented May 4, 2017

What's the gain?

@joshmanders

This comment has been minimized.

Show comment
Hide comment
@joshmanders

joshmanders May 4, 2017

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

joshmanders commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@calebporzio

calebporzio May 4, 2017

@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.

calebporzio commented May 4, 2017

@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 comment has been minimized.

Show comment
Hide comment
@ConnorVG

ConnorVG May 4, 2017

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

ConnorVG commented May 4, 2017

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

@byCedric

This comment has been minimized.

Show comment
Hide comment
@byCedric

byCedric May 4, 2017

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...

byCedric commented May 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@calebporzio

calebporzio May 4, 2017

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?

calebporzio commented May 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@byCedric

byCedric May 4, 2017

@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...

byCedric commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ConnorVG

ConnorVG May 4, 2017

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.

ConnorVG commented May 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattstauffer

mattstauffer May 4, 2017

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:

#563 (comment)

mattstauffer commented May 4, 2017

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:

#563 (comment)

@byCedric

This comment has been minimized.

Show comment
Hide comment
@byCedric

byCedric May 4, 2017

@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.

byCedric commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ConnorVG

ConnorVG May 4, 2017

@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.

ConnorVG commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mattstauffer

mattstauffer May 4, 2017

@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.

mattstauffer commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ConnorVG

ConnorVG May 4, 2017

@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.

ConnorVG commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@joshmanders

joshmanders May 4, 2017

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.

joshmanders commented May 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ConnorVG

ConnorVG May 4, 2017

@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?

ConnorVG commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@calebporzio

calebporzio May 4, 2017

$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

calebporzio commented May 4, 2017

$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

This comment has been minimized.

Show comment
Hide comment
@joshmanders

joshmanders May 4, 2017

@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.

joshmanders commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ConnorVG

ConnorVG May 4, 2017

@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.

ConnorVG commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@joshmanders

joshmanders May 4, 2017

@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.

joshmanders commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ConnorVG

ConnorVG May 4, 2017

@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-

ConnorVG commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Dylan-DPC

Dylan-DPC May 4, 2017

@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();

Dylan-DPC commented May 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@joshmanders

joshmanders May 4, 2017

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 commented May 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@joshmanders

joshmanders May 4, 2017

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

joshmanders commented May 4, 2017

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

@ConnorVG

This comment has been minimized.

Show comment
Hide comment
@ConnorVG

ConnorVG May 4, 2017

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 😬

ConnorVG commented May 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Riari

Riari May 4, 2017

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.

Riari commented May 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke May 4, 2017

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.

franzliedke commented May 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@BrandonShar

BrandonShar Aug 1, 2017

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 commented Aug 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@BrandonShar

BrandonShar Aug 1, 2017

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..

BrandonShar commented Aug 1, 2017

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..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment