EntityQuery wrong SQL with two reference fields conditions targetting same entity type

Can confirm this is an issue.

Looks like its an issue with the second term reference not creating a join with a second alias to the taxonomy_term_field_data table.

After testing it locally, the query worked when adding INNER JOIN taxonomy_term_field_data taxonomy_term_field_data_2 ON taxonomy_term_field_data_2.tid = taxonomy_term_data_2.tid and changing the second WHERE clause to taxonomy_term_field_data_2

Looks like its probably coming from /web/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php in the addField() function.

Unfortunately, I haven't done much in D8 so I didnt get much further then that.

If someone with more expertise in this area could chime in, that would be great!

Let's in general plan to make this method easier to read, I think that would help everyone: #2766745: Refactor \Drupal\Core\Entity\Query\Sql\Tables::addField to be better readable

I wrote a testcase for it, which shows an alternative syntax which actually works at the moment.

The issue seems to be on how \Drupal\Core\Entity\Query\Sql\Tables::ensureEntityTable() checks if the current table already exists on $this->entityTables array.

The method use the $table value, which doesn't change if the same tables is joined (and aliased) multiple times.

Changing occurrencies of $this->entityTables[$index_prefix . $table] with $this->entityTables[$index_prefix . $base_table] fix the problem.
However I'm not confident with this class, so I'm not sure if this is the best way of addressing the problem.

I'll run @dawehner tests + additional manual test to understand if it's at least a definitive fix, and if it is I'll provide a patch. In the meanwhile it would be good to have thoughts anyway.

I wrote a testcase for it, which shows an alternative syntax which actually works at the moment.

@dawehner I'm not sure what you mean, as your test - correctly - fails on my local env (+ throw an exception).

I'm pulling down your testcase, fixing the exception, and adding my changes.
Works locally, but I've ran only EntityQuery related tests. Let's see what testbot says.

Drupal\KernelTests\Core\Entity::ensureEntityTable() $base_table can be either the aliased table name or 'base_table', which will be replaced with the right table later on in the process.
This patch adds the check for the condition above and uses the $base_table instead of $table only if it is an alias.

If test is green (is green locally, but due my configuration I have to skip BrowserTests and The ones using sqlite) this change will probably need database manager review.

We can't overwrite $table, it's needed in the in the $this->addJoin() second parameter. Handling the processed table name in a new variable.

Let's add a meaningful comment to the new test.

Tagging 'Needs subsystem maintainer review' as I'm not sure whether this is the best way to address the issue.
I have concerns also about the condition $base != 'base_table' and if there are other reserved table placeholders we would need to consider (for example I'm looking at \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema\getEntitySchemaTables() keys).

This is bug so it should be filled against 8.3.x.

What if we use only $base_table in the key. Basically this is only the static cache key. Let's see.

Added back the docs from #3, and made the test more compact.

I don't think this needs subsystem maintainer review. It's just a bug.

@claudiu.cristea patch at #7 uses $base_table instead of $table as key, and it fails due 'base_table' placeholder used instead of the real name.

I've also remove additional docs from #3 due being out of issue topic, but happy if we include those!

The call for a maintainer was due the placeholder, as I don't know if there are othe possibilities other than 'base_table'.

But let's see testbot response!

Your approach is correct.

@gambry

I've also remove additional docs from #3 due being out of issue topic, but happy if we include those!

I think it's in the scope because we need to understand what we store in $this->entityTables.

Also, in the test, the main entity MUST refer 2 distinct entities, as in the original patch, otherwise the "test only" patch will not fail. This patch restores the references to distinct entities.

Tidied-up a little bit the test.

Thanks for tidying up everything, looks really good now.
Just two questions:

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -307,10 +307,11 @@ public function isFieldCaseSensitive($field_name) {
   +        $key = $index_prefix . ($base_table === 'base_table' ? $table : $base_table);
   

   Do you think it's the case we explain what's happening in here?

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -339,6 +340,23 @@ protected function ensureFieldTable($index_prefix, &$field, $type, $langcode, $b
   +  /**
   +   * Adds a join to a given table.
   +   *
   +   * @param string $type
   [...]
   +   * @return string
   +   *   Returns the alias of the joined table.
   +   */
   

   I think it's in the scope because we need to understand what we store in $this->entityTables.

   I see, and it does make sense. So shouldn't we - for the same reason - tidy up the docblock of ensureEntityTable()?

Why not documenting all the parameters?
Also there was a missing period on the addJoin() documentation.

+1 for RTBC... but I cannot do it because I've contributed with code :)

FYI I've applied this patch and it also seems to have fixed my issue while creating or groups. If folks didn't realize this was ALSO creating issues there. Patched worked great. Below is an example of what was breaking:

  private function getRelatedMedia($termName) {
    $media = $this->queryFactory->get('media');
    $or_group = $media->orConditionGroup()
      ->condition('field_supply_chain_topic.entity.name', $termName)
      ->condition('field_health_sector.entity.name', $termName);
    $results = $media
      ->condition('bundle', 'resource')
      ->condition($or_group)
      ->execute();
//get left hand join, not a full join like you'd expect
    return $results;
  }

Thanks for the patch!

It's failing to patch on 8.3.x for me, not sure why yet:

$ git apply -v --index ~/Downloads/entityquery_wrong_sql-2759757-23.patch 
Checking patch core/lib/Drupal/Core/Entity/Query/Sql/Tables.php...
error: while searching for:
    return $this->fieldTables[$index_prefix . $field_name];
  }

  protected function addJoin($type, $table, $join_condition, $langcode, $delta = NULL) {
    $arguments = array();
    if ($langcode) {

error: patch failed: core/lib/Drupal/Core/Entity/Query/Sql/Tables.php:339
error: core/lib/Drupal/Core/Entity/Query/Sql/Tables.php: patch does not apply
Checking patch core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php...

Just fixed an array() to short syntax in addJoin() (line 78 of the patch), which was causing failure to apply.

Just nitpicks:

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -296,21 +296,46 @@ public function isFieldCaseSensitive($field_name) {
   +   *   The join type.
   

   Would help to say what the valid values are, or refer to some other docs that say that.

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -296,21 +296,46 @@ public function isFieldCaseSensitive($field_name) {
   +   *   The alias of joined table.
   

   missing 'the'.

3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -296,21 +296,46 @@ public function isFieldCaseSensitive($field_name) {
   +        // Ensure a unique key for by concatenating the index prefix and the
   

   'for by' - missing word.

4. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -296,21 +296,46 @@ public function isFieldCaseSensitive($field_name) {
   +        // base table alias.
   

   For maintainability, it would be nice to say here why we're doing this. Something like 'This ensures that if we join to the same entity table several times for different entity reference fields, each join gets a separate alias.'

This patch addresses feedback from #29.

I've also changed \Drupal\Core\Entity\Query\Sql\Tables::$entityTables doc to reflect changes to the array keys done from ensureEntityTable().

This patch makes very much sense, I could only find minor documentation issues that could be fixed on commit:

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -296,21 +299,49 @@ public function isFieldCaseSensitive($field_name) {
   +   * Joins the entity table, if necessary, and return the alias for it.
   

   Minor nitpick: return -> returns.

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -296,21 +299,49 @@ public function isFieldCaseSensitive($field_name) {
   +   *   Join type, can either be INNER or LEFT.
   

   The join type...

3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -296,21 +299,49 @@ public function isFieldCaseSensitive($field_name) {
   +   *   the 'base_table' placeholder.
   

   The comment seems to wrap a bit early, I think 'the' could stay on the first line.

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
   @@ -946,4 +949,54 @@ public function testInjectionInCondition() {
   +   * Test EntityQuery works when querying the same entity from two fields.
   

   Tests that...

P.S. Note that there's a test-only patch in #19 which proves that this is a real problem.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x, thanks!

Made the docs fixes on commit.