Subselects don't work in DBTNG conditions, except when used as value for IN

I can confirm this bug.

Yep, it was never intended to support sub-queries used as scalars. Let's see how to add support for this in Drupal 8.

I have updated the documentation at http://drupal.org/node/310086 to make it clear that for now this only works with IN.

This patch worked fine for me on manual testing on my use case.

+++ b/includes/database/query.incundefined
@@ -1633,7 +1633,7 @@ class DatabaseCondition implements QueryConditionInterface, Countable {
+    if (!isset($operator) && !($field instanceof QueryConditionInterface)) {

Can you explain this change? Isn't it a valid use case to use condition($subquery, $values); ? Perhaps some kind of comment helps as well.

+++ b/includes/database/query.incundefined
@@ -1727,67 +1727,97 @@ class DatabaseCondition implements QueryConditionInterface, Countable {
+              $arguments[$placeholder] = $value;
+              $value_fragment[] = $placeholder;

Just for similarity on all other places first the value/field_fragment is created then the argument, this should probably be switch here.

In general the code is now even a bit easier to understand and described in detail.
0 days to next Drupal core point release.

$query->condition($value, $count_subquery, '=');

a subquery returns a set, a set can't be equal to a scalar. What is this issue?

OK I eat my words. Apparently scalar subqueries http://kb.askmonty.org/en/scalar-subqueries are a standard feature where SELECT returns 1 or 0 rows.

Hell knows whether this is ANSI. http://www.akadia.com/services/ora_scalar_subquery.html Oracle 9. http://www.databasejournal.com/features/mssql/article.php/3407911/Scalar... MS SQL back in 2004. And so forth.

Damien tells me in IRC this is standard. SQL--

#7: 1267508-subselects.patch queued for re-testing.

please ignore the above request.

I want to do this:

SELECT sid
FROM `uc_shipments`
WHERE order_id NOT
IN (
  SELECT order_id
  FROM `uc_orders`
)

...or...maybe better:

SELECT s.sid
FROM `uc_shipments` s
WHERE NOT
EXISTS (
  SELECT NULL
  FROM `uc_orders` o
  WHERE s.order_id = o.order_id
)

All of this is a bit over my head. Does the fact that this issue isn't fixed mean that I can't do this with the regular db_select() calls? See http://drupal.stackexchange.com/questions/18497/how-do-i-use-not-in-in-a....

You can certainly do this with notExists($sub_query) ... in general: try to avoid posting somehow support questions in non-support issues. This doesn't really help to move the issue forward, so better test the patch for example.

Hi, sorry to necro this one, as the last comment was about a year ago.

Is there still need for work on this post? What is the current status? I'm willing to work on this issue if it there isn't a solution.

Please look at #1837118: UPDATE foo SET bar=(SELECT...) is not supported for a similar problem + accepted solution.

Yes the issue is certainly valid I was just suggesting the other issue for the architectural solution, calling compile and such . If this is not enough, I can dig it up what we did over there, I certainly can't remember it's been *ages* since that one but I'd be glad to do dig into it and help this issue be fixed.

Here's a much simpler patch more suited to the current state of D8. I've marked this as a bug report and not a feature request since the documentation suggests this should work, and this is standard SQL syntax. The -FAIL.patch adds a test that demonstrates the problem, and the other patch has the test and the fix.

I have to agree with fietsenwin. The fix from lisa.ugray is a good find, but for me a quit and dirty fix. I do like the fix from fietsenwin.

I was not able to do a easy reroll for that fix from the fietsenwin patch. The tests for that patch where easily rerolled. The tests from the patch are the ones we need to test any solution.

I would like to get this issue fixed. And I will do so by reviewing.

I will do a full review later. But for now the testbot for SQLite is returning the syntax error: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1 near "(": syntax error. You made a lot of changes where you removed the space before the round bracket. I think that is the source of the SQLite bugs.

Some remarks after a short review:

1. +++ b/core/lib/Drupal/Core/Database/Query/ConditionInterface.php
   @@ -12,13 +12,21 @@
   +   * If called with 1 parameter, it should be a ConditionInterface that in
   +   * itself forms a valid where clause. Use e.g. to build clauses with nested
   +   * AND's and OR's.
   

   Not sure if we have testing for this. I cannot find it.

2. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -158,86 +163,127 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
   +        else if (!isset($condition['operator'])) {
   

   Nitpick: Must be elseif.

3. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -158,86 +163,127 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
   +        if ($condition['field'] instanceof ConditionInterface) {
   ...
   +          if ($condition['field'] instanceof SelectInterface) {
   

   Can you explain this one to me. Can $condition['field'] be an instance of ConditionInterface and SelectInterface?

4. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -300,15 +346,18 @@ protected function mapConditionOperator($operator) {
   +      'NOT BETWEEN' => array('delimiter' => ' AND '),
   

   We have #2686483: Add support for database condition operator "NOT BETWEEN" for adding the "NOT BETWEEN" operator. If this is needed we can postpone this issue on that.

5. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -158,86 +163,127 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
   +            if ($value instanceof SelectInterface) {
   

   Are we allowing that there be an array of select statement in the $condition['value']? This is something that I do not like. Maybe a test to make sure that it is not allowed.

@fietserwin: Thank you for your explanations. Your two changes for the patch from comment #47 look good.

I you want to support multiple select queries at right hand side, then extra tests for that are needed.

Minor: Asserts messsages doesn't need to use t().

For the testbot.

The problem as stated in the issue summary is fixed with this patch.
There are tests added to check the fix.
The patch looks good to me.
I am giving this a RTBC.

Good work fietserwin!

Needs a reroll, because #2686483: Add support for database condition operator "NOT BETWEEN" is in.

The test failures for PostgreSQL are not related to this patch.
The reroll looks good.
Back to RTBC.

1. This looks like a nice improvement to the database API - I think we need a change record to tell people about it.

2. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -301,15 +347,17 @@ protected function mapConditionOperator($operator) {
   -      'EXISTS' => array('prefix' => ' (', 'postfix' => ')'),
   -      'NOT EXISTS' => array('prefix' => ' (', 'postfix' => ')'),
   ...
   +      // Exists expects an already bracketed subquery as right hand part. Do
   +      // not define additional brackets.
   +      'EXISTS' => array(),
   +      'NOT EXISTS' => array(),
   

   I wonder what the impact for other db drivers this will have.

3. +++ b/core/lib/Drupal/Core/Database/Query/ConditionInterface.php
   @@ -131,6 +149,9 @@ public function notExists(SelectInterface $select);
   +   * @return array
   +   *   The, possibly nested, list of all conditions (by reference).
   

   This looks like a pretty tricky API change.

4. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectSubqueryTest.php
   @@ -93,6 +93,95 @@ function testConditionSubquerySelect() {
   +    $this->assertEqual(count($people), 2, 'Returned the correct number of rows.');
   

   There's assertCount()

5. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectSubqueryTest.php
   @@ -93,6 +93,95 @@ function testConditionSubquerySelect() {
   +    $this->assertTrue(in_array('Paul', $people) && in_array('John', $people), 'Returned Paul and John.');
   

   There are special phpunit asserts for this ie. assertArraySubset()

6. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectSubqueryTest.php
   @@ -93,6 +93,95 @@ function testConditionSubquerySelect() {
   +    $this->assertEqual(count($people), 1, 'Returned the correct number of rows.');
   +    $this->assertTrue(in_array('John', $people), 'Returned John.');
   ...
   +    $this->assertEqual(count($people), 2, 'Returned the correct number of rows.');
   +    $this->assertTrue(in_array('George', $people), 'Returned George.');
   +    $this->assertTrue(in_array('Paul', $people), 'Returned Paul.');
   

   As above

@fietserwin: One test fail to fix: testConditionSubquerySelect2 with "fail: [Other] Line 98 of core/tests/Drupal/KernelTests/Core/Database/SelectSubqueryTest.php:
0 => 'Paul'".

I shall write the change record.

Change record added.

The test failure for SQLite has noting to do with the patch for this issue.
All remarks of alexpott are addressed.
All changes look good.
Back to RTBC.

Back to RTBC.

Discussed @catch, @effulgentsia and @cottser we agree this needs to be in before the beta is cut on Tuesday. If this is not in before then it'll need to be for 8.3.x because whilst this probably won't impact other drivers - we can not be sure.

It'd be great if a sub system maintainer could have a look at this. Afacis the solution is solid and works great and has test coverage.

First off, I *adore* how detailed the comments are in this patch. Well done!

The code overall looks fine as is, and if committed as is I wouldn't block it. However, I do have a few small nits:

1. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -158,86 +163,127 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
   +            if ($condition['value'] instanceof SelectInterface && ($operator['operator'] === 'IN' || $operator['operator'] === 'NOT IN')) {
   

   Total not-blocking nitpick: I generally prefer to convert double-conditionals like this to an in_array(), as it's shorter and easier to read. (Vis, in_array($operator['operator'], ['IN', 'NOT IN']) )

2. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -158,86 +163,127 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
   +          $value_fragment = array();
   

   May as well use short-array syntax while we're here.

3. +++ b/core/lib/Drupal/Core/Database/Query/ConditionInterface.php
   @@ -32,33 +40,43 @@
   +   *   in itself. Use where(), if you would like to add a more complex condition
   

   No comma after where().

One larger, note, though, is that the compile() method is now quite long (even longer than it was before), and very deeply nested. That results in a very high cyclomatic complexity. Is there any way we can break it up into a series of sub-methods? At minimum, it looks like the main foreach()'s body could get split off to a sub-method. There may be others, or it may just to too complex to do, but it would be great if we could at least try.

If doing so is the difference between this patch being committed to 8.2 or not, though, I'm OK with committing it and tidying that up later.

The SQLite and Postgres fails are concerning, though. #69 indicates they may be spurious, so I defer to the committers on that front.

@Crell said:

If doing so is the difference between this patch being committed to 8.2 or not, though, I'm OK with committing it and tidying that up later.

Removing the tag "Needs subsystem maintainer review".

Created a followup #2775819: The method Drupal\Core\Database\Query\Condition::compile is too long and complex for:

One larger, note, though, is that the compile() method is now quite long (even longer than it was before), and very deeply nested. That results in a very high cyclomatic complexity. Is there any way we can break it up into a series of sub-methods? At minimum, it looks like the main foreach()'s body could get split off to a sub-method. There may be others, or it may just to too complex to do, but it would be great if we could at least try.

I think that the 3 minor points of comment #74 can be fixed on commit.

Thanks @Crell for the sub system maintainer review..

There's been some discussion of whether this is a bug or feature - let's split the difference and call it a task.

Committed 22a241c and pushed to 8.2.x. Thanks!

diff --git a/core/lib/Drupal/Core/Database/Query/Condition.php b/core/lib/Drupal/Core/Database/Query/Condition.php
index a6bb762..24a3582 100644
--- a/core/lib/Drupal/Core/Database/Query/Condition.php
+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -255,7 +255,7 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
             $condition['value'] = array($condition['value']);
           }
           // Process all individual values.
-          $value_fragment = array();
+          $value_fragment = [];
           foreach ($condition['value'] as $value) {
             if ($value instanceof SelectInterface) {
               // Right hand part is a subquery. Compile, put brackets around it
diff --git a/core/lib/Drupal/Core/Database/Query/ConditionInterface.php b/core/lib/Drupal/Core/Database/Query/ConditionInterface.php
index c9b6333..2310e03 100644
--- a/core/lib/Drupal/Core/Database/Query/ConditionInterface.php
+++ b/core/lib/Drupal/Core/Database/Query/ConditionInterface.php
@@ -42,7 +42,7 @@
    *
    * @param string|\Drupal\Core\Database\Query\ConditionInterface $field
    *   The name of the field to check. This can also be QueryConditionInterface
-   *   in itself. Use where(), if you would like to add a more complex condition
+   *   in itself. Use where() if you would like to add a more complex condition
    *   involving operators or functions, or an already compiled condition.
    * @param string|array|\Drupal\Core\Database\Query\SelectInterface|null $value
    *   The value to test the field against. In most cases, and depending on the

Fixed on commit. I can't do #74.1 on commit - it's a code change plus in_array() is a tricky beast - you (nearly) always want to make the 3rd argument TRUE so it doesn't produce unexpected results.

Looks this needs follow-ups - port to d7 and simplify code

One larger, note, though, is that the compile() method is now quite long (even longer than it was before), and very deeply nested. That results in a very high cyclomatic complexity. Is there any way we can break it up into a series of sub-methods? At minimum, it looks like the main foreach()'s body could get split off to a sub-method. There may be others, or it may just to too complex to do, but it would be great if we could at least try.

@andypost: There is a follow-up for the compile() method: #2775819: The method Drupal\Core\Database\Query\Condition::compile is too long and complex.

It seems like this caused a test fail in TMGMT.

\Drupal\tmgmt_locale\Tests\LocaleSourceUiTest is failing.

We have a condition like this:

$select->isNull("lt_$missing_target_language.language");

$missing_target_language in this case is 'gsw-berne'. That used to be converted to gswberne, but no longer.

Maybe that's on purpose and can't be avoided, but I'm not sure?

We already have other places where we explicitly escape..

The code is in \Drupal\tmgmt_locale\LocaleSourcePluginUi

The failing test is in my comment, you can also see it here: https://www.drupal.org/pift-ci-job/413927, including the incorrectly generated query. I'm 100% sure that this issue changed that behavior, the only question is if that is a bug or by design. The test passes on the commit before this and fails with this.

Note that it only fails on the error_log parsing, on the terminal, the test is reported as passing.

Thanks for the feedback. We already opened #2783123: Fix escape condition in \Drupal\tmgmt_locale\LocaleSourcePluginUi, we'll try to implement your feedback there. Reviews would be great :)

I've reverted this due to #2783165: Follow up to "Subselects don't work in DBTNG conditions, except when used as value for IN" - that bug is worse than the limitation this fixes, and no need to have time pressure on fixing the security issue. Also bumping to 8.3.x

From #2783165: Follow up to "Subselects don't work in DBTNG conditions, except when used as value for IN":

The rewrite passed all existing and new tests on all supported databases and was thoroughly reviewed by the author and others. However 1 small change was not discovered: escaping plain fields, which was done in the old code but no longer in the new code.

The patch has added fix for the problem and also has added testing for it. The second patch has only the added testing.

Wrong patch for 1267508-92-without-the-fix.patch.

The patch with the fix and the added test passes and the patch with only the added test fails.

Ready for a good review.

@fietserwin: Thank you for the review!

For 97.1 and 97.3 Thanks and fixed!
For 97.2 That line is not being changed, but it is being moved. This is also happening in your own and previous commit patch from comment #67.

For:

I never touched this statement and it is also probably out of scope for this issue but I think that comment starting characters should also be part of the list of dangerous characters: for Mysql:#-/ (for #, -- resp. /*). Minus is an operator in itself but is not a comparison or logical operator.

and

I found some other minor points but they are not part of this patch:
- 80 character limit violations on some comments.
- 1 untyped property ($queryPlaceholder)
- missing operators in the map: !=, <> (will work but according to the comment a small "performance hit").

Please create follow-ups for those. I am happy to do the reviewing for them.

Testbot is happy.

@fietsenwin: Thanks and I updated the change record.

I would nice to confirm that the updated fix does not cause the same issue for @Berdir's contrib module.

@alexpott: That is what I did in the comments #92 and #94.

Back to RTBC.

19 fails in SQLite is OK?

@chx: The latest automatic testing without a patch has 16 fails. See https://www.drupal.org/pift-ci-job/435525.

I created an issue for the problems with the testbot for PHP 5.5 & SQLite. See #2791163: Random automatic testing failures on SQLite with PHP 5.5.

Look, it's fine by me, as far as I am concerned @daffie is already the SQLite maintainer, I already encouraged them to make it official.

Committed/pushed to 8.3.x, thanks!