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] in/not in operators #28192

Merged
merged 1 commit into from
Apr 12, 2019
Merged

[5.8] in/not in operators #28192

merged 1 commit into from
Apr 12, 2019

Conversation

jerguslejko
Copy link
Contributor

This PR introduces the ability to pass in or not in strings as an operator to the query builder.

// these two calls produce the same query
$query->where('foo', 'in', [1, 2]);
$query->whereIn('foo', [1, 2]);

// these two calls produce the same query
$query->where('foo', 'not in', [1, 2]);
$query->whereNotIn('foo', [1, 2]);

The reasons behind this PR:

  • being able to pass-in these strings as operators allows for nicer query generation:

    [$name, $operator, $value] = $filter->get(); // Filter model stored in the database
    
    // This PR allows for this
    $query->where($name, $operator, $value);
    
    // Instead of this
    if ($operator == 'in') {
        $query->whereIn($name, $value);
    } else if ($operator == 'not in') {
        $query->whereNotIn($name, $value);
    } else {
        $query->where($name, $operator, $value);
    }
  • in the current implementation, it is kind of unintuitive that $query->where('foo', 'in', [1, 2]) produces where foo = "in"

Thanks for reviewing this.

@jerguslejko jerguslejko changed the base branch from master to 5.8 April 11, 2019 18:36
@donnysim
Copy link
Contributor

What will happen if you do $query->where('direction', 'in') etc.? Like you're aiming for direction = 'in'?

@jerguslejko
Copy link
Contributor Author

@donnysim if the where method is called with 2 arguments, the = operator is assumed.

Therefore, $query->where('direction', 'in') will generate direction = 'in'

@donnysim
Copy link
Contributor

Is there a test for such case? It would feel safer if there is one in case someone tampers further with it in the future, but I do like this change 👍

@jerguslejko
Copy link
Contributor Author

@donnysim this is the same issue as doing $query->where('foo', '=') - I want all rows where the foo field has an equal sign in it. This is already handled by the framework

@ahinkle
Copy link
Contributor

ahinkle commented Apr 11, 2019

I don't think we should over complicate the query builder with multiple different types of syntax operations. The merging of this opens the door for many more PRs with alternative constructors.

@jerguslejko
Copy link
Contributor Author

@ahinkle the query builder already supports this type of querying. $query->where('foo', 'like', '%a%') works this way

@ahinkle
Copy link
Contributor

ahinkle commented Apr 11, 2019

Yes, we don't have a whereLike; as opposed to the already existing whereIn.

@jerguslejko
Copy link
Contributor Author

@ahinkle let's wait and see what @taylorotwell thinks

@bonzai
Copy link
Contributor

bonzai commented Apr 11, 2019

#15219
#15266

@taylorotwell taylorotwell merged commit bc32756 into laravel:5.8 Apr 12, 2019
@jerguslejko
Copy link
Contributor Author

thanks taylor

@Cannonb4ll
Copy link
Contributor

Cannonb4ll commented Apr 18, 2019

Please see referenced issue and PR, this PR is completely breaking withPivot(), I also fail to see how this is helpful in any way since we already had the explicit whereIn() and whereNotIn() methods.

@duxthefux
Copy link
Contributor

it also uses weak == comparison which breaks the case ->where("col", true) and ->where("col", false)...took me an hour to debug this. Why are people using == instead of ===...


 >>> 'in'==0
=> true

'>>> 'in'==true
=> true

@decadence
Copy link
Contributor

Reverted here: 04a547e

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.

8 participants