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.8] Fix belongsToMany->wherePivot condition for boolean columns #28252

Closed
wants to merge 1 commit into from
Closed

Conversation

edvordo
Copy link
Contributor

@edvordo edvordo commented Apr 18, 2019

Description

Fixes breaking change introduced in #28192, which used weak comparison which in turn caused searching for boolean values belongsToManyRelation->wherePivot('primary', true) (without specifying operator) (See #28251 for more details) in the pivot table to evaluate to true for the conditions $operator == 'in' / $operator == 'not in'.

Switching to typed comparison takes care of this issue.

@staudenmeir
Copy link
Contributor

Maybe we should revert #28192 completely. Even with this fix, ->wherePivot('foo', 'in') is broken.

@JeroenBoesten
Copy link
Contributor

JeroenBoesten commented Apr 18, 2019

Maybe we should revert #28192 completely. Even with this fix, ->wherePivot('foo', 'in') is broken.

You tested this? Because that should be taken care of by this portion of the where() function in theory.

        [$value, $operator] = $this->prepareValueAndOperator(
            $value, $operator, func_num_args() === 2
        );

@staudenmeir
Copy link
Contributor

@JeroenBoesten wherePivot() works differently:
->wherePivot('foo', 'in') calls ->where('foo', 'in', null, 'and')

This is also the reason why #28192 didn't break ->where('foo', true).

@taylorotwell
Copy link
Member

I'm going to revert that entire PR.

@edvordo
Copy link
Contributor Author

edvordo commented Apr 18, 2019

Should at least the test(s) be added so that it screams at people just in case they add it in future again?

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