allRevisions() entity queries ignore non-revisionable fields

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

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.

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 :)

Nice, looks great now.

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

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

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?

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.

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.

It was rtbc before.

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.