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

[5.3] Unify join and builder syntax #13576

Merged
merged 4 commits into from
May 18, 2016
Merged

[5.3] Unify join and builder syntax #13576

merged 4 commits into from
May 18, 2016

Conversation

acasar
Copy link
Contributor

@acasar acasar commented May 16, 2016

This is a massive cleanup of the join clause (~200 lines of production code removed). The idea is to extend the query builder and reuse its functionality instead of duplicating it. This has multiple benefits:

  • no need to duplicate the logic for where clauses
  • simplified join compilation (by reusing where clause compiler)
  • unified syntax between join clause and query builder (i.e. "=" operator is now optional, access to additional clauses like whereRaw, whereSub, ...)

The only breaking change is the removal of $where boolean parameter in the on clause and the removal of some public JoinClause properties.

@willrowe
Copy link
Contributor

Would there be any issues with using methods available on Builder that wouldn't necessarily be valid in a join statement? I'm thinking of methods like select, orderBy, or groupBy.

@acasar
Copy link
Contributor Author

acasar commented May 17, 2016

They will be ignored, same as with nested where... Currently nothing is stopping you from writing:

User::where(function($query){
    $query->orderBy('foo');
})->get();

I first wanted to extract the where builder in a separate class and use it for nested wheres & joins, but it would make merging 5.2 -> 5.3 a nightmare.

The only problem is if you introduce non-where bindings - the query would fail in both cases.

@srmklive
Copy link
Contributor

Totally agree with @acasar in this regard. Seems a really good feature to have in 5.3

@@ -506,7 +506,7 @@ public function testHasWithContraintsAndJoinAndHavingInSubquery()
$builder = $model->where('bar', 'baz');
$builder->whereHas('foo', function ($q) {
$q->join('quuuux', function ($j) {
$j->on('quuuuux', '=', 'quuuuuux', 'and', true);
$j->where('quuuuux', '=', 'quuuuuux');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you need to change this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last param (true) was removed from the on clause since it no longer makes sense.

@taylorotwell taylorotwell merged commit e3546cd into laravel:master May 18, 2016
@taylorotwell
Copy link
Member

Looks good. Thanks!

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.

4 participants