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

[11.x] Fix outer boolean on nested where #53197

Open
wants to merge 3 commits into
base: 11.x
Choose a base branch
from

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Oct 17, 2024

This PR fixes some recent changes to how nested queries were created when using where with array syntax.

The PR is in three parts:

  1. Revert the original change.
  2. Make the tests more robust to cover more scenarios to ensure we don't break this again.
  3. (optional) re-introduce the original change in a non-breaking way

If we don't want the original change, we should still merge the first two commits.

To be honest, at this point it feels kind dirty. I'm tempted to say we ignore the last commit and just remove the array syntax from the docs entirely and leave it in a weird inconsistent state.

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@hafezdivandari
Copy link
Contributor

Isn't it better to just revert the original changes and add support for array syntax to whereAny and orWhereAny instead?

// Current usage
DB::table('users')->where('active', true)>whereAny(['name', 'email'], 'like', 'Example%')->toRawSql();
// SELECT * FROM users WHERE active = true AND (name LIKE 'Example%' OR email LIKE 'Example%')

// Add support for single array argument
DB::table('users')->where('active', true)>whereAny(['name' => 'my name', 'email' => 'my email'])->toRawSql();
// SELECT * FROM users WHERE active = true AND (name = 'my name' OR email = 'my email')

// And array list argument
DB::table('users')->where('active', true)>whereAny([['name', '=', 'my name'], ['age', '>', 30]])->toRawSql();
// SELECT * FROM users WHERE active = true AND (name = 'my name' OR age > 30)

@timacdonald
Copy link
Member Author

Even with the original PR reverted, this test still passes. Using array syntax leads to inconsistent results.

I don't think we should expand the array syntax support. My vote goes to removing it from the docs, tbh. This inconsistency is pretty grim.

public function testItHasInconsistentResultsForOrWhereWithArray()
{
    $queries = [];
    $builder = $this->getBuilder();
    $builder->select('*')->from('users')->where('xxxx', 'xxxx')->orWhere(['foo' => 1, 'bar' => 2]);
    $queries[] = $builder->toSql();

    $builder = $this->getBuilder();
    $builder->select('*')->from('users')->where('xxxx', 'xxxx')->orWhere([['foo', 1], ['bar', 2]]);
    $queries[] = $builder->toSql();

    $this->assertSame([
        'select * from "users" where "xxxx" = ? or ("foo" = ? or "bar" = ?)',
        'select * from "users" where "xxxx" = ? or ("foo" = ? and "bar" = ?)',
    ], $queries);

    $queries = [];
    $builder = $this->getBuilder();
    $builder->select('*')->from('users')->where('xxxx', 'xxxx')->orWhereColumn(['foo' => '_foo', 'bar' => '_bar']);
    $queries[] = $builder->toSql();

    $builder = $this->getBuilder();
    $builder->select('*')->from('users')->where('xxxx', 'xxxx')->orWhereColumn([['foo', '_foo'], ['bar', '_bar']]);
    $queries[] = $builder->toSql();

    $this->assertSame([
        'select * from "users" where "xxxx" = ? or ("foo" = "_foo" or "bar" = "_bar")',
        'select * from "users" where "xxxx" = ? or ("foo" = "_foo" and "bar" = "_bar")',
    ], $queries);
}

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Oct 17, 2024

I totally agree, the inconsistancy must be resolved on 12.x?

But the array syntax (after fix) is useful, I think whereAny and orWhereAny can support this as the usage is clear, also with whereAll and whereNone.

My vote goes to removing it from the docs, tbh.

The ->orWhere(['foo' => 1, 'bar' => 2]); syntax compiles wrong query, and I couldn't find this syntax on the docs.

@zero-to-prod
Copy link

I don't think this syntax has ever been supported ->orWhere(['foo' => 1, 'bar' => 2]) nor does it make sense imo.

@Dmitrev
Copy link

Dmitrev commented Oct 17, 2024

I don't think this syntax has ever been supported ->orWhere(['foo' => 1, 'bar' => 2]) nor does it make sense imo.

It's sort of supported, but a bit inconsistent.

all examples are made using v11.28.0

using a single dimensional array is pretty much what I would expect.

DB::table('users')->orWhere( ['id' => '1'] )->toRawSql();
// "select * from `users` where (`id` = '1')

Using a multi dimensional array, things get a bit weird, now we have is null and id key is discarded

DB::table('users')->orWhere( [['id' => '1']] )->toRawSql();
// "select * from `users` where (`1` is null)"

Another interesting example, this seems pretty logical to me, comparing to null automatically uses is correctly.

DB::table('users')->orWhere( ['id' => null] )->toRawSql();
// "select * from `users` where (`id` is null)"

Then when using a multidimensional array, the builder probably just discards all null values

DB::table('users')->orWhere( [['id' => null]] )->toRawSql();
// "select * from `users`"

@timacdonald, totally agree this syntax is tricky and shouldn't be advertised.
but the tricky thing is that I believe that this functionality is technically exposed via the database assertions:

https://laravel.com/docs/11.x/database-testing#assert-database-has
https://github.com/laravel/framework/blob/11.x/src/Illuminate/Testing/Constraints/HasInDatabase.php#L54

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