entity query nested conditions must use LEFT joins when any of the parent condition groups is using OR

So it looks like the first problem was already fixed in #2527064: Nested condition groups in entity queries are broken but not the second part.

Updating issue title and summary.

The way we now pass down that information about something being inside an OR condition group is very ugly, but it's the same pattern as we already use in there, so it made kind of sense to continue with that.

So if I understand it correctly the problem is the following: Even though we generally use and INNER join for tables required by AND conditions and a LEFT join for tables required by OR conditions, once we are nested inside of a top-level OR condition it's incorrect to use INNER join - even for AND conditions - because that will incorrectly limit the result set if the other "branch" of the OR comes true. Tagged needs issue summary update, because that could be expanded a bit IMO to clarify things.

I think the patch looks fine to me, in principal. Maybe we can find a better name for the property, I think hasOrCondition isn't 100% precise. It's really whether it's contained inside of an or condition, no? Now I'm not proposing we call the thing containedInOrCondition, but maybe we can fight something a littler nicer.

In any case we should document that variable on ConditionBase or wherever it makes sense (I don't the class graph well enough to say off-hand) and then initialize it to FALSE, then we can skip the empty() check.

...and yes, the "Needs tests" tag is already there. So someone has go down into the rabbit hole that is EntityQueryTest. Maybe look at #2527064: Nested condition groups in entity queries are broken for inspiration; I cried less than previously expected when writing that test. And we should run the tests here on all DB backends (drupalci++), to be sure we're not causing any trouble for them.

Maybe we can find a better name for the property.

I ended up using nestedInOrCondition.

In any case we should document that variable on ConditionBase or wherever it makes sense (I don't the class graph well enough to say off-hand) and then initialize it to FALSE, then we can skip the empty() check.

Done. Let me know, if this is better.

...and yes, the "Needs tests" tag is already there.

This was kind of a quick one, since it was enough to fix the already existing test added in #2527064: Nested condition groups in entity queries are broken so that it actually asserts the correct result set.

And we should run the tests here on all DB backends (drupalci++)

I'm not sure, what I would need to do here. Could you elaborate on that one or point me to the relevant documentation? That would be appreciated.

To run additional test, you click on "Add test/ retest" and then you can select what you want there. Added a test for PostgreSQL and Sqlite.

Patch looks good. The name of that variable makes more sense (even I thought it could also be $nestedInsideOrCondition, but in any case better than $nestedOrCondition). The test is correct to add missing red triangles (4 and 12).

Already reviewed in #8 so RTBC.

Thanks for investigating and fixing this tricky issue!

In general the solution looks sensible to me, I have only an architectural concern:

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
   @@ -36,11 +36,12 @@ public function compile($conditionContainer) {
   +        $sql_condition->nestedInOrCondition = strtoupper($this->conjunction) == 'OR';
   

   Adding a public member to the SQL condition does not look great to me: we are introducing a backwards dependency between SQL queries and Entity queries, which makes the relationship (softly) circular. Since apparently the SQL layer does not need this flag at all, we should try to move it into the entity query Condition class (and make it protected).

   I think something like this should work:

       $condition['field']->nestedInOrCondition = $this->nestedInOrCondition || strtoupper($this->conjunction) === 'OR';
       $condition['field']->compile($sql_condition);
   

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
   @@ -36,11 +36,12 @@ public function compile($conditionContainer) {
   +        $type = strtoupper($this->conjunction) == 'OR' || ($conditionContainer instanceof SqlCondition && $conditionContainer->nestedInOrCondition) || $condition['operator'] == 'IS NULL' ? 'LEFT' : 'INNER';
   

   Since we are changing this line, can we replace == occurrences with === ones?

I tried a slightly different test and it seems not be working:

    $query = $this->factory->get('entity_test_mulrev');

    $first_and = $query->andConditionGroup()
      ->condition($this->figures . '.color', 'red')
      ->condition($this->figures . '.shape', 'triangle');
    $second_and = $query->andConditionGroup()
      ->condition($this->greetings . '.value', 'merhaba')
      ->condition($this->greetings . '.format', 'format-tr');

    $or = $query->orConditionGroup()
      ->condition($first_and)
      ->condition($second_and);

    $and = $query->andConditionGroup()
      ->condition($or)
      ->condition($this->figures . '.color', '', '<>');

    $this->queryResults = $query
      ->condition($and)
      ->condition('type', reset($this->bundles))
      ->sort('id')
      ->execute();

    $this->assertResult(4, 6, 12, 14);

I would expect this to behave as the original test, since all items have a color, however the assertion fails. I think this means that we need to propagate somehow the flag value upwards in the condition tree.

4 and 12 don't have color. It's that that makes the INNER JOIN not work.

Thanks to both of you for your review!

The name of that variable makes more sense (even I thought it could also be $nestedInsideOrCondition, but in any case better than $nestedOrCondition)

Since I was updating the patch anyway, I changed the name of the property to $nestedInsideOrCondition.

Since apparently the SQL layer does not need this flag at all, we should try to move it into the entity query Condition class (and make it protected).

That's much better indeed. Thanks for coming up with that. It worked for me locally and it came back green as well, so I updated the patch.

Since we are changing this line, can we replace == occurrences with === ones?

Sure, I don't see why it shouldn't work with the strict typing.

Regarding #19, jcisio already pointed out, why the test didn't work, so I think we're still good to go.

Yep, this looks good now, #20 totally makes sense :)

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
@@ -36,11 +43,12 @@ public function compile($conditionContainer) {
+        $type = strtoupper($this->conjunction) === 'OR' || $this->nestedInsideOrCondition || $condition['operator'] === 'IS NULL' ? 'LEFT' : 'INNER';

Minor: could we swap the first two tests so that they match those in the IF branch? This makes it immediately visible that we are dealing with the same condition in both cases. We could even store them in a variable before entering the IF/ELSE code block.

I'm happy to move this back to RTBC once #23 is addressed :)

Ok, swapped so that IF matches ELSE. No interdiff because it's trivial.

Looks good to me, back to RTBC.

Saving credits

Committed e551206 and pushed to 8.6.x. Thanks!

Backported to 8.5.x as 5ed644d.