Impossible entity query with condition on revision metadata keys

Great find, also nice test! Looks pretty great to me, just one question:

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
@@ -187,15 +187,20 @@ public function addField($field, $type, $langcode) {
+          $revision_table = $entity_type->getRevisionTable();

Should we not check that the field storage is revisionable and maybe also that the field name is a revision metadata key?

Thanks, yeah that looks good to me. Will give one of the other entity maintainers the chance to take a look at this as well, but for me this is good to go.

Looks good to me as well. This patch shows how badly \Drupal\Core\Entity\Query\Sql\Tables needs to be rewritten after we finish all the issues from #2960147: Finalize the entity storage :/

#10 makes perfect sense.

Just thinking but might this be better to change:

        if ($all_revisions && $field_storage && $field_storage->isRevisionable()) {
          $data_table = $entity_type->getRevisionDataTable();
          $entity_base_table = $entity_type->getRevisionTable();
        }
        else {
          $data_table = $entity_type->getDataTable();
          $entity_base_table = $entity_type->getBaseTable();
        }

to

          if ($field_storage && $field_storage->isRevisionable()) {
          $data_table = $entity_type->getRevisionDataTable();
          if ($all_revisions) {
            $entity_base_table = $entity_type->getRevisionTable();
          }
          else {
            $entity_base_table = $entity_type->getBaseTable();
          }
        }
        else {
          $data_table = $entity_type->getDataTable();
          $entity_base_table = $entity_type->getBaseTable();
        }

That way we'll get one less join. OTOH this will lead to always using the revision table even when not needed so that's probably wrong. How about:

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
@@ -191,11 +192,18 @@ public function addField($field, $type, $langcode) {
+            $revision_table = $entity_type->getRevisionTable();
...
+        if ($revision_table) {
+          $entity_tables[$revision_table] = $this->getTableMapping($revision_table, $entity_type_id);
+        }

Change this to assign $data_table instead. The revision table contains all the data from the non revision data table right? And then we won't have to join across the three tables and have the later if.

I would really prefer not to try and optimize \Drupal\Core\Entity\Query\Sql\Tables::addField() at this point, it needs to be completely rewritten after #2960147: Finalize the entity storage.

Random failure.

This patch causes Drupal\KernelTests\Core\Entity\EntityQueryTest to fail.

The new test method needs to be marked as @legacy now due to #2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key.

Re-RTBCing.

@tstoeckler it's not a legacy test though. Legacy tests will be removed in D9 and this shouldn't be. We need a non legacy test.

Re #19: See the comment in EntityTestMulWithRevisionLog for the reasoning:

This entity type does not define revision_metadata_keys on purpose to test the BC layer.

In other words, all tests that use that entity type are "inherently" legacy, because they implicitly test that BC layer. That is very unfortunate, I absolutely agree. But it seems that was a conscious decision in that issue so it seems kind of out of scope of this issue to revert or change that decision.

Luckily, though, the test can actually be written to use entity_test_revlog (i.e. the non-translatable "sibling") to avoid this problem entirely. So here's an adapted patch including a test-only one to prove that it correctly fails like this, as well.

Very nice! #19 has been addressed wonderfully, so back to RTBC :)

Committed and pushed 78da9a1dca to 8.7.x and fcac0e25f8 to 8.6.x. Thanks!

Yay, one bug down.

Looks like querying by revision id field is still broken though: #2766135: EntityQuery with condition on the revision field leads to wrong results