Change record: https://www.drupal.org/node/2708973

If an entity query uses ->allRevisions() the base_table and data_table are not queried, only the revision_table and revision_data_table are. This means fields that are not revisionable are not able to be queried.

Take this hypothetical example:

$this->entityTypeManager->getStorage($entity_type_id)
  ->getQuery()
  ->allRevisions()
  ->condition('foo', 'bar')
  ->execute();

If I have added a field foo to all entity types, which is not revisionable, and I want to return all revisions of a given entity type where foo equals bar. This would throw an error such as:

Uncaught PHP Exception Drupal\\Core\\Entity\\Query\\QueryException: "'foo' not found" at core/lib/Drupal/Core/Entity/Query/Sql/Tables.php line 25

Why is this so important?

This is currently blocking Multiversion and Workbench Moderation from working together. Multiversion makes all content entities revisionable, but not all fields revisionable. It even adds non-revisionable fields to revisionable entities, because sometimes it makes sense, especially for "meta" kinds of data. For example in Multiversion the workspace an entity belongs to is a non-revisionable field, we don't want to allow different revisions of an entity to belong to different workspaces, this will cause all kinds of pain.

Workbench Moderation only really cares about revisions, it even allows you to create forward revisions, these are revisions newer than the current default revision. Therefore when looking for the newest revision Workbench Moderation uses an allRevisions() query. Multiversion hooks into this and adds a condition for the workspace field and which is when we get a Uncaught PHP Exception Drupal\\Core\\Entity\\Query\\QueryException: "'workspace' not found" issue.

I expect there will be many more cases where this will come up in the future.

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Status: Active » Needs review
FileSize
6.4 KB

Here is a simple test to show the issue.

In there test two entities are created, on these the user is changed and translations are added. It then queries for all revisions and gets both revisions for both entities. It then queries for all revisions where the non-revisionable field is set to bar, it should just return the two revisions for the first entity, but it throws an error:

 Drupal\KernelTests\Core\Entity\ContentEntityNonRevisionableFieldTest::testRevisionChanged
Drupal\Core\Entity\Query\QueryException: 'foo' not found

Status: Needs review » Needs work

The last submitted patch, 2: allrevisions_queries-2700261-2.patch, failed testing.

timmillwood’s picture

Adding a fix for the test uploaded in #2.
Also adding another test to test the same issue on non-translatable entities.

Status: Needs review » Needs work

The last submitted patch, 4: allrevisions_queries-2700261-4.patch, failed testing.

The last submitted patch, 4: allrevisions_queries-2700261-4.patch, failed testing.

The last submitted patch, 4: allrevisions_queries-2700261-4.patch, failed testing.

timmillwood’s picture

It works locally ;)

SIMPLETEST_DB=sqlite://localhost//tmp/test2.sqlite SIMPLETEST_BASE_URL=http://drupal1.dev ../vendor/bin/phpunit -v /home/timmillwood/Code/drupal1/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

Runtime:	PHP 7.0.5-2+deb.sury.org~wily+1 with Xdebug 2.4.0
Configuration:	/home/timmillwood/Code/drupal1/core/phpunit.xml.dist

..

Time: 2.45 seconds, Memory: 6.00Mb
timmillwood’s picture

Looks like the patch was missing an entity_test entity type.

dixon_’s picture

Status: Needs review » Needs work

Just a quick initial review below. I'll do a more detailed review later today hopefully.

  1. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevFoo.php
    @@ -0,0 +1,71 @@
    +class EntityTestMulRevFoo extends EntityTestMulRev {
    

    Lets rename this new test entity type to something more descriptive, like EntityTestMulRevNonRevField

  2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestRevFoo.php
    @@ -0,0 +1,70 @@
    +class EntityTestRevFoo extends EntityTestRev {
    

    Same here, let's use the more descriptive name as suggested above.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php
    @@ -0,0 +1,190 @@
    +    $this->assertEquals($user2->id(), $entity2->getOwner()->id(), 'User 2 found on entity 2');
    +
    +
    +    $entity->addTranslation('de');
    

    Double empty line

timmillwood’s picture

Status: Needs work » Needs review
FileSize
15.13 KB

Updated with changes from #10 and also simplified the implementation a little.

timmillwood’s picture

Version: 8.0.x-dev » 8.1.x-dev

Filing against 8.1.x as 8.0.x is now frozen, although it passes against 8.0.x, 8.1.x, and 8.2.x.

amateescu’s picture

Status: Needs review » Needs work

I could find only some small things to fix, except the first point which I think is pretty important, so marking NW for that.

  1. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevNonRevField.php
    @@ -0,0 +1,71 @@
    +class EntityTestMulRevNonRevField extends EntityTestMulRev {
    
    +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestRevNonRevField.php
    @@ -0,0 +1,70 @@
    +class EntityTestRevNonRevField extends EntityTestRev {
    

    Can't we add these fields to the existing EntityTestMulRev and EntityTestRev classes?

    We already have a lot of these entity test classes for which we (unnecessarily) create the db tables on every test method run and it would be a shame to add even more for just this test.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php
    @@ -0,0 +1,189 @@
    +use Drupal\entity_test\Entity\EntityTestMulChanged;
    +use Drupal\entity_test\Entity\EntityTestMulRevChanged;
    

    Doesn't seem to be used in this file.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php
    @@ -0,0 +1,189 @@
    +  public static $modules = ['language', 'user', 'system', 'field', 'text', 'filter', 'entity_test'];
    

    The parent test class already adds everything in this list, except 'language', so we should add only that module in this list.

  4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php
    @@ -0,0 +1,189 @@
    +   * The EntityTestMulRevNonRevField entity type storage.
    ...
    +   * The EntityTestRevNonRevField entity type storage.
    

    Based on point 1. above, these might need to be renamed to use the ..MulRev and ..Rev classes.

  5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php
    @@ -0,0 +1,189 @@
    +    // Enable an additional language.
    +    ConfigurableLanguage::createFromLangcode('de')->save();
    +    ConfigurableLanguage::createFromLangcode('fr')->save();
    

    Enable *two* additional languages? :)

  6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php
    @@ -0,0 +1,189 @@
    +   * Tests non-revisionable fields on revisionable (and translatable) entities.
    

    I don't think we need the parenthesis around 'and translatable', it's not like we're testing both scenarios in this method.

timmillwood’s picture

timmillwood’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: allrevisions_queries-2700261-14.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.4 KB

Fixing the serialization test I broke.

Status: Needs review » Needs work

The last submitted patch, 17: allrevisions_queries-2700261-17.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
12.17 KB

Further serialization issues fixed.

timmillwood’s picture

Re-rolling to make sure it still applies cleanly.

amateescu’s picture

Title: allRevisions queries ignore non-revisionable fields » allRevisions() entity queries ignore non-revisionable fields
Status: Needs review » Needs work

Ok, getting close :)

  1. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRev.php
    @@ -1,6 +1,8 @@
     namespace Drupal\entity_test\Entity;
    +use Drupal\Core\Entity\EntityTypeInterface;
    +use Drupal\Core\Field\BaseFieldDefinition;
    

    Needs an empty line before the use statements, but based on the comment below, both use statements will not be needed anymore.

  2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRev.php
    @@ -46,4 +48,20 @@
     class EntityTestMulRev extends EntityTestRev {
     
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    $fields = parent::baseFieldDefinitions($entity_type);
    +
    +    $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;
    +  }
    

    The 'non_rev_field' is already provided by the parent class (EntityTestRev) so we don't really need to add here as well, no?

  3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php
    @@ -0,0 +1,182 @@
    +/**
    + * @file
    + * Contains \Drupal\KernelTests\Core\Entity\ContentEntityNonRevisionableFieldTest.
    + */
    

    A recent change in the coding standards says that we should not provide a @file docblock anymore :)

timmillwood’s picture

Implemented changes in #21.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Nice, looks great now.

P.S.: An interdiff would have been helpful :)

timmillwood’s picture

Issue summary: View changes
timmillwood’s picture

Issue summary: View changes
alexpott’s picture

Issue tags: +rc target triage

Adding this to the rc target triage list so the committers can discuss before 8.1.0 is released.

effulgentsia’s picture

Re #26, I think this would be very good to get into 8.1, since it's a blocker for the contrib modules mentioned in the summary. However, I think it might be patch-safe, so could go into 8.1.1 instead of 8.1.0. Except that it changes db queries, which might introduce some side-effect on production sites (e.g., performance?), in which case much better to do during a minor update than a patch update. So I don't know. Opinions from folks on this issue?

xjm’s picture

Issue tags: -rc target triage +rc target

So @catch and I discussed this and agreed that specific database queries could be considered a (very) internal API, and therefore there is a slight risk of disruption in patch releases (e.g. for a module performing a query alter). (https://www.drupal.org/core/d8-bc-policy#schema says "Writing queries directly against core tables is not recommended and may result in code that breaks between minor versions", which sort of implies that such things won't break between patch releases, so I think we'd want to apply the same logic here.) So, we agreed to make this an RC target.

If it does not land in the next 12h, we should probably commit it for 8.1.1 anyway, create a CR, and mention it in the release notes, since the risk of disruption is theoretical and the bug is problematic.

catch’s picture

Yeah I don't think the theoretical risk of broken query alters is very likely. However I also don't feel at all confident that we have test coverage of such things, which has made me a bit reticent to commit the patch so far.

The theoretical risk of performance degradation doesn't make this any better for 8.1.0 than 8.1.1 - that's what the RC period was for to flush things like that out, but it puts me closer to committing it just after 8.1.0 is tagged.

timmillwood’s picture

Issue summary: View changes

Initial draft of change record added: https://www.drupal.org/node/2708973

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: allrevisions_entity-2700261-22.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
amateescu’s picture

Status: Needs review » Reviewed & tested by the community

It was rtbc before.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 442799a and pushed to 8.1.x and 8.2.x. Thanks!

I agree with @catch I think the query alter issues are theoretical. In fact I'm not sure any queries that were working will actually change - it's just that things that didn't work now do.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.