Skip to content

[5.6] Ignore non-where bindings in nested where() constraints - #24000

Merged
taylorotwell merged 1 commit into
laravel:5.6from
staudenmeir:bindings
May 17, 2018
Merged

taylorotwell merged 1 commit into
laravel:5.6from
staudenmeir:bindings

Conversation

@staudenmeir

Copy link
Copy Markdown
Contributor

Nested where() constraints merge all bindings from the given query, even the ones from other query parts.

This breaks queries that reference a relationship with WHERE bindings (e.g. MorphMany) in $withCount and use a nested where() constraint:

class ParentModel extends Model
{
    protected $table = 'parent';

    protected $withCount = ['children'];

    public function children()
    {
        return $this->morphMany(ChildModel::class, 'morphable');
    }
}

class ChildModel extends Model
{
    protected $table = 'child';
}

$query = ParentModel::where(function($q) {
    $q->where(1, 1);
});

The query doesn't work as expected because an extra binding is added:

dd($query->getBindings());
// expected: ['App\ParentModel', 1];
// actual: ['App\ParentModel', 'App\ParentModel', 1]

As a result 'App\ParentModel' is used for the second placeholder and 1 is ignored.

This is caused by Model::newQueryWithoutScopes() adding $withCount to every query.
So the query received by Builder::where() contains the $withCount bindings.

This PR solves the problem by ignoring non-where bindings when the nested query is merged.

Fixes #23957.

@taylorotwell

Copy link
Copy Markdown
Member

Is it possible the nested where could legitimately have other bindings though?

@staudenmeir

staudenmeir commented Apr 30, 2018 •

Copy link
Copy Markdown
Contributor Author

I tried to find a case, but couldn't think of one.

We could also solve this problem at the root with something like Model::newQueryWithoutScopesWithoutCount() (preferably with a better name).

This method would create a query without $withCount & $with and could also be used for INSERT/UPDATE/DELETE queries.

@mpyw

mpyw commented May 14, 2018

Copy link
Copy Markdown
Contributor

I wrote a trait patch for this issue until this PR merged.

https://gist.github.com/mpyw/cb43c4cbd48d1ae1f63df949b1cc896b

@taylorotwell

Copy link
Copy Markdown
Member

I kinda prefer the newQueryWithoutScopesWithoutCount approach if you have any better names ideas. Would be curious to see what that looks like.

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.

3 participants