laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.65k stars 11.04k forks source link

[11.28.0] `->orWhere()` behaviour changed with array input #53184

Closed Dmitrev closed 1 month ago

Dmitrev commented 1 month ago

Laravel Version

11.28.0

PHP Version

8.2.24

Database Driver & Version

No response

Description

I'm not sure if this was a bug and is now fixed, but either way it's a breaking change and thought it was worth creating an issue for.

The Query builder ->orWhere() command by default now combines statements using the OR rather than AND keywords

I found an closed PR that is related to this: https://github.com/laravel/framework/pull/53147

The following change broke the functionality: https://github.com/laravel/framework/pull/53147/files#diff-2ed94a0ea151404a12f3c0d52ae9fb5742348578ec4a8ff79d079fa598ff145d

Perhaps this needed to be patch for Query builder as well? PR was only targeting Eloquent

I think the Query builder should probably have the same functionality of passing a boolean param to avoid having the functionality be inconsistent between the two.

There is a clear workaround in this case, which is to not use the array syntax and instead use the orWhere with a callback like this:

$query->orWhere(function(Builder $query) {
    $query->where(...)->where(...);
});

Steps To Reproduce

Run the following in tinker (should work on any clean install of Laravel):

DB::table('users')->orWhere([['id', '=', 1], ['id', '=', 2]])->toRawSql();

output Laravel 11.28.0:

= "select * from `users` where (`id` = 1 or `id` = 2)"

output Laravel 11.23.5 (notice the and instead of or)

= "select * from `users` where (`id` = 1 and `id` = 2)"
Dmitrev commented 1 month ago

I was able to do some additional testing and narrowed the issue down to the commit that caused the issue.

Remove the boolean here, reverts back the behaviour. https://github.com/laravel/framework/pull/53147/files#diff-2ed94a0ea151404a12f3c0d52ae9fb5742348578ec4a8ff79d079fa598ff145d

sivabalan02 commented 1 month ago

I also face the same issue. Reverting back to 11.22 helps

hafezdivandari commented 1 month ago

@timacdonald you may want to take a look at this.

zero-to-prod commented 1 month ago

The problem is that the elements in the array are or joined instead of and joined.

Take a look at the $boolean in the where() method.

This is the line that introduced the change of behavior: https://github.com/laravel/framework/blob/80843d20cf9337b94fb71e7f25f33f4b1e6c7c5f/src/Illuminate/Database/Query/Builder.php#L929

Passing the boolean: $boolean argument does this.

taylorotwell commented 1 month ago

Hmm, this is a tough one. The inconsistent behavior that @timacdonald fixed makes this feel like a bugfix to me, but it is breaking since people have relied on the inconsistent behavior.

zero-to-prod commented 1 month ago

ave relied on the inconsistent behavior.

Definitly feels like a Hyrum's Law thing.

hafezdivandari commented 1 month ago

@taylorotwell It seem that the changes on the PR are invalid, the boolean applies to the outer constraint not the nested ones?

User::where('active', true)->orWhere(['name' => 'Tim', 'verified' => true])->toRawSql();

// This one is wrong?
// select * from "users" where "active" = 1 or ("name" = 'Tim' or "verified" = 1)"

User::where('active', true)->orWhere([['name', 'Tim'], ['verified', true]])->toRawSql();

// This one was correct, but broken in the PR?

// Before the PR:
// select * from "users" where "active" = 1 or ("name" = 'Tim' and "verified" = 1)

// After the PR:
// select * from "users" where "active" = 1 or ("name" = 'Tim' or "verified" = 1)
timacdonald commented 1 month ago

Got a fix up for this. Unfortunately the array syntax results in inconsistent queries depending on which one you use.

Sorry for the troubles here, folks. I've added a heap of additional tests to hopefully ensure we don't hit this again.

https://github.com/laravel/framework/pull/53197

Afrowson commented 1 month ago

@taylorotwell We also relied on this "feature" since 2021... now we have to fix our code... that is OK. I can see why it was a "bug" but please... if you decide to not revert the breaking change, at least comment it on the Laravel11 Migration guide... I spent far to much time debugging the root issue of my failing tests...