[5.5] Letting where in query builder take an array

[machuga]

Description

There was a discussion on internals, laravel/ideas#572,
about adding in as an operator for where. I riffed on it and ignored adding in as
a focus and simply mapped values of arrays to be passed along to whereIn.

This functionality is similar to where doing something like

where('id', '=', null) translates to whereNull('id')

Now where('id', [1,2]) or where('id', '=', [1,2])
will translate to whereIn('id', [1,2]).

And, like where with null will map any operator other than =
to whereNotNull, we map it to whereNotIn.

This functionality exists in other ecosystems, which doesn't necessarily warrant that
it should be part of ours, but I had some extra time and decided to make a PR for it
because I haven't made a Laravel PR in forever, and it was something some folk seemed
interested in. Low hanging fruit and what not. As such, I submit it for feedback.

Known Concerns

• Expanding the implicit API of where
• More code to maintain
• Would possibly prevent errors from being raised when people accidentally send arrays to where clauses
• Because arrays and associative arrays are the same data structures, there could be some weird behaviors (the same that whereIn could see).
• API is shoddy with 3 args, where('id', '=', [1,2]) looks silly and incorrect. We allow the same for null, but it's not ideal.

[fitztrev]

whereIn() allows $values to be instanceof Arrayable. Should this, too? That way you could pass it a collection.

[machuga]

Ya I think that'd make sense. I'll patch it today.

[sisve]

Does this also work with ...

$builder->where([
    ['col1', [1, 2, 3]],
    ['col2', [1, 2, 3]]
]);

How about those other where-methods that aren't already accepting arrays, like whereDate, whereMonth, whereYear?

Note: This functionality will conflict with future development in #18229 if the syntax $builder->where('money', new Money(1337, 'USD')) is implemented and the value objects implement Arrayable.

[sisve]

Also... this code overrides/ignores the $operator, and assumes that everyone passing in array/Arrayable really wanted whereIn.

$user = User::find($request->input('user_id'));

And since ::find($id) just calls $this->where('id', '=', $id)->first(), you suddenly start accepting arrays into ::find and will return the first matching value. This also affects ::delete which does something similar, and you now get support for deleting multiple records at once.

This change, in its current form, affects many more methods than expected.

[sisve]

Okay, so ... I had a major brainfreeze. I forgot about the Eloquent\Builder code. So the Model::find and Model::delete issues are invalid, the issues are about the Database\Builder. Instead of the Model:: prefix and think DB::table('users')-> instead.

$user = DB::table('users')->find($id);
DB::table('users')->delete($id);

On the other hand, that brainfreeze may still be in effect and I am totally missing something.

[lagbox]

@sisve Seems the main concern might be that if you passed an array by mistake when you didn't really want to, the query would still happen, where previously you would get some type of error due to conversion of type that would make it obvious what you passed wasn't what you expected?

[machuga]

Passing a two dimensional array into where as the first argument gets handled by the top pre-existing conditional in that method.

It doesn't override operator, it instead utilizes existing conventions and functionally that occur.

I see references in a comment of the PR you linked to a hypothetical API, but that will be up to the implementation. If he implements that API, his conditional is higher, and has a stronger specificity, then this should be fine.

We can of course tweak the implementation here to loosely accept any iterable in general as well.

As for find, I agree that someone passing it in may do so unintentionally, and I cover that in the Known Concerns section.

Sorry for lack of code inlining and my general shortness - writing on a phone half awake 😄

[sisve]

I should probably clarify my stance on this issue. My earlier comments are mostly about noticing changes that needs to be documented. I am not against the feature, I just want it properly documented in the changelog/upgrade notes that functions that previously assumed single primary keys now supports arrays. People should already be validating their inputs, but that is not always the case...

[ConnorVG]

The main issue I find with this is the inherent need to then support the other comparators. Sure, = and <> / != are extremely simple WHERE IN / WHERE NOT IN but what if people put in > or < etc? We'd then be expected to support the natural query building of WHERE (X > Y OR X > Z).

This gets a strong no from me.

[machuga]

@ConnorVG I'm not sure the need is inherent. I implemented it the current way because it's identical to how null is handled. I don't think there is any protection in stopping people from doing `where('foo', '>', null), and I don't think Laravel is expected to do prevent anything silly with it, so perhaps this assumption is not very likely.

Rails has never supported it and I don't think I've seen any Python or other ORMs that'd allow it either.

Rails example:

irb(main):001:0> User.where('id > ?', [1,2])
User Load (6.5ms)  SELECT  "users".* FROM "users" WHERE (id > 1,2) LIMIT $1  [["LIMIT", 11]]
ActiveRecord::StatementInvalid: PG::DatatypeMismatch: ERROR:  argument of WHERE must be type boolean, not type record

Overall I'd be in favor for not allowing arrays to be passed to where/3 (where the = is passed in), and just allowing it with where/2, but that's not how we handle other things in this method.

So while I understand that concern, I'll point out that this is not creating a new potential issue. It piggybacks on the existing issue null and possibly other values are handled in this situation. It's not terrible, not ideal, but is par for the course - adds convenience while possibly being misunderstood.

Either way I'll ensure it's documented properly under the Concerns section above.

[ConnorVG]

@machuga the problem is that with a primitive understanding of null, there will be an inherent lack in comparator attempts other than equal to or not equal to. When allowing arrayable data to be passed as the comparison, this quality is lost. The values are much more comparable, therefor I fully expect people the then expect the alternate comparators to be supported. Even if this is not the case directly out of the gate, someone (probably multiple people) will eventually submit issues and/or PRs to implement this feature which just (in my opinion) induces bloat and unreadable usage.

[machuga]

Hmm I certainly understand the concern, but not entirely sure I agree with it based on the precedence already set by the ORM and other areas for convenience in Laravel. Either way I'm very happy this discussion is covering multiple viewpoints of the API spreading 👍

[taylorotwell]

I think I want to maybe hold off on this for now because I don't feel it's super clear or known how this will affect wider APIs and methods.

[machuga]

Works for me!