Issue #2700261 by timmillwood, amateescu: allRevisions() entity queries ignore...

Issue #2700261 by timmillwood, amateescu: allRevisions() entity queries ignore non-revisionable fields

core/lib/Drupal/Core/Entity/Query/Sql/Tables.php

@@ -81,9 +81,20 @@ public function addField($field, $type, $langcode) {
 
     $field_storage_definitions = $this->entityManager->getFieldStorageDefinitions($entity_type_id);
     for ($key = 0; $key <= $count; $key ++) {
-      // If there is revision support and only the current revision is being
-      // queried then use the revision id. Otherwise, the entity id will do.
-      if (($revision_key = $entity_type->getKey('revision')) && $all_revisions) {
+      // This can either be the name of an entity base field or a configurable
+      // field.
+      $specifier = $specifiers[$key];
+      if (isset($field_storage_definitions[$specifier])) {
+        $field_storage = $field_storage_definitions[$specifier];
+      }
+      else {
+        $field_storage = FALSE;
+      }
+
+      // If there is revision support, only the current revisions are being
+      // queried, and the field is revisionable then use the revision id.
+      // Otherwise, the entity id will do.
+      if (($revision_key = $entity_type->getKey('revision')) && $all_revisions && $field_storage && $field_storage->isRevisionable()) {
         // This contains the relevant SQL field to be used when joining entity
         // tables.
         $entity_id_field = $revision_key;
@@ -95,15 +106,6 @@ public function addField($field, $type, $langcode) {
         $entity_id_field = $entity_type->getKey('id');
         $field_id_field = 'entity_id';
       }
-      // This can either be the name of an entity base field or a configurable
-      // field.
-      $specifier = $specifiers[$key];
-      if (isset($field_storage_definitions[$specifier])) {
-        $field_storage = $field_storage_definitions[$specifier];
-      }
-      else {
-        $field_storage = FALSE;
-      }
 
       /** @var \Drupal\Core\Entity\Sql\DefaultTableMapping $table_mapping */
       $table_mapping = $this->entityManager->getStorage($entity_type_id)->getTableMapping();
@@ -152,11 +154,18 @@ public function addField($field, $type, $langcode) {
         // finds the property first. The data table is preferred, which is why
         // it gets added before the base table.
         $entity_tables = array();
-        if ($data_table = $all_revisions ? $entity_type->getRevisionDataTable() : $entity_type->getDataTable()) {
+        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();
+        }
+        if ($data_table) {
           $this->sqlQuery->addMetaData('simple_query', FALSE);
           $entity_tables[$data_table] = $this->getTableMapping($data_table, $entity_type_id);
         }
-        $entity_base_table = $all_revisions ? $entity_type->getRevisionTable() : $entity_type->getBaseTable();
         $entity_tables[$entity_base_table] = $this->getTableMapping($entity_base_table, $entity_type_id);
         $sql_column = $specifier;
 

core/modules/serialization/src/Tests/EntitySerializationTest.php

@@ -122,6 +122,7 @@ public function testNormalize() {
       'default_langcode' => array(
         array('value' => TRUE),
       ),
+      'non_rev_field' => array(),
       'field_test_text' => array(
         array(
           'value' => $this->values['field_test_text']['value'],
@@ -190,6 +191,7 @@ public function testSerialize() {
       'user_id' => '<user_id><target_id>' . $this->user->id() . '</target_id><target_type>' . $this->user->getEntityTypeId() . '</target_type><target_uuid>' . $this->user->uuid() . '</target_uuid><url>' . $this->user->url() . '</url></user_id>',
       'revision_id' => '<revision_id><value>' . $this->entity->getRevisionId() . '</value></revision_id>',
       'default_langcode' => '<default_langcode><value>1</value></default_langcode>',
+      'non_rev_field' => '<non_rev_field/>',
       'field_test_text' => '<field_test_text><value>' . $this->values['field_test_text']['value'] . '</value><format>' . $this->values['field_test_text']['format'] . '</format></field_test_text>',
     );
     // Sort it in the same order as normalised.

core/modules/system/tests/modules/entity_test/src/Entity/EntityTestRev.php

@@ -63,6 +63,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
     $fields['name']->setRevisionable(TRUE);
     $fields['user_id']->setRevisionable(TRUE);
 
+    $fields['non_rev_field'] = BaseFieldDefinition::create('string')
+      ->setLabel(t('Non Revisionable Field'))
+      ->setDescription(t('A non-revisionable test field.'))
+      ->setRevisionable(FALSE)
+      ->setCardinality(1)
+      ->setReadOnly(TRUE);
+
     return $fields;
   }
 

core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php

+<?php
+
+namespace Drupal\KernelTests\Core\Entity;
+
+use Drupal\entity_test\Entity\EntityTestMulRev;
+use Drupal\entity_test\Entity\EntityTestRev;
+use Drupal\language\Entity\ConfigurableLanguage;
+
+/**
+ * Tests non-revisionable fields on revisionable (and translatable) entities.
+ *
+ * @group Entity
+ */
+class ContentEntityNonRevisionableFieldTest extends EntityKernelTestBase {
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = ['language'];
+
+  /**
+   * The EntityTestMulRev entity type storage.
+   *
+   * @var \Drupal\Core\Entity\EntityStorageInterface
+   */
+  protected $mulRev;
+
+  /**
+   * The EntityTestRev entity type storage.
+   *
+   * @var \Drupal\Core\Entity\EntityStorageInterface
+   */
+  protected $rev;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    // Enable an additional language.
+    ConfigurableLanguage::createFromLangcode('de')->save();
+
+    $this->installEntitySchema('entity_test_mulrev');
+    $this->installEntitySchema('entity_test_rev');
+    $this->mulRev = $this->entityManager->getStorage('entity_test_mulrev');
+    $this->rev = $this->entityManager->getStorage('entity_test_rev');
+  }
+
+  /**
+   * Tests non-revisionable fields on revisionable and translatable entities.
+   */
+  public function testMulNonRevisionableField() {
+    $user1 = $this->createUser();
+    $user2 = $this->createUser();
+
+    // Create a test entity.
+    $entity = EntityTestMulRev::create(array(
+      'name' => $this->randomString(),
+      'user_id' => $user1->id(),
+      'language' => 'en',
+      'non_rev_field' => 'Huron',
+    ));
+    $entity->save();
+
+    // Create a test entity.
+    $entity2 = EntityTestMulRev::create(array(
+      'name' => $this->randomString(),
+      'user_id' => $user1->id(),
+      'language' => 'en',
+      'non_rev_field' => 'Michigan',
+    ));
+    $entity2->save();
+
+    $this->assertEquals('Huron', $entity->get('non_rev_field')->value, 'Huron found on entity 1');
+    $this->assertEquals('Michigan', $entity2->get('non_rev_field')->value, 'Michigan found on entity 2');
+
+    $entity->setNewRevision();
+    $entity->setOwner($user2);
+    $entity->save();
+    $entity2->setNewRevision();
+    $entity2->setOwner($user2);
+    $entity2->save();
+    $this->assertEquals($user2->id(), $entity->getOwner()->id(), 'User 2 found on entity 1');
+    $this->assertEquals($user2->id(), $entity2->getOwner()->id(), 'User 2 found on entity 2');
+
+    $entity->addTranslation('de');
+    $entity->save();
+    $entity2->addTranslation('de');
+    $entity2->save();
+
+    $expected_revision_ids = [
+      4 => 2,
+      3 => 1,
+      2 => 2,
+      1 => 1,
+    ];
+    $revision_ids = $this->mulRev->getQuery()
+      ->allRevisions()
+      ->sort('revision_id', 'DESC')
+      ->execute();
+    $this->assertEquals($expected_revision_ids, $revision_ids, 'Revision ids found');
+
+    $expected_non_rev_field_revision_ids = [
+      3 => 1,
+      1 => 1,
+    ];
+    $non_rev_field_revision_ids = $this->mulRev->getQuery()
+      ->allRevisions()
+      ->condition('non_rev_field', 'Huron')
+      ->sort('revision_id', 'DESC')
+      ->execute();
+    $this->assertEquals($expected_non_rev_field_revision_ids, $non_rev_field_revision_ids, 'Revision ids found');
+  }
+
+  /**
+   * Tests non-revisionable fields on revisionable entities.
+   */
+  public function testNonRevisionableField() {
+    $user1 = $this->createUser();
+    $user2 = $this->createUser();
+
+    // Create a test entity.
+    $entity = EntityTestRev::create(array(
+      'name' => $this->randomString(),
+      'user_id' => $user1->id(),
+      'non_rev_field' => 'Superior',
+    ));
+    $entity->save();
+
+    // Create a test entity.
+    $entity2 = EntityTestRev::create(array(
+      'name' => $this->randomString(),
+      'user_id' => $user1->id(),
+      'non_rev_field' => 'Ontario',
+    ));
+    $entity2->save();
+
+    $this->assertEquals('Superior', $entity->get('non_rev_field')->value, 'Superior found on entity 1');
+    $this->assertEquals('Ontario', $entity2->get('non_rev_field')->value, 'Ontario found on entity 2');
+
+    $entity->setNewRevision();
+    $entity->setOwner($user2);
+    $entity->save();
+    $entity2->setNewRevision();
+    $entity2->setOwner($user2);
+    $entity2->save();
+    $this->assertEquals($user2->id(), $entity->getOwner()->id(), 'User 2 found on entity 1');
+    $this->assertEquals($user2->id(), $entity2->getOwner()->id(), 'User 2 found on entity 2');
+
+    $expected_revision_ids = [
+      4 => 2,
+      3 => 1,
+      2 => 2,
+      1 => 1,
+    ];
+    $revision_ids = $this->rev->getQuery()
+      ->allRevisions()
+      ->sort('revision_id', 'DESC')
+      ->execute();
+    $this->assertEquals($expected_revision_ids, $revision_ids, 'Revision ids found');
+
+    $expected_non_rev_field_revision_ids = [
+      3 => 1,
+      1 => 1,
+    ];
+    $non_rev_field_revision_ids = $this->rev->getQuery()
+      ->allRevisions()
+      ->condition('non_rev_field', 'Superior')
+      ->sort('revision_id', 'DESC')
+      ->execute();
+    $this->assertEquals($expected_non_rev_field_revision_ids, $non_rev_field_revision_ids, 'Revision ids found');
+  }
+
+}