Throw a helpful exception for empty IN conditions in Database\Query\Condition

That doesn't make much sense for me. If you do an empty IN, it should be unconditionally false.

Well as is I believe it's an SQL syntax error. I agree we should do better than that. The question is what we should do instead:

1) Skip it, as #0 does.
2) Force it to a WHERE 1=2 (or other such condition that is unconditionally false)
3) Throw an exception, since code upstream screwed up. (That's what we do for mixing values and default_values in insert queries, for instance.)

I agree with doing something about it, ie not won't fix. Which something we should do, though, I'm not sold either direction yet.

Thinking aloud, silently ignoring an empty IN clause, if you didn't realize the variable was empty, could result in data loss, no? That is, if you had the equivalent of this query:

DELETE FROM {mytable} WHERE a=:a and b IN (:b)

And :b is unexpectedly empty, then the query would delete likely more than intended. If :b had one value, it would delete a small number of items. If :b is empty, the IN clause is dropped, and it would delete a huge number of things.

That seems very bad. :-)

So I think that rules out "ignore empty". That leaves replacing IN () with an always-false statement, or raising a more useful error. (MySQL at least tends to not have the best error messages; providing better errors via sane and documented exceptions where feasible is, IMO, better than letting MySQL handle it.)

+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -76,6 +76,10 @@ public function condition($field, $value = NULL, $operator = NULL) {
+      throw new \InvalidArgumentException(sprintf("Query condition '%s %s ()' cannot be empty.", $field, $operator));

I believe this formats to "Query condition 'provider IN ()' cannot be empty." That seems fine.

Most of the DB layer, I believe, is subclassing base exceptions and throwing descriptive exceptions. So this is probably something like InvalidQueryException extends \InvalidArgumentException implements DatabaseException. That way, all database exceptions can still be caught by catching DatabaseException.

Was thinking the same as @Crell when I read the issue title and summary, so updated to what we are actually doing.

Added the exception class and tests.

I like the new title, too. Thanks, Berdir.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.php
@@ -481,4 +482,29 @@ function testInvalidSelectCount() {
+  function testEmptyInCondition() {
+    try {
+      db_select('test', 't')
+        ->fields('t')
+        ->condition('age', array(), 'IN')
+        ->execute();
+    }
+    catch (InvalidQueryException $e) {
+      $this->assertEqual("Query condition 'age IN ()' cannot be empty.", $e->getMessage());
+    }
+
+    try {
+      db_select('test', 't')
+        ->fields('t')
+        ->condition('age', array(), 'NOT IN')
+        ->execute();
+    }
+    catch (InvalidQueryException $e) {
+      $this->assertEqual("Query condition 'age NOT IN ()' cannot be empty.", $e->getMessage());
+    }
+  }

This test should fail if the expected exception is not thrown.

That already happens, as an exception is thrown if that query is executed.

We can add a $this->fail() below, but we'll never reach it, even if the expected exception is not thrown (aka in a test-only patch)

@Berdir I saw that... but say someone changes the code in Condition to not throw an exception and do option 2 from the issue summary - this test won't fail and will be probably hang about confusing people. This is why PHPUnit's expected exception annotation is so great :)

If we were using PHPUnit for these tests, then yes we should just do that. We're not, however, and that test refactor is out of scope here.

That said, I think Alex is correct that we need to test the negative case, too. Probably just set a variable inside the catch, and then pass/fail based on whether the variable is set to "yep, an exception happened".

And while we're at it, let's split that test method into 2. It shouldn't be one method. That's a Drupal-is-dumb pattern. :-)

We can just call this->fail().

And I don't agree with multiple methods, I'm testing the exact same code base there, and even though it's a DUBT test now, test methods still have quite some overhead. PHPUnit @expectedException would require that, but as you said yourself, we're not going there :) I'm just making sure that the exception message is dynamic and contains the condition, having this in the same test is imho more self-explaining.

Eh, or that I guess. These mega tests just make me cry every time I see them.

+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -76,6 +77,10 @@ public function condition($field, $value = NULL, $operator = NULL) {
+    if (is_array($value) && empty($value)) {

Very minor micro-optimization but can we reverse the conditions here?

Fixed. Back to RTBC.

Committed/pushed to 8.x, thanks!