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

[BUG] - empty whereIn array results in condition ignored #265

Merged
merged 3 commits into from
Mar 10, 2021

Conversation

VinceG
Copy link
Contributor

@VinceG VinceG commented Feb 18, 2021

Q A
Bug fix? Yes
New feature? No
BC breaks? no
Related Issue Fix #200
Need Doc update no

Describe your change

This will add an empty condition to the whereIn method to make sure it'll add the condition only if the array is not empty. if it's empty it'll add a false condition to make sure the condition will stop ALL records from being returned.

What problem is this fixing?

Originally passing an empty array to whereIn resulted in ignoring the entire condition, this results in unexpected search results where a user might see more results than they should.

For example

// Returns all records matching the query ignores the id condition
// it'll return all results matching the search query and ignores completely the whereIn condition
$query = App\Models\Areas\Area::search('santa')->whereIn('author_id', [])->get();

With this fix, it'll do exactly what Laravel does with its query builder where if the array is empty it'll append a false condition.

// Returns all records matching the query appends 0 = 1 similar to how laravel handles the whereIn with empty array
// https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/Query/Grammars/Grammar.php#L271
$query = App\Models\Areas\Area::search('santa')->whereIn('id', [])->get();

added a corresponding test to make sure everything passes with this change.

…dition added instead of ignoring the condition altogether
@VinceG
Copy link
Contributor Author

VinceG commented Feb 18, 2021

Looks like the tests are failing but not related to this PR.

@DevinCodes
Copy link
Contributor

I re-ran the tests so that the credentials work properly (sorry for the inconvenience on that).

It seems like there are some PHPStan errors that make the CI fail, but this is unexpected. There's an issue on the PHPStan repo that describes the same situation we're encountering. We have to wait until this commit is released, or ignore the errors for now through the phpstan.neon.dist file.

@DevinCodes
Copy link
Contributor

@VinceG Let's unblock this, and ignore the PHPStan errors for now. I'll make sure keep track of this in the coming days/weeks.

Can you please ignore the errors for now by adding the following to the phpstan.neon.dist file, under the ignoreErrors key? Thank you in advance!

- '#Cannot call method withScoutMetadata\(\) on class-string\|object.#'
- '#Cannot call method getScoutKey\(\) on class-string\|object.#'
- '#Cannot call method toSearchableArray\(\) on class-string\|object.#'

@VinceG
Copy link
Contributor Author

VinceG commented Mar 3, 2021

@DevinCodes Done.

Copy link
Contributor

@DevinCodes DevinCodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR @VinceG ! I'll merge and release this soon 🚀

@DevinCodes DevinCodes merged commit 3bebf8d into algolia:master Mar 10, 2021
@VinceG VinceG deleted the bugfix/200-empty-where-in branch March 10, 2021 17:37
@DevinCodes
Copy link
Contributor

This fix is released in v1.14.0 🎉

Thank you again for your contribution!

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.

[bug] Passing an empty array to ->whereIn() ignores the whole statement
2 participants