Update Builder.php

[yosef-boaelite]

Fixed $boolean parameter being ignored by "where" function, when the $column parameter is an array.

In detail: the following code should produce a query with "or" conditions:

..->where(["col1"=>"val1","col2"=>"val2"],null,null,"or");

But, it produced the query with "and" conditions - ignoring the last parameter "or" and taking it as "and" (default value)

[jarnovanleeuwen]

FYI: This change caused very strange behaviour in our application as some part of a query flipped from a = a or (b = b and c = c) to a = a or (b = b or c = c). Even though this probably was a programming bug on our side, I can imagine this PR can introduce a hard-to-spot bug in many applications and perhaps a test was missing?

[yosef-boaelite]

I tested the change with a straightforward usage of the "where" function. The parameter that you expected to be handled, was just skipped and considered default. Can you please post your code with the query builder, which produced that query?

…

On Wed, Aug 23, 2017 at 1:09 PM, Jarno van Leeuwen ***@***.*** > wrote: FYI: This change caused very strange behaviour in our application as some part of a query flipped from (a = a and b = b) or (c = c and d = d) to (a = a and b = b) or (c = c or d = d). Even though this probably was a programming bug on our side, I can imagine this PR can introduce a hard-to-spot bug in many applications and perhaps a test was missing? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#20553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKABLPrvTPrS0zIGCF-gy7hek-pzMMoeks5sa_pjgaJpZM4O2GkQ> .

[jarnovanleeuwen]

Yeah, your PR is probably correct, but applications that relied on the previous orWhere behaviour might break.

Example:

$query->where(['a' => 'a'])->orWhere(['b' => 'b', 'c' => 'c'])

[yosef-boaelite]

I see. Honestly it's my first PR ever, so I don't know exactly what to do in such cases - one side is that the code of the function is more correct now, but on the other side it changes behavior of other functions that are relying on it. As for your use case, I didn't find the good solution for this so far. All solutions I saw require further changes in the code that might cause more problems. So, if it is what has to be done I'm ready to revert my changes. Taylor, what do you think?

…

On Wed, Aug 23, 2017 at 2:01 PM, Jarno van Leeuwen ***@***.*** > wrote: Yeah, your PR is probably correct, but applications that relied on the previous orWhere behaviour might break. Example: $query->where(['a' => 'a'])->orWhere(['b' => 'b', 'c' => 'c']) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#20553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKABLBy_whYWvEibmBOgADmWPfUzmtxFks5sbAZugaJpZM4O2GkQ> .

[jarnovanleeuwen]

Oh, it is actually very easy to modify the code to work with this change. Simply pass a closure to the orWhere method. I was wondering if this should be treated as a breaking change and thus should be scheduled for the Laravel 5.5 release instead. Op wo 23 aug. 2017 om 17:42 schreef yosef-boaelite <[email protected]

…

I see. Honestly it's my first PR ever, so I don't know exactly what to do in such cases - one side is that the code of the function is more correct now, but on the other side it changes behavior of other functions that are relying on it. As for your use case, I didn't find the good solution for this so far. All solutions I saw require further changes in the code that might cause more problems. So, if it is what has to be done I'm ready to revert my changes. Taylor, what do you think? On Wed, Aug 23, 2017 at 2:01 PM, Jarno van Leeuwen < ***@***.*** > wrote: > Yeah, your PR is probably correct, but applications that relied on the > previous orWhere behaviour might break. > > Example: > > $query->where(['a' => 'a'])->orWhere(['b' => 'b', 'c' => 'c']) > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#20553 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AKABLBy_whYWvEibmBOgADmWPfUzmtxFks5sbAZugaJpZM4O2GkQ > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#20553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABS8laQUKVEUyid3cH_6V-gH49SHf_Fqks5sbEhegaJpZM4O2GkQ> .

[yosef-boaelite]

Thanks for advice. I saw the solution with a Closure, but thought it's not good enough, because still requires changes of user code. To schedule to the next version sounds good. So... how to do it? I didn't find the branch 5.5 on the repo. The last one is 5.4. On Wed, Aug 23, 2017 at 9:05 PM, Jarno van Leeuwen <[email protected]

…

wrote: Oh, it is actually very easy to modify the code to work with this change. Simply pass a closure to the orWhere method. I was wondering if this should be treated as a breaking change and thus should be scheduled for the Laravel 5.5 release instead. Op wo 23 aug. 2017 om 17:42 schreef yosef-boaelite < ***@***.*** > > I see. > > Honestly it's my first PR ever, so I don't know exactly what to do in such > cases - one side is that the code of the function is more correct now, but > on the other side it changes behavior of other functions that are relying > on it. > > As for your use case, I didn't find the good solution for this so far. All > solutions I saw require further changes in the code that might cause more > problems. > > So, if it is what has to be done I'm ready to revert my changes. > > Taylor, what do you think? > > On Wed, Aug 23, 2017 at 2:01 PM, Jarno van Leeuwen < > ***@***.*** > > wrote: > > > Yeah, your PR is probably correct, but applications that relied on the > > previous orWhere behaviour might break. > > > > Example: > > > > $query->where(['a' => 'a'])->orWhere(['b' => 'b', 'c' => 'c']) > > > > — > > You are receiving this because you authored the thread. > > Reply to this email directly, view it on GitHub > > <#20553# issuecomment-324295324 > >, > > or mute the thread > > < > https://github.com/notifications/unsubscribe-auth/AKABLBy_ whYWvEibmBOgADmWPfUzmtxFks5sbAZugaJpZM4O2GkQ > > > > . > > > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#20553 (comment) >, > or mute the thread > <https://github.com/notifications/unsubscribe- auth/ABS8laQUKVEUyid3cH_6V-gH49SHf_Fqks5sbEhegaJpZM4O2GkQ> > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#20553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKABLAEuAT5Pub0zPBOk2PFIvtTsWBhFks5sbGnugaJpZM4O2GkQ> .