DBTNG/EQ condition works inconsistently with arrays

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.

The last submitted patch, 38: 2823910-37.patch, failed testing. View results →

Back to RTBC.

The last submitted patch, 38: 2823910-37.patch, failed testing. View results →

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

Automatically closed - issue fixed for 2 weeks with no activity.

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.

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 💪

Automatically closed - issue fixed for 2 weeks with no activity.