DBTNG/EQ condition works inconsistently with arrays

So, it looks to me as though the condition code is lacking validation.

I agree there should be no surprise. Both should work or both shoudl fail - bet we also don't validate BETWEEN conditions which should take an array of exactly 2 elements

Just a quick comment regarding the tone of the issue summary: http://xjmdrupal.org/blog/someone-worked-hard-on-it

@chx
This is purely about being nice to people and enable people to participate in the discussion. Here is an alternative version of the issue summary. It contains less sentences, which don't add anything to the issue itself,
while making it clearer for people to understand what is going on.

As of #2358079: Require a specific placeholder format in db_query() in order to trigger argument expansion, and require explicit 'IN' parameter for conditions db_query() requires :foo[] when you pass in an array as arguments. On top of that, it also requires to explicit define the 'IN' parameter, even when using the SELECT objects.

Let's have a look at an actual example:

// This part doesn't work.
$select->condition('foobar', [1, 2]);
// This part works.
$select->condition('foobar', [1]);
// This part works as well.
$select->condition('foobar', [1], 'IN');

There is a clear inconsistency between passing in one or multiple parameters.

Possible solutions:

1. Don't make 'IN' requires when you pass in multiple values
2. Make 'IN' optional again

@todo:
Describe security implications, which might be totally fine at the end.

Thank you

For my part I think solution 1 makes the most sense.

I say that based on the fact that automatic IN query support was one of several contributing factors that made SA-CORE-2014-005 easily exploitable. The developer should know whether they are looking up a single value or a list.

Here's a rough start at adding more validation.

It's a little tricky since we access such random things for every parameter.

Hmm, NULL is not scalar. Silly PHP.

Nice - uncovered a bug in UserStorage class

This came up as a daily Bug smash target

This appears like it could be still relevant in D9.5 (Correct me if I'm wrong)

Rerolled but leaving in NW as tests are needed.

Error generating interdiff as the starting patch was done a while ago. Started with #17

Adding “needs tests” tag.

Also before fixing the existing tests is the validation still needed?

Added the testing that was needed and made the change more simple.

Style guide fix.

Small improvement.

Fixed the test failure for Drupal\Tests\Core\Database\ConditionTest.

FYI please try to include interdiffs, just helps.

Ran the tests local without the fix and confirmed they failed
Failed asserting that exception of type "Drupal\Core\Database\InvalidQueryException" is thrown.

I like the idea of throwing an exception.

Since this doesn't work I don't think it needs a change record. But will leave for committer to decide that.

Back to RTBC.

This needs a change notice and a release note snippet, as the behaviour change could break sites. And for that reason we'll only commit it to 10.1.x.

I'll have a go at both.

Additionally, test providers should use keyed arrays to convey additional information in the case of a fail, that can be fixed on commit though.

I added a change notice and added the behaviour change to the draft release notes.

Fixed on commit

diff --git a/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php b/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php
index 621f06c24b4..798b4dc5640 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php
@@ -600,15 +600,15 @@ public function testEmptyInCondition() {
    */
   public function providerNonArrayOperatorWithArrayValueCondition() {
     return [
-      ['=', '='],
-      ['>', '>'],
-      ['<', '<'],
-      ['>=', '>='],
-      ['<=', '<='],
-      ['IS NULL', 'IS NULL'],
-      ['IS NOT NULL', 'IS NOT NULL'],
-      ['', '='],
-      [NULL, '='],
+      '=' => ['=', '='],
+      '>' => ['>', '>'],
+      '<' => ['<', '<'],
+      '>=' => ['>=', '>='],
+      '<=' => ['<=', '<='],
+      'IS NULL' => ['IS NULL', 'IS NULL'],
+      'IS NOT NULL' => ['IS NOT NULL', 'IS NOT NULL'],
+      'Empty string' => ['', '='],
+      'Not set' => [NULL, '='],
     ];
   }

Published the change record

I'm that person who's site was broken. I have a view argument that works in 10.0 but throws this exception in 10.1. Wouldn't that sort of behavior normally be deprecated instead of break sites on a minor release?

Compromise, if count > 1, throw exception, else trigger deprecation?

Patch with the deprecation middle ground.

trying to fix the regression and provide a proper deprecation in 10.1.

Add back automated IN support. ( automatic IN query support was one of several contributing factors that made SA-CORE-2014-005 easily exploitable. ).

That was in query and it is no reason to cripple the query builder. There was no need to do that then or now. We have added more Drupalisms and degraded DX for no reason whatsoever. The job of the query builder is to do the sensible thing and it can easily do so seeing an array -- it's much harder for the string thing. So my vote is: a serious analysis should be made whether #2 is really detrimental to security for I believe it is not.

Since removing automatic IN() support didn't happen in this issue, putting it back should be its own issue rather than keeping going in this one.

I do think the main issue with SA-CORE-2014-005 was committing patches from needs work at midnight on Christmas eve more than trying to support array expansion as such.

#52 looks good to me. I wonder a bit if we want to trigger_error() E_USER_WARNING when count > 1 too, but if there are no known cases of this being relied upon, the exception is still clearer.

Committed and pushed to 11.x and cherry-picked to 10.1.x

Thanks for testing the new release before it comes out @neclimdul 💪