Problem/Motivation

A recent critical issue (#2766957: Forward revisions + translation UI can result in forked draft revisions) brought up the fact the the 'revision_translation_affected' field is basically required for any translatable and revisionable entity type.

Proposed resolution

Let's make the entity field manager provide this field by default.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
FileSize
1.79 KB

Here's a start. Now that I wrote the patch, I'm wondering whether this should actually be provided by default by the storage, like 'default_langcode'.

plach’s picture

Probably it should, but we should be able to detect whether a non-core entity type already defined it by itself, otherwise it could be a non-BC change.

plach’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Tests green, but this is obviously not ready :)

I guess we will need also an upgrade path for any non-core translatable and revisionable content type that does not implement the RTA flag. Actually we have content_moderation_state in core, but that's internal so not sure how much it matters.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
11.33 KB

Going deeper with this field would mean something like this. No interdiff because this is a new patch :)

Status: Needs review » Needs work

The last submitted patch, 5: 2896845-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

Status: Needs work » Needs review
FileSize
9.69 KB
5.47 KB

Damn it, making it an entity key automatically promotes it to 'not null'.

Status: Needs review » Needs work

The last submitted patch, 7: 2896845-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Sam152’s picture

I think it makes sense for this to be an entity key. FWIW, you can revert the not null handling like so:

diff --git a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
index c5751e1..105aaa0 100644
--- a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1694,6 +1694,7 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
     $not_null_keys = $this->entityType->getKeys();
     // Label fields are not necessarily required.
     unset($not_null_keys['label']);
+    unset($not_null_keys['revision_translation_affected']);
     // Because entity ID and revision ID are both serial fields in the base and
     // revision table respectively, the revision ID is not known yet, when
     // inserting data into the base table. Instead the revision ID in the base

pk188’s picture

Status: Needs work » Needs review
FileSize
10.53 KB

Updated according to #9.

Sam152’s picture

Status: Needs review » Needs work

#10 doesn't use entity keys which was what #9 was based on.

amateescu’s picture

@Sam152, I thought about doing that but I didn't want to increase the number of hacks in that code area :)

One other option would be to make it a revision metadata key but this flag is not really metadata, is it?

Sam152’s picture

Probably a better question for @plach, but it seems to fit the definition of "metadata" to me.

timmillwood’s picture

IIRC revision metadata can't be translatable.

plach’s picture

Honestly I don't remember if revision metadata can be translatable, but if it can RTA would qualify in my book :)

amateescu’s picture

I just checked and @timmillwood is right, revision metadata field are specifically excluded from being part of any data table, so they can't be translatable :/

amateescu’s picture

Status: Needs work » Needs review
FileSize
12.42 KB
1.09 KB

Ok, let's hack the entity keys some more then. It's better than hardcoding the field name everywhere..

Interdiff is to #5.

Status: Needs review » Needs work

The last submitted patch, 17: 2896845-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

plach’s picture

FileSize
106.89 KB

I manually tested #17 and, after running the update, this is what I get:

amateescu’s picture

Title: Add the 'revision_translation_affected' base field to EditorialContentEntityBase » Provide the 'revision_translation_affected' base field by default for all revisionable and translatable entity types
Status: Needs work » Needs review
FileSize
16.73 KB
4.95 KB

#19 was happening because the update function was using the cached entity type definitions which didn't include the new auto-populated key. Easy to fix though :)

amateescu’s picture

Discussed this issue with @plach on IRC and he suggested the improvements from the interdiff attached.

We also wrote a CR for this issue: https://www.drupal.org/node/2897586

tstoeckler’s picture

Awesome change, and great work in getting it done!

One comment on the latest interdiff:

+++ b/core/modules/system/system.install
@@ -2014,7 +2014,8 @@ function system_update_8402() {
+        ->setInitialValue(TRUE);

I think this deserves a comment. Is the reason for adding this as a sort of "safeguard" that we rather mark all revision translations as affected, rather than not mark some as affected that actually would be affected?

plach’s picture

You're right about the comment. It's about being consistent with the previous API return value: if the field was not defined the value returned was always TRUE.

plach’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.install
@@ -1984,3 +1985,40 @@ function system_update_8401() {
+  // Clear the cached entity type definitions so we get the new
+  // 'revision_translation_affected' entity key

Missing trailing dot :)

Setting to NW for #22.

amateescu’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
17.08 KB
1.02 KB

I just love copy-pasting @plach's comments :D

plach’s picture

Status: Needs review » Reviewed & tested by the community

#21 is green, and #25 contains only comment changes so we should be good here.

CR for this available at https://www.drupal.org/node/2897586

timmillwood’s picture

Great job @amateescu!

+1 to RTBC.

I think we need to remove commit credit for @pk188.

plach’s picture

I think we need to remove commit credit for @pk188.

Why? He did work on this issue, even if it was a small reroll.

timmillwood’s picture

Why? He did work on this issue, even if it was a small reroll.

The patch wasn't a reroll, it was implementing #9, regarding entity keys, when the patch wasn't using entity keys, as mentioned in #11.

Just trying to crack down in credit for incorrect patches, we had the same issue here too https://www.drupal.org/node/2856363#comment-12140284

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Just to make sure, since we don't have other revisionable/translatable entity types in core, the update will be a no-op.

But if #2880149: Convert taxonomy terms to be revisionable and publishable had landed, then it would run on taxonomy terms.

That makes it less risky to commit during alpha, since it will only affect contrib entity types or altered core ones, but do we need explicit test coverage for the update?

timmillwood’s picture

Unless I missed something, this patch will update the Media core entity type, which doesn't have a revision_translation_affected field.

Sam152’s picture

What happens if this gets in, the update runs, then an issue like #2880149: Convert taxonomy terms to be revisionable and publishable lands which changes an entity to be revisionable. Does that leave it with pending entity updates? At that point are they required to install the storage definition themselves, like in system_update_8402?

timmillwood’s picture

@Sam152 the update to SqlContentEntityStorageSchemaConverter in this patch covers that.

Sam152’s picture

Ah, totally missed that. Nice one.

plach’s picture

+++ b/core/modules/system/src/Tests/Update/EntityUpdateToRevisionableAndPublishableTest.php
@@ -77,6 +77,9 @@ protected function setDatabaseDumpFiles() {
+   *
+   * @covers entity_test_update_update_8400
+   * @covers system_update_8402
    */
   public function testConvertToRevisionableAndPublishable() {
     // Check that entity type is not revisionable nor publishable prior to
@@ -100,6 +103,11 @@ public function testConvertToRevisionableAndPublishable() {

@@ -100,6 +103,11 @@ public function testConvertToRevisionableAndPublishable() {
     $storage = \Drupal::entityTypeManager()->getStorage('entity_test_update');
     $this->assertEqual(count($storage->loadMultiple()), 102, 'All test entities were found.');
 
+    // Check that the 'revision_translation_affected' field has been added by
+    // system_update_8402().
+    $field_storage_definitions = $this->lastInstalledSchemaRepository->getLastInstalledFieldStorageDefinitions('entity_test_update');
+    $this->assertTrue(isset($field_storage_definitions['revision_translation_affected']));
+

@catch

Isn't this enough as explicit test coverage?

timmillwood’s picture

Version: 8.5.x-dev » 8.4.x-dev

As this is already RTBC it should be fine for 8.4.x

catch’s picture

Status: Reviewed & tested by the community » Needs review

@plach it's not confirming that the schema in the db is correct after the update or that the default value is set correctly. Can we load one of the entities and check?

timmillwood’s picture

I've been doing some testing this morning and it doesn't look as though system_update_8402 is run for any entity types. node and block_content are the only entity types which make it to the foreach, but they already have the RTA field, so don't get it installed.

When running \Drupal\system\Tests\Update\EntityUpdateToRevisionableAndPublishableTest none of the entity_test entity types make it into the update.

timmillwood’s picture

Status: Needs review » Needs work

It seems as though at the time of running system_update_8402 entity_test_update entity type doesn't have a revision entity key, so is seen as not revisionable, although in \Drupal\system\Tests\Update\EntityUpdateToRevisionableAndPublishableTest::testConvertToRevisionableAndPublishable it is seen as revisionable by the end of the updates. So it must not be getting the key added until after system_update_8402.

Think I'm a little out of my depth, and @amateescu is out this week, but I'll check in with @plach once he's online.

amateescu’s picture

Assigned: Unassigned » amateescu

The analysis in #40 is correct, and the reason we can not properly test this in EntityUpdateToRevisionableAndPublishableTest is because we need an entity type that is revisionable and translatable before running system_update_8402(), but converting an entity type to revisionable uses a post_update hook which cannot run before an update hook.

Working on it.

amateescu’s picture

Assigned: amateescu » Unassigned
Status: Needs work » Needs review
FileSize
208.08 KB
21.79 KB
8.56 KB

There's no other way around it, we need to add a new test db dump which contains entity_test_update entities that are already translatable and revisionable and write a separate test case for it.

Since the full patch contains a test db dump now, I'm also attaching a "for review" patch :)

plach’s picture

Status: Needs review » Needs work

Thanks @amateescu, looks good to me! I found only a few minor issues.

  1. +++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
    @@ -2,6 +2,8 @@
    +use Drupal\Core\Field\BaseFieldDefinition;
    

    Unrelated change?

  2. +++ b/core/modules/system/src/Tests/Update/EntityUpdateAddRevisionTranslationAffected.php
    @@ -0,0 +1,88 @@
    +class EntityUpdateAddRevisionTranslationAffected extends UpdatePathTestBase {
    

    Missing Test suffix in the class name.

  3. +++ b/core/modules/system/src/Tests/Update/EntityUpdateAddRevisionTranslationAffected.php
    @@ -0,0 +1,88 @@
    +  /**
    +   * The entity manager service.
    +   *
    +   * @var \Drupal\Core\Entity\EntityManagerInterface
    +   */
    +  protected $entityManager;
    

    Can we use the entity type manager here?

  4. +++ b/core/modules/system/src/Tests/Update/EntityUpdateAddRevisionTranslationAffected.php
    @@ -0,0 +1,88 @@
    +    $entity = \Drupal::entityTypeManager()->getStorage('entity_test_update')->load(1);
    

    Can we use the entity type manager stored in the class property here?

timmillwood’s picture

@amateescu - Does this mean that if we make an entity type revisionable and publishable (such as comments or taxonomy terms) they will get NULL revision_translation_affected values because they won't have ->setInitialValue(TRUE);?

amateescu’s picture

Status: Needs work » Needs review
FileSize
207.63 KB
1.74 KB

@plach, good catches :) Fixed #43.1, 2, and 4. For 3) we can't use the entity type manager because \Drupal\system\Tests\Entity\EntityDefinitionTestTrait::updateEntityTypeToRevisionableAndTranslatable uses the old entity manager :/

@timmillwood, nope, we're explicitly setting revision_translation_affected to TRUE in SqlContentEntityStorageSchemaConverter::copyData() and test it in SqlContentEntityStorageSchemaConverterTest. See the first two parts of the interdiff from #42.

timmillwood’s picture

Awesome, thanks for the clarification @amateescu!

+1 for RTBC from me then, but I'll let @plach take another look.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.4.0 release notes

Thanks for the additional test coverage. Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Also tagged for a release notes mention.

  • catch committed 88d1f67 on 8.5.x
    Issue #2896845 by amateescu, pk188, plach, timmillwood, Sam152: Provide...
jibran’s picture

Status: Reviewed & tested by the community » Fixed

Published the change notice.

xjm’s picture

Issue tags: +8.4.0 release notes

This also sounds maybe worth mentioning in the release notes?

Edit: Looks like catch also tagged it; not sure how or why the tag didn't remain on the issue.

Sam152’s picture

I suspect #49 and #48 were some kind of race condition. The "Fixed" didn't stick around either.