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.6] Ignore non-where bindings in nested where() constraints #23982

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

staudenmeir
Copy link
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.

@morloderex
Copy link
Contributor

tests for this?

@staudenmeir
Copy link
Contributor Author

->addBinding('ignore', 'select') tests that non-where bindings are ignored. Before this change 'ignore' would have been added to the bindings and the test would have failed.

@taylorotwell
Copy link
Member

Please re-submit with an entirely new test that shows the corrected behavior and would have been broken before. Try not to modify existing tests.

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.

None yet

3 participants