Skip to content
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

[9.x] Implement Responsable on query builder #40990

Closed
wants to merge 3 commits into from
Closed

Conversation

kemp
Copy link
Contributor

@kemp kemp commented Feb 13, 2022

This pull request implements the Responsable interface on the Query Builder.

Before:

Route::get('/users', function () {
    return User::where('id', '>', 7)->get();
});

After:

Route::get('/users', function () {
    return User::where('id', '>', 7);
});

This allows the user to return a query builder object as a response directly, which automatically calls ->get().

Why?

This is one of the small paper-cuts I've found in the framework that hinders the developer experience, and potentially confuses new developers with this error:

Symfony\Component\HttpFoundation\Response::setContent(): Argument #1 ($content) must be of type ?string, Illuminate\Database\Eloquent\Builder given, called in [...]

Additionally, a relevant question on Stack Overflow has been viewed over 22 thousand times.

Tests

I have started work on the tests for this feature, but may need help from someone in the community, as the framework is quite complex and I'm not sure exactly how to test this interaction between the query builder and router.

Why 9.x?

This feature does not break existing behavior. This functionality is additive.

@driesvints
Copy link
Member

Hi, I've fixed the tests on 9.x so please rebase your PR and mark your PR as ready when tests pass. Thanks

@driesvints driesvints marked this pull request as draft February 14, 2022 08:24
@punyflash
Copy link
Contributor

When I just started learning Laravel, there were a lot of misconceptions for me when I tried to understand the difference between collection and query builder. I think this feature will be quite damaging for understanding Eloquent concepts

@kemp
Copy link
Contributor Author

kemp commented Feb 14, 2022

@driesvints I have updated my fork of the repo, but the tests are still failing (on Windows):

There was 1 failure:

1) Illuminate\Tests\Support\SupportHelpersTest::testRetryWithPassingWhenCallback
Failed asserting that 0.1747579574584961 matches expected 0.1.

@punyflash Thank you for your feedback. I understand this feature might cause confusion, and I'm certainly in no place to determine whether it should or should not go in the framework (that's up to the maintainers), but I do believe that this functionality provides value, because in many cases the API for collections and the query builder is the same and "just works", and this would be another step in that direction. Additionally, I believe that the error cited in the description of my PR is arguably more confusing.

I do see however that this feature may be considered controversial (sitting at 4 👎 at the time of writing), and would be willing to publish a package instead for those who are interested. 🙂

@kemp kemp marked this pull request as ready for review February 14, 2022 14:30
@driesvints
Copy link
Member

Seems like these were some flakey tests. All green now.

@kemp Taylor will decide on this PR.

@IronSinew
Copy link

IronSinew commented Feb 16, 2022

I feel it has too many presumptions. If I had a query like where("id", 1) one could surmise that you could expect a first()- like response of one object (or null) and not necessarily a traversable that you'd get with get() ... or maybe someone could presume it should be a firstOrFail() type response.

I think ultimately that's where there is get vs first vs firstOrFail etc..

@taylorotwell
Copy link
Member

I had the same thoughts as @IronSinew - think I will hold off for now, but thanks for the suggestion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants