Add support to migrate multilingual revisions

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/EntityRevisionTest.php
@@ -0,0 +1,118 @@
+    $this->assertNotEmpty($source_ids);
+    $this->assertSame($node->id(), $source_ids['nid']);
+    $this->assertSame($node->getRevisionId(), $source_ids['vid']);
+    $this->assertSame('fr', $source_ids['langcode']);

I like all these asserts. Can we have a few more to assert we can load and have all the history of revisions for all the title changes? That will help us know if we overwrote anything.

Default 1
French 1
French 2
Titre nouveau, tabarnak!

Unfortunately data loss is pretty much always critical regardless of what subsystem it's in or which plates it's on, so reupgrading. This is one of the most clear-cut points in https://www.drupal.org/core/issue-priority#critical-bug. We wouldn't downgrade this unless there was some mitigation that made this circumstance impossible under normal migration operation.

We also prefer to leave issues at critical until we've verified that they've not so that they don't get lost. The committers will probably discuss this more the next time we do a triage.

Retitling to describe the impact of the bug; let me know if that title's accurate. Thanks! Also thanks for the clear issue summary.

I've added a test for what I asked for in #5. While we aren't perhaps able to reproduce what originally caused the issue for @phenaproxima in the OP, we can confirm that there is a bug with the tests.

Arg, here's the patches.

Here's some minor improvements(?) based on a self-review of the patch. Mainly some naming changes.

Looks really nice, just a one question.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
   @@ -70,7 +70,9 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +    $entity = $this->updateEntity($entity, $row) ?: $entity;
   

   Why the change? If this is restored to the original the new test still passes. The doc for updateEntity says it will return NULL if the entity is the same as input, but I don't see that. What have I missed here?

2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/EntityRevisionTest.php
   @@ -0,0 +1,129 @@
   +class EntityRevisionTest extends MigrateTestBase {
   

   Needs a short description in the doc block.

3. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/EntityRevisionTest.php
   @@ -0,0 +1,129 @@
   +    // Confirm the french revision was used in the migration, instead of the
   +    // default revision.
   

   Like the extra tests. Makes it extra clear what is being tested.

I think this looks good, except for what @quietone mentioned above. And I like the extra test too, it clearly show that the translation was updated, and not the default language.

So, this fixes some things. Mostly comments. Bumping back to NW though to provide an upgrade path. We need to loop through all migrations that use EntityRevision as the destination. And alter the sql table schema so it includes the langcode column. For this update, we should load the revision, get its default langcode, and set the langcode that.

OK, here's the patches from 16 and the new ones for 17.

So, an interesting thought to consider as I was reviewing the code in the last patch this morning. Core doesn't have any migrations that fill the criteria of needing the update hook in this patch. Why? Because the fix associated with this issue is only for revisions that also support translations. There are none of those in core right now. Which makes it really hard to test the update hook.

Based on that information, I'm going to re-categorize this as a feature request. We cannot have a bug for something that we don't currently support. I'd also argue this isn't critical either. But this has already been bumped back and forth from a critical once, so I'll leave that to someone else to decide.

The following is the line of code in the update hook that made me come to this conclusion. The key is isTranslationDestination()

if ($destination_plugin instanceof EntityRevision && $migration->getIdMap() instanceof Sql && $migration instanceof EntityContentBase && $migration->isTranslationDestination()) {

We only need this functionality if we are dealing with an EntityRevision destination that is a translation destination.

See #2746541: Migrate D6 and D7 node revision translations to D8 for the feature where we are still working on building out this feature. Based on the fact it is only major, I'm going to drop priority on this issue.

And I'm also going to post a new patch here after my next meeting that removes the update hook. No need for it.

Strike that. I'm going to tag as novice and have someone else re-roll the patch without those changes. Just remove all the hook_update code and re-post a patch.

DId a Reroll of last uploaded patch

Looks like a legit error. Not sure why, since the patch was working earlier.

Got the test version of EntityRevision::updateEntity() to return a sensible value.

Brief review, just looking at comments. Leaving at NR to allow others to feedback.

1. +++ b/core/modules/migrate/migrate.install
   @@ -16,3 +18,40 @@ function migrate_update_8001() {
   + * Copy of Sql::getFieldSchema() for use by updates.
   

   What? Admittedly, not familiar with this hook but the summary line isn't helping either.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
   @@ -70,7 +70,9 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +    // We need to update the translated entity, so that the destination row IDs
   

   Do we know if this is a translated entity here? Or updateEntity called to make sure translations are updated, if it has translations?

26.2: fixed comment.
26.1: That function in Sql is protected, so we don't have access to it. I've added some comments to link to it better.

Assigning to myself for review

I just have a question about the comment in #18

Core doesn't have any migrations that fill the criteria of needing the update hook in this patch. Why? Because the fix associated with this issue is only for revisions that also support translations. There are none of those in core right now. Which makes it really hard to test the update hook.

Not sure if this means there is more work to do here or in a followup.

Re #30: there is no more work. Either here or in a follow-up. No update is needed. Because there are no core migrations to update.

Assigning to myself for review

Reread the issue, reviewed the code (just one small item) and ran the tests locally. It all looks great but I can't RTBC because I don't know much about .install and how the new function _migrate_get_field_schema fits into the picture.

One nit

+++ b/core/modules/migrate/migrate.install
@@ -16,3 +18,42 @@ function migrate_update_8001() {
+ *   MigrateSourceInterface or MigrateDestinationInterface::getIds().

s/MigrateSourceInterface/MigrateSourceInterface::getIds()/

Unassigning myself

The _migrate_get_field_schema() function should be removed. It was used in the hook_update() that was removed in #21, so it is not used anywhere anymore.

Other than that, I like the patch. Having tried to work on #2746541: Migrate D6 and D7 node revision translations to D8 in the past, I can see now why it didn't work.

@maxocub, thx! Now it makes sense, I just missed looking at the removed hook_update code.

Well, that's an easy fix. Less code now.

One nit and good to go.

+++ b/core/modules/migrate/migrate.install
@@ -5,6 +5,8 @@
+use Drupal\Core\Field\BaseFieldDefinition;
+

Not used, can be deleted.

Thanks heddn.

I think this is now good to go. I know I've gone over it many times, phenaproxima did a self review and supports RTBC (#12). Although that was a while ago the changes since then added a hook update and that code that was then removed. My misunderstanding about the function in .install was cleared up my maxocub in #36. And we have good tests as well.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -181,20 +181,42 @@ public function isTranslationDestination() {
+      $this->addDestinationId($ids, 'langcode', new MigrateException('This entity type does not support translation.'));

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
@@ -177,10 +179,14 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
+    $this->addDestinationId($ids, 'revision', new MigrateException('This entity type does not support revisions.'));
...
+      $this->addDestinationId($ids, 'langcode', new MigrateException('This entity type does not support translation.'));

Why not just pass in the error message and create and throw the exception in addDestinationId(). All the exceptions passed in are the same class. And if we need support different exception types then lets pass in the class name rather than creating an object.

Also should this be back to critical bug?

I feel (not very strongly) this should stay as a major and a feature. This is a very edge case and is currently Data loss is the reason for critical. This isn't an upgrade situation. This is a pure migrate issue at this point as is only an API enhancement, not a bug.

That's why we even removed the update hooks in early versions of this issue. We just don't currently support what we are doing here. See #18

I've also re-titled things. I'm not sure if we need a CR here because again, this is something we didn't previously even support. It is a new capability.

The reason this is a D8 only issue is because: Only in D8 did we start supporting multiple languages for the same entity id. In Drupal 7 and before, each language was its own node id or term id with a related language code.

New patch attached to address the feedback in #42.

Feedback addressed, back to RTBC.

I also agree that this should stay Major since there's no support for migrating multilingual revisions yet.

Re featurey-ness and majory-ness doesn't the fact we now have current Drupal sources impact this issue and make it a critical bug?

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -181,20 +181,42 @@ public function isTranslationDestination() {
+  protected function addDestinationId(array &$ids, $key, $error_message = NULL) {

Doing some more thinking about this. Given this is a base class that people extend and migrate is now stable I'm not 100% about adding protected methods to it without really good reason. We don't have to do this. We can just add the langcode key in in EntityRevision::getIds() the same way the revision ID is added in HEAD. I think we should be more cautious about breaking things now we are stable.

Let's remove the addDestinationId protected method and add that logic inline.

Addressing #45 & #46.

Issues raised in #42 and #45 have been addressed, this can return to RTBC.

Should there be a follow up to add addDestinationId() later?

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -181,12 +181,15 @@ public function isTranslationDestination() {
+        throw new MigrateException('This entity type does not support translations.');

We should have the entity type available here, so could use that in the exception message?

I added the entity type to the exception messages and I added a test for the EntityRevision exception messages because there were none.

Feedback from #49 is addressed. Back to rtbc.

Unrelated.

diff --git a/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
index 3e18d28b1a..3dd2161180 100644
--- a/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -206,9 +206,10 @@ public function getIds() {
    *   The row object to update from.
    *
    * @return \Drupal\Core\Entity\EntityInterface
-   *   An updated entity from row values.
+   *   An updated entity from row values. Note that the entity passed in will
+   *   also be updated.
    */
-  protected function updateEntity(EntityInterface $entity, Row $row) {
+  protected function updateEntity(EntityInterface &$entity, Row $row) {
     $empty_destinations = $row->getEmptyDestinationProperties();
     // By default, an update will be preserved.
     $rollback_action = MigrateIdMapInterface::ROLLBACK_PRESERVE;
diff --git a/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
index ff3f0071c7..fb7f0dcb5a 100644
--- a/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
@@ -160,9 +160,7 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
       $entity->enforceIsNew(FALSE);
       $entity->setNewRevision(TRUE);
     }
-    // We need to update the entity, so that the destination row IDs are
-    // correct.
-    $entity = $this->updateEntity($entity, $row);
+    $this->updateEntity($entity, $row);
     $entity->isDefaultRevision(FALSE);
     return $entity;
   }

I think given that in non-translatable circumstances the entity passed in is also updated we should continue to do that and rely on the side effects that we already rely on. To do this all we have to do is pass the object in by reference.

Won't that break BC? In core we have the Book module that extends EntityContentBase and overrides updateEntity(). I suppose there's might be some contrib modules that also do that.

@maxocub looking at the book override...

  /**
   * {@inheritdoc}
   */
  protected function updateEntity(EntityInterface $entity, Row $row) {
    $entity->book = $row->getDestinationProperty('book');
  }

It's already relying on the side effect. I think we should update them to use the pass by reference (atm we're passing by pointer) and actually isn't the book one quite broken for multi-lingual revisioned books?

I'm not aware of contrib overriding. The only contrib project I thought might was ERR, but they just call it. Since the method is not on an interface and is protected, I think that means its internal implementation details. But still, I don't think anyone is overriding it anyway.

From my limited research though, this will only result in a Warning, not an error. So, let's try it and see what happens.

Interdiff in #57 is correct. Patch was wrong. Here's what should have been uploaded.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
@@ -170,7 +170,7 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
+      $this->updateEntity($entity, $row) ?: $entity;

We can remove " ?: $entity" since updateEntity() will always return the entity.

All feedback are addressed, back to RTBC.

I've done a bit more research and discovered that we also have \Drupal\migrate\Plugin\migrate\destination\EntityContentBase::updateEntity() which also has quite a few overrides. The config version always works via the side effect. I think we should only work that way in Drupal 9. I've added a deprecation and CR to cover this change - https://www.drupal.org/node/2960492.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -197,15 +200,18 @@ public function getIds() {
+   *   will be removed in Drupal 9. See tbd.

CR here.

Also, we need test coverage of the BC layer.

Fixing the patch.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -170,7 +170,11 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +      $result = $this->updateEntity($entity, $row);
   +      if ($result !== NULL) {
   +        trigger_error(__CLASS__ . '::updateEntity() should not return an entity. See https://www.drupal.org/project/drupal/issues/2921661');
   +        $entity = $result;
   +      }
   

   This will not catch cases were things like ERR overrides getEntity. Its pretty common for contrib to override getEntity. However, hopefully this communicate to enough folks. And we aren't breaking BC, so this is OK.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -248,9 +256,6 @@ protected function updateEntity(EntityInterface $entity, Row $row) {
   -    // We might have a different (translated) entity, so return it.
   -    return $entity;
   

   This will break BC if someone overrides getEntity. They will not get the object returned.

Back to NW for 67.2

I'm not sure what we should do here. To return or not to return $entity?

I also want to remind that this is blocking #2746541: Migrate D6 and D7 node revision translations to D8 which is almost ready to migrate revisions of node translations.

Looking at this again, I see that the changes we made to the return doc of updateEntity() says that it is deprecated and will be removed in Drupal 9. So we should not break BC and remove it, not before Drupal 9.

I'm not sure what to do here. I see what we are trying to do. We're are attempting to warn folks to not return values from updateEntity. But that call is for protected code. And most people do not override updateEntity. They override getEntity and call updateEntity. Most people, meaning not very many people and the only case I've seen where folks do any overrides is ERR and they override getEntity. But not updateEntity. So I'm of the opinion we break BC and don't return the value. And then update the docs on updateEntity. Most folks are already assuming its value is changed internal to updateEntity, so we'd probably be OK. And again, this is internal API stuff.

Here's what I've suggested.

I agree with #73, but can't RTBC.

Just to walk through my thought process here (and clarify for anyone else coming in and wondering about this change), I'm not thrilled with passing objects by reference - in general, it's a surprise to have the actual object reference rather than just the object contents altered. But, in this case to handle translations we do need to potentially swap the parent entity object for the appropriate translation. We had been doing this by returning the (potentially new) entity and assigning it in the caller, but changing the argument to by reference makes this explicit. So, OK.

1. +++ b/core/modules/book/src/Plugin/migrate/destination/Book.php
   @@ -24,7 +24,7 @@ protected static function getEntityTypeId($plugin_id) {
   +  protected function updateEntity(EntityInterface &$entity, Row $row) {
        $entity->book = $row->getDestinationProperty('book');
      }
   

   Out of scope, but - how does this work without calling parent::updateEntity()?

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -186,12 +186,14 @@ public function getIds() {
      /**
       * Updates an entity with the contents of a row.
       *
   +   * Note that the entity passed in is updated.
   +   *
   

   The original comment here seems vague and incomplete (I admit, there's probably at least a 50% chance it's mine:P). And the additional comment doesn't really help - although I understand it means to point out that the entity object itself (rather than just its contents) may be altered, it actually just ends up rephrasing the original comment from "updates an entity" to "the entity is updated".

   This seems like a good place to point out that the abstract Entity destination class getEntity() calls updateEntity(), but the expected function signature is not documented in Entity. I think we should have abstract protected function updateEntity() declared in Entity (with a better comment) and EntityConfigBase and EntityContentBase can @inheritdoc.

3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -186,12 +186,14 @@ public function getIds() {
       * @param \Drupal\Core\Entity\EntityInterface $entity
       *   The entity to update.
   

   This seems the place to point out the argument is by reference.

4. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -21,7 +21,7 @@ class EntitySearchPage extends EntityConfigBase {
   +  protected function updateEntity(EntityInterface &$entity, Row $row) {
        $entity->setPlugin($row->getDestinationProperty('plugin'));
        $entity->getPlugin()->setConfiguration($row->getDestinationProperty('configuration'));
      }
   

   Similar (out-of-scope) digression as for Book above - shouldn't this call the parent?

So, let's back-up to the patch in #50. Before we did all the pass by reference stuff.

Here's a verbatim re-roll of that patch. No interdiff, because this is a re-roll.

Unrelated failure.

Committed/pushed to 8.6.x and cherry-picked to 8.5.x, thanks!

I think this is causing trouble for testing in non DrupalCI environments when running test directly via phpunit.

I started getting this which I was not getting before:

$ $DRUPAL_PATH/vendor/bin/phpunit --group Database

PHP Fatal error:  Cannot declare class Drupal\Tests\migrate\Unit\Plugin\migrate\destination\BaseFieldDefinitionTest, because the name is already in use in /home/travis/drupal8/core/modules/migrate/tests/src/Unit/Plugin/migrate/destination/EntityRevisionTest.php on line 162

See for example https://travis-ci.org/mondrake/drudbal/jobs/399699209

@mondrake, thanks for reporting this.

This patch should fix the problem for 8.6.x. I haven't looked at 8.5 and I don't have time now.

Confirmed, Bisected to this issue as well.

Confirming patch in #86 fixed the problem for me.

Thanks

EDIT: on 8.6.x.

drupal-project started to fail too. https://travis-ci.org/drupal-composer/drupal-project/builds/399953427

OK. good, this works for 8.5.x as well, considering the files it is touching, that makes sense.

Committed and pushed 369c0ffd6a to 8.6.x and dfbe8684c1 to 8.5.x. Thanks!

Thanks for the quick follow-up.

Is there a plan to get an updated core release since the latest 8.5.5 tag is unable to run tests due to the fatal error?

Published https://drupal.org/node/2960492

Is there any way to rush through an 8.5.6 release? Killing all the unit tests also kills automated build processes, and seems like a pretty CRITICAL bug. I've added a hard limit to my system to prevent upgrading Drupal for now, but would ideally like to remove that.

Confirming patch in #86 fixed the problem for me.

Confirming patch in #86 fixed the problem for me on 8.5.x.

Just wanted to report that the patch is still necessary on 8.5.6.

This patch is included in Drupal 8.5.7.