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

[6.0] Add subquery support for from() and table() - OPTION 2 #29602

Merged
merged 2 commits into from Aug 19, 2019

Conversation

@reinink
Copy link
Contributor

commented Aug 16, 2019

This PR adds the ability to use subqueries with the database table() method and query builder from() method. This is an alternative approach to my other PR: #29594.

$query->select('name')
    ->from(function ($query) {
        $query->something();
    }, 'users')
    ->get();

DB::table(function ($query) {
    $query->something();
}, 'users')->get();

What's nice about this approach is that you can alias regular table names as well:

DB::table('users', 'alias')

Pinging @staudenmeir to verify that my approach to aliasing here is safe. Based on my tests, the aliased table name appears to be properly escaped.

As for breaking changes, I think the only one is the argument change on the Illuminate/Database/Capsule/Manager@table method.

@reinink reinink changed the title Add subquery support for "from" and "table" Add subquery support for from() and table() - OPTION 2 Aug 16, 2019

@reinink reinink changed the title Add subquery support for from() and table() - OPTION 2 [6.0] Add subquery support for from() and table() - OPTION 2 Aug 16, 2019

@reinink reinink force-pushed the reinink:improve-from-subqueries-v2 branch 2 times, most recently from 6e41053 to e797c11 Aug 16, 2019

@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

When I suggested this approach, I didn't realize that Manager also has a table() method. I think this is a particularly nasty breaking change, as it only affects people who are using the illuminate/database package outside of Laravel. I imagine that they don't typically read the Laravel upgrade guide.

@reinink

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

as it only affects people who are using the illuminate/database package outside of Laravel.

Yup. That said, it seems kind of insane to cater to this though. 🤷‍♂

@mfn

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

I imagine that they don't typically read the Laravel upgrade guide.

They have to, there is no alternative.

I'm one of those users using illuminate/database outside Laravel.

The individual packages don't have proper release management (like individual changelog) so the only thing is the Laravel release guide itself.

@reinink reinink force-pushed the reinink:improve-from-subqueries-v2 branch from e797c11 to 5df36f2 Aug 17, 2019

@reinink reinink force-pushed the reinink:improve-from-subqueries-v2 branch from 5df36f2 to 6c1e014 Aug 17, 2019

@driesvints

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@reinink it's always important that we also consider the components individually. One of the nice things about the framework is that you can re-use the components individually in custom build apps outside the framework. As long as we always add everything to the upgrade guide we should be good 👍

@taylorotwell taylorotwell changed the base branch from master to 6.0 Aug 19, 2019

@taylorotwell taylorotwell merged commit c451f22 into laravel:6.0 Aug 19, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.