Problem/Motivation

#2875033 optimized joins and table selection in the SQL entity query implementation.

As part of that change, special handling was added for multi-value fields used in separate andConditionGroup()conditions. Separate condition groups need separate joins so that different values can be matched on different items of the same multi-value field.

For example:

$query = \Drupal::entityTypeManager()
  ->getStorage('node')
  ->getQuery()
  ->accessCheck(FALSE);

$query->condition(
  $query->andConditionGroup()
    ->condition('field_tags.target_id', $tag1_tid)
);

$query->condition(
  $query->andConditionGroup()
    ->condition('field_tags.target_id', $tag2_tid)
);

$result = $query->execute();

This should find entities where field_tags contains both $tag1_tid and $tag2_tid.

The implementation added by #2875033 contains the following condition in Drupal\Core\Entity\Query\Sql\Tables:

if ($index_prefix === '' && $field_storage->getCardinality() > 1) {
  $index_prefix = $condition_prefix;
}

This works when a multi-value field has a finite cardinality greater than 1, for example 2, 3, etc.

However, it does not work when the field has unlimited cardinality.

Drupal represents unlimited cardinality using:

FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED

which has the value `-1`.

Therefore:

$field_storage->getCardinality() > 1

evaluates to FALSE for an unlimited field.

As a result, an unlimited field is not treated as multi-value by this logic. The same field table alias can be reused for separate andConditionGroup() conditions instead of creating the separate joins required to match values on different field items.

This causes an EntityQuery that should match entities containing both values to return no results.

This appears to be a regression introduced by #2875033.

Steps to reproduce

  1. Create a content type with a multi-value entity reference field, for example field_tags.
  2. Set the field cardinality to **Unlimited**.
  3. Create an entity where field_tags contains two different values, for example term IDs 1 and 2.
  4. Execute an EntityQuery using separate andConditionGroup() conditions:
$query = \Drupal::entityTypeManager()
  ->getStorage('node')
  ->getQuery()
  ->accessCheck(FALSE);

$query->condition(
  $query->andConditionGroup()
    ->condition('field_tags.target_id', 1)
);

$query->condition(
  $query->andConditionGroup()
    ->condition('field_tags.target_id', 2)
);

$result = $query->execute();
  1. Observe that the query does not create separate joins for the two conditions and the entity is not returned.
  2. Change the field cardinality from Unlimited to a finite value greater than 1.
  3. Run the same query again.
  4. Observe that separate joins are generated and the entity is returned as expected.

Proposed resolution

Use the field storage definition's isMultiple() method instead of checking whether the cardinality is greater than 1.

Change:

if ($index_prefix === '' && $field_storage->getCardinality() > 1) {
  $index_prefix = $condition_prefix;
}

to:

if ($index_prefix === '' && $field_storage->isMultiple()) {
  $index_prefix = $condition_prefix;
}

isMultiple() correctly handles both finite multi-value cardinalities and CARDINALITY_UNLIMITED.

Add test coverage for separate andConditionGroup() conditions on a field whose cardinality is FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED.

Issue fork drupal-3622419

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

dench0 created an issue. See original summary.

dench0’s picture

dench0’s picture

Status: Active » Needs review
amateescu’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed the MR and the fix is correct. Bugs are fixed in the main branch first, then backported to the lower ones, so I've re-targeted the MR.

amateescu’s picture

Version: 11.4.x-dev » main
larowlan’s picture

Priority: Normal » Major
valthebald’s picture

+1 to RTBC. Regression is real, and suggested patch fixes it correctly

  • catch committed 802f07cf on 11.4.x
    fix: #3622419 EntityQuery AND condition groups do not work for fields...

  • catch committed c4ec0e2c on 11.x
    fix: #3622419 EntityQuery AND condition groups do not work for fields...
catch’s picture

Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Good find, the fix itself is easier to read than the issue title which is always nice.

Committed/pushed to main, and cherry-picked to 11.x and 11.4.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed c0277ee2 on main
    fix: #3622419 EntityQuery AND condition groups do not work for fields...