The getEntity Method of \Drupal\entity_reference_revisions\Plugin\migrate\destination\EntityReferenceRevisions calls $this->updateEntity() after creating a new (possibly stub) row. If the row is a stub from the migrate_lookup plugin, then the bundle key is set in the row's emptyDestinationProperties array, and subsequently deleted from the entity, causing an error on save. updateEntity() should only be called on existing entities.

In addition, the method does not allow for creation of new revisions of an existing entity during a migration. It either updates an existing revision or creates a whole new entity.

What Should Happen:

If a reference_id is provided, it should load an update that revision. If it isn't and an id is provided, then it should load that entity, update it, and create a new revision. If neither are provided, it should create a new entity, process a possible stub, and return.

CommentFileSizeAuthor
#13 2944836-13.entity_reference_revisions.Migrate-destination-plugin-attempts-to-update-new-entities-preventing-creation-of-stub-entities.patch15.25 KBmikelutz
#13 interdiff.2944836.11-13.txt5.92 KBmikelutz
#11 2944836-11.patch13.43 KBheddn
#11 interdiff_8-11.txt764 bytesheddn
#8 2944836-8.patch13.43 KBheddn
#8 interdiff_4-8.txt5.07 KBheddn
#5 2944836-4_tests_only.patch7.45 KBheddn
#4 interdiff.2944836.2-4.txt9.25 KBmikelutz
#4 2944836-4.entity_reference_revisions.Migrate-destination-plugin-attempts-to-update-new-entities-preventing-creation-of-stub-entities.patch10.32 KBmikelutz
#2 2944836-2.entity_reference_revisions.Migrate-destination-plugin-attempts-to-update-new-entities-preventing-creation-of-stub-entities.patch2.28 KBmikelutz

Comments

mikelutz created an issue. See original summary.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Active » Needs review
StatusFileSize
new 2944836-2.entity_reference_revisions.Migrate-destination-plugin-attempts-to-update-new-entities-preventing-creation-of-stub-entities.patch2.28 KB

Here is a fix. The current class doesn't appear to have test coverage, and I don't have time to create one from scratch right now, but I will try to do so in the next days.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Needs work because needs tests.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
StatusFileSize
new 2944836-4.entity_reference_revisions.Migrate-destination-plugin-attempts-to-update-new-entities-preventing-creation-of-stub-entities.patch10.32 KB
new interdiff.2944836.2-4.txt9.25 KB

I found the existing test class, and updated the data set to Check for what I would expect the behavior of the getEntity() function should be.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs work » Needs review
StatusFileSize
new 2944836-4_tests_only.patch7.45 KB

I'd like to see the failures.

Status: Needs review » Needs work

The last submitted patch, 5: 2944836-4_tests_only.patch, failed testing. View results

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new interdiff_4-8.txt5.07 KB
new 2944836-8.patch13.43 KB

The default for migrating entities when passing only the entity_id without the revision_id is to update the default record. That's encoded into the most base of Entity destination classes for quite some time. However, I see the need/desire to override this feature. So I've added some tests and provided this as a configurable option. Do you think it will make folks happy to have both methods?

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Issue tags: +Needs change record

Do we do change records for ERR? Flagging. But I think this is good for final reviews.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

This looks good, and will definitely work to solve my problem over in paragraphs, and minimizes the changes to anybody using the code now. One nitty suggestion:

       $entity->setNewRevision(FALSE);
+      $entity = $this->updateEntity($entity, $row) ?: $entity;
     }
+    // If there is no revision supplied, but there is an entity_id
+    // supplied that exists, update it.
+    elseif (!empty($item_id) && ($entity = $this->storage->load($item_id))) {
+      // If so configured, create a new revision while updating.
+      if ($this->getConfiguration()['new_revisions']) {
+        $entity->setNewRevision(TRUE);
+      }
+      $entity = $this->updateEntity($entity, $row) ?: $entity;
+    }

$entity->setNewRevision defaults to false on entity load, so $entity->setNewRevision(FALSE) is unnecessary, but I like it because it makes the code clearer. For consistancy, either explicitly set $entity->setNewRevision(FALSE) if the config isn't triggered, or remove the unneeded one above it.

Otherwise, +1 for RTBC

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Priority: Normal » Major
Issue tags: -Needs change record
StatusFileSize
new interdiff_8-11.txt764 bytes
new 2944836-11.patch13.43 KB

re #11: I've removed it above. In thinking about it, this is a strait up bug fix. If someone had experienced the problem, they would know about it. So removing the CR tag.

Also bumping to Major, since it blocks stability for field collection and paragraph migration efforts.

quietone’s picture

quietone commented
Status: Needs review » Needs work

I have read the issue and checked that patch with the code inspection of PhpStorm. I'm not very familiar with entity reference revisions nor the use of ConfigurablePluginInterface but it all looks good to me. Thanks for helping me to learn more about those.

I found a few things, mostly to help with readability.

  1. +++ b/tests/src/Kernel/Plugin/migrate/destination/EntityReferenceRevisionsDestinationTest.php
@@ -75,7 +73,7 @@ public function testGetEntityTypeId(array $definition, $expected) {
+    $datas = $this->getEntityDataProvider();
 
     foreach ($datas as &$data) {

    Whoa, data is the plural, datum is the singular. It is the foreach that should change to foreach ($data as $datum). But I see there are three total instances of 'datas' in this file, so it is the norm here. Because of that no need to fix it in this patch. Maybe a novice followup to fix those? 

  2. +++ b/src/Plugin/migrate/destination/EntityReferenceRevisions.php
@@ -70,12 +112,33 @@ public function getIds() {
+      if (!empty($item_id) && $entity->id() != $item_id) {

    Can we add another set of parenthesis to be extra clear about the order of operations? The similar statement in the elseif following has the extra set and is easier to read. 

  3. +++ b/src/Plugin/migrate/destination/EntityReferenceRevisions.php
@@ -70,12 +112,33 @@ public function getIds() {
+    $item_id = $oldDestinationIdValues ?
...
     if (!empty($revision_id) && ($entity = $this->storage->loadRevision($revision_id))) {

    Can this just be entity_id? It would make the large if block that follows a lot easier to read.

  4. +++ b/src/Plugin/migrate/destination/EntityReferenceRevisions.php
@@ -70,12 +112,33 @@ public function getIds() {
+    // If a specific revision_id is supplied and exists, assert the entity id
...
+    // If there is no revision supplied, but there is an entity_id

    As well as then be in agreement with the comments that refer to $item_id as the entity_id.

Yes, lets unblock the field collection and paragraph migrations!

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.2944836.11-13.txt5.92 KB
new 2944836-13.entity_reference_revisions.Migrate-destination-plugin-attempts-to-update-new-entities-preventing-creation-of-stub-entities.patch15.25 KB

Funny, I thought the same thing about the 'datas' when I saw them, but I was trying to avoid changing things beyond the issue. I didn't actually add them, but there was a codesniffer issue with the line that I went ahead and fixed in the original patch. Anyway, easy enough to change them out right here, along with the other recommendations. As I said before, it always makes me happy when a patch works well enough that we can sit and bikeshed over variable names.

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

@mikelutz, thanks, that is so much easier to read now.

I reckon this is good to go.

  • miro_dietiker committed 22ebdcc on 8.x-1.x authored by mikelutz 
    Issue #2944836 by mikelutz, heddn, quietone: Migrate destination plugin...
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Ha, this whole $data / $datum thing made me struggle and i spent a few seconds:
https://www.theguardian.com/news/datablog/2010/jul/16/data-plural-singular

It makes me feel like we're coding in latin now?
Never seen this term used this way in english...

Still committed as-is, looked great otherwise.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

Ha! I won't post an opinion on data are vs data is, but unless you are referring to more that one of the Star Trek : The Next Generation character, datas is definitely wrong, lol.

Status: Fixed » Closed (fixed)

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