Problem/Motivation

First, a quick tip of the hat to a certain legendary core developer, who shall here remain nameless, for discovering this problem.

Migrate includes an EntityRevision destination, which can migrate data into specific revisions of entities. Because EntityRevision inherits from the EntityContentBase destination, it also supports migrating data into specific translations of specific revisions of entities.

However, there is a critical hole in that functionality: the migrated translation data is keyed only by revision ID, and not by langcode. This can potentially result in data loss -- one language's data being overwritten with data intended for another language. Uh-oh!

It also means that it is impossible to look up the source identifier by destination revision ID and language, since the langcode was not part of the destination identifier.

Proposed resolution

EntityRevision needs to allow migrated data to be keyed by langcode in addition to revision ID.

Remaining tasks

  • Write a patch
  • Review it
  • Improve it
  • Commit it
  • Breathe a big fat sigh of relief

User interface changes

None.

API changes

Nothing user- or developer-facing.

Data model changes

Nothing user- or developer-facing.

CommentFileSizeAuthor
#86 2921661-86-8.6.x.patch6.89 KBquietone
#76 2921661-76.patch15.3 KBheddn
#73 interdiff_70-73.txt1.11 KBheddn
#73 2921661-73.patch19.42 KBheddn
#70 interdiff-2921661-66-70.txt1.08 KBmaxocub
#70 2921661-70.patch19.79 KBmaxocub
#66 2921661-66.patch21.7 KBalexpott
#63 2921661-63.patch21.39 KBalexpott
#63 61-63-interdiff.txt15.21 KBalexpott
#61 interdiff_59-61.txt786 bytesheddn
#61 2921661-61.patch17.01 KBheddn
#59 2921661-59.patch17.03 KBheddn
#57 interdiff_50-57.txt3.89 KBheddn
#57 2921661-57.patch82.92 KBheddn
#50 2921661-50.patch15.34 KBmaxocub
#50 interdiff-2921661-47-50.txt8.87 KBmaxocub
#47 interdiff-2921661-43-47.txt3.2 KBmaxocub
#47 2921661-47.patch8.34 KBmaxocub
#43 interdiff_40-43.txt2.69 KBheddn
#43 2921661-43.patch8.95 KBheddn
#40 interdiff_38-40.txt399 bytesheddn
#40 2921661-40.patch8.97 KBheddn
#38 interdiff_27-38.txt1.69 KBheddn
#38 2921661-38.patch9.36 KBheddn
#27 2921661-27.patch10.84 KBheddn
#27 interdiff_24-27.txt1.58 KBheddn
#24 interdiff-21-24.txt1.29 KBJo Fitzgerald
#24 2921661-24.patch10.78 KBJo Fitzgerald
#21 interdiff-17-21.txt2.03 KBbadmetevils
#21 2921661-21.patch10.27 KBbadmetevils
#17 interdiff_16-17.txt3.78 KBheddn
#17 2921661-17.patch12.02 KBheddn
#17 interdiff_11-16.txt2.33 KBheddn
#17 2921661-16.patch8.25 KBheddn
#11 interdiff_10-11.txt3.18 KBheddn
#11 2921661-11.patch7.76 KBheddn
#10 2921661-9.patch7.72 KBheddn
#10 interdiff_3-9.txt1.22 KBheddn
#3 2921661-3.patch7.09 KBphenaproxima
#3 2921661-3-FAIL.patch3.36 KBphenaproxima
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Priority: Normal » Critical

By committer agreement, Migrate criticals are now just plain critical. Sorry!

phenaproxima’s picture

Status: Active » Needs review
FileSize
3.36 KB
7.09 KB

Behold, a patch with a test, and a test-only patch to prove the problem. Boo-yah!

phenaproxima’s picture

Issue summary: View changes
heddn’s picture

Status: Needs review » Needs work
+++ 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!

phenaproxima’s picture

Issue tags: +Needs tests

I should clarify: data will not be overwritten in a normal, direct migration (like the kind in the test). It works more or less by accident, but it does work.

Here's what I believe will break it: trying to migrate some data into a previously-migrated revision translation. I believe that will load the right revision, but the wrong translation, and the revision's default language will end up getting data that was intended for one of its translations.

I'll add another test to confirm that case.

phenaproxima’s picture

Priority: Critical » Major
Issue tags: -Migrate critical

So I know that this can lead to potential data loss, and therefore I said it was critical. However, until I can prove the nature of the data loss (i.e., with a fail patch), I don't think I want to add more stuff to our collective plate. So I'm downgrading this for now. Once I have a fail patch that can prove the data loss, we can return this to critical.

xjm’s picture

Title: EntityRevision destination can't handle translations » One language's data may overwrite another's during migration
Priority: Major » Critical

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.

heddn’s picture

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

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.

heddn’s picture

FileSize
1.22 KB
7.72 KB

Arg, here's the patches.

heddn’s picture

FileSize
7.76 KB
3.18 KB

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

phenaproxima’s picture

Honestly...having had time to consider it, I think this patch looks good. It is thoroughly testing that revision translations are properly keyed by revision ID and language, which means there can be absolutely no ambiguity when doing data lookups from previous migrations (the ambiguity would be the source of the data loss, since the translation to load would not be specified, thus potentially overwriting data in the default translation).

I can't RTBC since I had a heavy hand in this one, but it has my blessing. I vote RTBC.

quietone’s picture

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.

maxocub’s picture

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.

phenaproxima’s picture

Status: Needs review » Needs work

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?

Looking at the code, the doc comment is wrong. updateEntity() never returns NULL, so we might as well change that now.

heddn’s picture

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.

heddn’s picture

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

heddn’s picture

Category: Bug report » Feature request
Status: Needs work » Needs review

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.

heddn’s picture

Title: One language's data may overwrite another's during migration » One language's Revision data may overwrite another's during migration
Priority: Critical » Major
Issue tags: +Needs reroll, +novice

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.

Jo Fitzgerald’s picture

Status: Needs review » Needs work
badmetevils’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
10.27 KB
2.03 KB

DId a Reroll of last uploaded patch

Status: Needs review » Needs work

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

heddn’s picture

Issue tags: -novice

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

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
10.78 KB
1.29 KB

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

heddn’s picture

quietone’s picture

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?

heddn’s picture

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.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Assigned: Unassigned » quietone

Assigning to myself for review

quietone’s picture

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.

quietone’s picture

Assigned: quietone » Unassigned
heddn’s picture

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.

quietone’s picture

Assigned: Unassigned » quietone

Assigning to myself for review

quietone’s picture

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()/

quietone’s picture

Assigned: quietone » Unassigned

Unassigning myself

maxocub’s picture

Status: Needs review » Needs work

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.

quietone’s picture

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

heddn’s picture

Status: Needs work » Needs review
FileSize
9.36 KB
1.69 KB

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

quietone’s picture

Status: Needs review » Needs work

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.

heddn’s picture

Status: Needs work » Needs review
FileSize
8.97 KB
399 bytes
quietone’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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?

heddn’s picture

Title: One language's Revision data may overwrite another's during migration » Add support to migrate multilingual revisions
Status: Needs work » Needs review
FileSize
8.95 KB
2.69 KB

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.

maxocub’s picture

Status: Needs review » Reviewed & tested by the community

Feedback addressed, back to RTBC.

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

heddn’s picture

Category: Feature request » Bug report

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

maxocub’s picture

Status: Needs work » Needs review
FileSize
8.34 KB
3.2 KB

Addressing #45 & #46.

quietone’s picture

Status: Needs review » Reviewed & tested by the community

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

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

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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?

maxocub’s picture

FileSize
8.87 KB
15.34 KB

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

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Feedback from #49 is addressed. Back to rtbc.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 50: 2921661-50.patch, failed testing. View results

heddn’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
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.

maxocub’s picture

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.

alexpott’s picture

@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?

heddn’s picture

Status: Needs work » Needs review
FileSize
82.92 KB
3.89 KB

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.

Status: Needs review » Needs work

The last submitted patch, 57: 2921661-57.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
17.03 KB

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

maxocub’s picture

Status: Needs review » Needs work
+++ 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.

heddn’s picture

Status: Needs work » Needs review
FileSize
17.01 KB
786 bytes
maxocub’s picture

Status: Needs review » Reviewed & tested by the community

All feedback are addressed, back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
15.21 KB
21.39 KB

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.

Status: Needs review » Needs work

The last submitted patch, 63: 2921661-63.patch, failed testing. View results

heddn’s picture

+++ 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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
21.7 KB

Fixing the patch.

heddn’s picture

  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.

heddn’s picture

Status: Needs review » Needs work

Back to NW for 67.2

maxocub’s picture

Issue tags: +blocker

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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
19.79 KB
1.08 KB

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.

Status: Needs review » Needs work

The last submitted patch, 70: 2921661-70.patch, failed testing. View results

heddn’s picture

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.

heddn’s picture

Status: Needs work » Needs review
FileSize
19.42 KB
1.11 KB

Here's what I've suggested.

maxocub’s picture

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

mikeryan’s picture

Status: Needs review » Needs work

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?

heddn’s picture

Status: Needs work » Needs review
FileSize
15.3 KB

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.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I tried to find things to complain about in this patch, but I couldn't. I'd say it's ready to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2921661-76.patch, failed testing. View results

heddn’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2921661-76.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Another unrelated test failure.

  • catch committed 9eb7988 on 8.6.x
    Issue #2921661 by heddn, maxocub, alexpott, phenaproxima, Jo Fitzgerald...

  • catch committed 769b731 on 8.5.x
    Issue #2921661 by heddn, maxocub, alexpott, phenaproxima, Jo Fitzgerald...
catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

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

mondrake’s picture

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

quietone’s picture

Version: 8.5.x-dev » 8.6.x-dev
Status: Fixed » Needs review
FileSize
6.89 KB

@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.

Status: Needs review » Needs work

The last submitted patch, 86: 2921661-86-8.6.x.patch, failed testing. View results

neclimdul’s picture

Confirmed, Bisected to this issue as well.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community

Confirming patch in #86 fixed the problem for me.

Thanks

EDIT: on 8.6.x.

webflo’s picture

quietone’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Thanks for the quick follow-up.

  • alexpott committed 369c0ff on 8.6.x
    Issue #2921661 follow-up by quietone: Add support to migrate...

  • alexpott committed dfbe868 on 8.5.x
    Issue #2921661 follow-up by quietone: Add support to migrate...
Dave Reid’s picture

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?

Gábor Hojtsy’s picture

Chris Gillis’s picture

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.

mnshantz’s picture

Confirming patch in #86 fixed the problem for me.

mnshantz’s picture

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

vermario’s picture

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

Status: Fixed » Closed (fixed)

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

oknate’s picture

This patch is included in Drupal 8.5.7.