EntityReference migrate destination

There's some core changes that need to happen to make this work. The values, when they are retrieved are numeric indexed. This comes from the SQL lookup. You might open a core issue too

Here's a problem - what if we're migrating referenced values that don't have a possible revision (ie: generated on previous migration) Is that what this would handle? Returning the migrated destination ID and revision ID?

Re #4: #2644634-11: Missing Migration Destination in 8.*

The linked core issue above makes life easier, but it isn't strictly necessary. Just cumbersome.

Here's some initial thoughts.

Enough has moved around that the interdiff isn't that useful, so I haven't included one. It was bigger than the actual patch.

1. +++ b/tests/modules/err_migration_test/src/Plugin/migrate/source/DummySource.php
   @@ -0,0 +1,45 @@
   +  protected function initializeIterator() {
   +    return [
   +      ['id' => 1, 'revision_id' => 1, 'name' => 'content item 1'],
   

   This is the initial stubs of data needed for getEntity(). We still need to actually migrate and create an entity to test getEntity

2. +++ b/tests/src/Kernel/Plugin/migrate/destination/EntityReferenceRevisionsDestinationTest.php
   @@ -0,0 +1,45 @@
   +   * Tests deriver
   +   */
   +  public function testDestinationDeriver() {
   

   This should be fixed and @covers added. We are actually testing getEntityTypeId() here.

Im migrating paragraphs, and the map table doesnt create destid2 column using patch #7, but it does with #6. I think it is because \Drupal\entity_reference_revisions\Plugin\migrate\destination\EntityReferenceRevisions now extends from \Drupal\migrate\Plugin\migrate\destination\EntityRevision, that extends from \Drupal\migrate\Plugin\migrate\destination\EntityContentBase which doesnt calls to parent::getIds() so it returns only revision id.

I created a patch #2820886: EntityRevision should calls parent::getIds(); that tries to solve this, anyway attached is a new version which returns both, revision id and id.

@ruloweb, thanks for your feedback. I hadn't noticed that. This addresses that and add test support for all parts.

Did another quick look at the upstream code in EntityContentBase, and I think we're going to need to override rollback(). Primarily because it only make the assumption that we only have a single argument to pass to the storage->load(). And we need to pass both composite keys. But that should be simple, if someone wants to jump on that before I get back to it.

I think this might be the last missing pieces. (Famous last words). But let the reviewing begin. Rollbacks now function correctly.

Patch #12 works pretty ok :) for me, thanks.

There is an small tricky bug on last patch, ids order matters, currently it saves [id, revision_id] but in getIds() it returns [revision_id, id].

On this way, destid1 gets revision_id and destid2 gets id, but when migrate tries to get the entity it will think that destid1 = id and destid2 = revision_id. You will see this bug only if you enable stubs #2810907-7: Migrate SQL Map doesn't get array keys for compound keys.

Attached is a patch which solve this.

Im working with an entity/paragraph sctructure like:

- entity
    - Text field 
    - File field multiple
    - Paragraph field multiple
      - Text field
      - Text field

and it works pretty ok, thanks so much for all the work :)

New patch, It includes the small update from #14 and change how getEntity works, it is inspired a little bit in EntityRevision:getEntity().

In my migration, the nodes get created with ERR stubs, but after I run the ERR migrate, these stubs are not updated, this is because we need to call to updateEntity(), also processStubRow() can be replaced by this function.

Thanks!

Tested and it works beautifully. The migrate map in the database correctly shows both destid's (target_id and target_revision_id) when a destination value of "entity_reference_revisions:paragraph" is set:

destination:
  plugin: entity_reference_revisions:paragraph

(Now if only Migrate::transform would play nicely with this! I'm working on a bug in the Migration process plugin over here: https://www.drupal.org/node/2767643#comment-11768021)

1. +++ b/src/Plugin/Derivative/MigrateEntityReferenceRevisions.php
   @@ -0,0 +1,26 @@
   +class MigrateEntityReferenceRevisions extends MigrateEntityRevision  {
   +  /**
   

   missing docblock and empty line after class declaration.

2. +++ b/src/Plugin/migrate/destination/EntityReferenceRevisions.php
   @@ -0,0 +1,160 @@
   +  protected static function getEntityTypeId($pluginId) {
   +    // Remove "entity_reference_revisions:".
   +    return substr($pluginId, 27);
   +  }
   

   isn't this the same as getDerivativeId() ?

3. +++ b/src/Plugin/migrate/destination/EntityReferenceRevisions.php
   @@ -0,0 +1,160 @@
   +    $entity = $this->loadEntityReferenceRevision(reset($destination_identifiers), array_pop($destination_identifiers));
   

   would it be more readable to hardcode the keys 0 and 1, because I think that's what this is doing?

4. +++ b/src/Plugin/migrate/destination/EntityReferenceRevisions.php
   @@ -0,0 +1,160 @@
   +   * @param mixed $id
   +   *   The ID of the entity to load.
   ...
   +  public function loadEntityReferenceRevision($entityId, $revisionId) {
   

   documentation doesn't match the arguments. should also use $entity_id and so on.

5. +++ b/src/Plugin/migrate/destination/EntityReferenceRevisions.php
   @@ -0,0 +1,160 @@
   +    $entities = $this->storage->loadByProperties([$this->getKey('id') => $entityId, $this->getKey('revision') => $revisionId]);
   

   revision id is unique, loadRevision($revision_id) is actually enough, which means you don't have to pass the entity_id and actually don't need this method at all?

   looking at the test, possibly this is supposed to work if revision_id is missing, but then it would have to set this conditionally?

1. +++ b/core/modules/migrate/tests/modules/migrate_events_test/src/Plugin/migrate/destination/EntityReferenceRevisions.php
   @@ -5,6 +5,7 @@
   +use Drupal\migrate\MigrateException;
   

   This isn't necessary. Seems like it got brought in by accident? Or is it necessary?

2. +++ b/core/modules/migrate/tests/modules/migrate_events_test/src/Plugin/migrate/destination/EntityReferenceRevisions.php
   @@ -71,13 +72,13 @@ public function getIds() {
   +    if (!empty($entity_id) && !empty($revision_id) && ($entity =$this->storage->loadRevision($revision_id))) {
   

   We probably don't need to check if we have a value in entity_id since all we need is the revision_id. If that is true, we can also remove the entire entity_id variable above then (I think). Check and confirm we aren't using it later on, but if we only needed it for loading, then get rid of the variable.

3. #17-2: We cannot call getDerivativeId at this point because it isn't static and getEntityTypeId is static.

@heddn:

use Drupal\migrate\MigrateException;
 

that was necessary because this is part of code e.g

 else {
          throw new MigrateException('This entity type does not support translation.');
        } 

For some reason, this seems like it was applied against core, not ERR. Re-rolling against contrib.

thx, heddn. So, here is first step at a review. I'll need to do more later, especially when I figure out how to run the tests.

1. +++ b/src/Plugin/Derivative/MigrateEntityReferenceRevisions.php
   --- /dev/null
   +++ b/src/Plugin/migrate/destination/EntityReferenceRevisions.php
   

   Can add a use statement for the TranslatableInterface used in rollbackTranslation.

2. +++ b/tests/src/Kernel/Plugin/migrate/destination/EntityReferenceRevisionsDestinationTest.php
   @@ -0,0 +1,103 @@
   +      $entity = $migration->getDestinationPlugin()->loadEntityReferenceRevision(1, 1);
   

   Where is method loadEntityReferenceRevision?. grep didn't find it.

Isn't this the same as getDerivativeId() ?

Let's document the answer to this. Where is the best place?

Has anyone tried the rollback with real data?

I can't get any of these tests to run. What is the command to use?

Strange. Tests are failing locally. They should also on the test bot. Let's see how it fares. Responding to #24 in this patch.

I think I just figured out why. Hopefully we have some failures now.

I successfully ran a migration using this destination plugin
then did a rollback and --force migration both successfully.
I am looking at db tables for content to verify data migrations

drush mi paragraph_migration
drush mr paragraph_migration
drush mi paragraph_migration --force

@Lowell, thanks for the testing!
And I found some minor things.

1. +++ b/tests/src/Kernel/Plugin/migrate/destination/EntityReferenceRevisionsDestinationTest.php
   @@ -0,0 +1,105 @@
   + * Tests the migration plugin.
   

   s/migration plugin/migration destination plugin/

2. +++ b/src/Plugin/Derivative/MigrateEntityReferenceRevisions.php
   @@ -0,0 +1,31 @@
   + * @package Drupal\entity_reference_revisions\Plugin\Derivative
   

   Not familiar with @package, is this needed? There are only 4 instances in core and this is the only one if entity reference revisions.

3. +++ b/src/Plugin/Derivative/MigrateEntityReferenceRevisions.php
   @@ -0,0 +1,31 @@
   +        $this->derivatives[$entityType] = array(
   

   Prefer short array syntax for new code.

4. +++ b/tests/src/Kernel/Plugin/migrate/destination/EntityReferenceRevisionsDestinationTest.php
   @@ -0,0 +1,105 @@
   +   * Tests get entity.
   +   *
   +   * @covers ::getEntity
   +   * @covers ::rollback
   +   * @covers ::rollbackNonTranslation
   +   */
   +  public function testGetEntity() {
   

   This is testing more than get entity. Perhaps the comment can be updated to reflect what this is really doing. And, should rollbackTranslations be tested?

5. +++ b/tests/src/Kernel/Plugin/migrate/destination/EntityReferenceRevisionsDestinationTest.php
   @@ -0,0 +1,105 @@
   +use Drupal\migrate\Plugin\MigrateDestinationPluginManager;
   

   Shows as not used in IDE.

Cleaned up the comments to #30

Here's a more complete test. What I haven't figured out yet is how to get the multiple paragraphs migration stuff to work like I'd like.

And here's the patch. The migrate process plugin will only ever return the first value passed into it. Seems like an issue for something like this.

Who knew that flags to array_filter were new in php 5.6?

I'm going to need to open some core issues against migrate, because the DX here is fairly painful.

Ideally, I'd be able to structure my source data in this format as well:

                  [
                    'id' => 1,
                    'title' => 'Article 1',
                    'photo' => 'Photo1 here',
                    'author1' => 'Author 1',
                    'author2' => 'Author 3',
                  ],
                  [
                    'id' => 2,
                    'title' => 'Article 2',
                    'photo' => 'Photo2 here',
                    'author1' => 'Author 2',
                    'author2' => 'Author 4',
                  ],

However, I cannot do this, because then I have to pass in 'source_ids' and then $value is always ignored and it always returns the first author in the source.

Lastly, it would be super nice if migrate process plugin would allow me to return multiple values. Instead of commenting:
// Break out of the loop as soon as a destination ID is found.
That would make this process even cleaner because I could insert the records in multiple_err_author1 & multiple_err_author2 and use the id column, then pass in id 1, 2 and get back all the entities from both of those migrations that reference id 1 & 2.

While writing up these notes, I figured out a saner way to do the multiple mapping.

Tests are passing locally. Added some additional assertions to see if we can get more details on what is failing.

I figured out why local passed and test bot didn't. I forgot that core patch #2767643: Scalar to array migration returns NULL is required to making this work. Not marking as NR for that reason. However, if you apply that patch, this patch and its tests pass with flying colors.

I've also added to the IS notes on how to use the patch. And I've got someone in the office here writing up a blog post that spells things out in fuller detail.

Interdiff in #44 is fine. Here's the actual patch.

#2767643: Scalar to array migration returns NULL landed so back to NR.

You can see the application of this patch #46 set an example in my blog -- https://www.mtech-llc.com/blog/charlotte-leon/migration-csv-data-paragraphs

+++ b/src/Plugin/migrate/destination/EntityReferenceRevisions.php
@@ -0,0 +1,147 @@
+   * Rollback non-translation destinations.
+   *
+   * @param array $destination_identifiers
+   *   The IDs of the destination object to delete.
+   */
+  protected function rollbackNonTranslation(array $destination_identifiers) {
+    $entity = $this->storage->loadRevision(array_pop($destination_identifiers));
+    if ($entity) {
+      $entity->delete();
+    }
+  }

What exactly is the expected behavior of this method?

This isn't how you delete a revision, this is deleting an entity with all its revisions.

To delete a revision, you need deleteRevision(), but that isn't supported for the default revision.

There are 3 possible scenarios I think:

1. Default revision and single revision
2. default revision but there are other revisions
3. non-default revision

1. and 3 aren't that complicated, we could check for default revision and then either call deleteRevision() or delete(). But no idea what to do about 2. I guess I'm fine with deleting as well then, but we should possibly at least explicitly document this here?

The expected behavior is to rollback all the data that was inserted. Typically this is paragraphs data. So if someone migrates a few paragraphs from CSV, each of those is probably going to be a new paragraph. And deleting the entity is OK. But if someone is migrating from D7, then on rollback, we only want to delete the paragraph(s) that were migrated, and nothing more. D7 will have revisions, so deleting the full entity might not be what is expected. Based on that analysis, I think that means we want to do 1 & 3, but not #2.

@Berdir, does my description make sense and do you agree with that analysis?

AFAIK the current coding standard is still that variables must be $revision_id, but could be fixed on commit.

Also, test fails look unrelated.

Fixed code standards and hopefully the tests now pass. Leaving at RTBC.

I'ved opened #2836129: Failing tests on EntityReferenceRevisionsAutocompleteTest

Committed, however there is a TODO in the code linking to #2783715: Entity destination ID schema should match entity ID definition
It seems this is outdated. So what's to improve?

+      // TODO: Improve after https://www.drupal.org/node/2783715 is finished.
+      $ids[$revision_key]['type'] = 'integer';
+

We needed to wait on getDefinitionFromEntity being available. It is now available in both 8.2.x and 8.3.x. How far back do we need to support things? Do we need to wait until 8.1 is no longer supported?

8.1 is unsupported since the day 8.2 was released :) So yes, it is safe to use it.

I've added #2836432: Use getDefinitionFromEntity inside of getIds migration destination. One thing to consider is that older versions of 8.2.x will not have getDefinitionFromEntity. It was added in 8.2.2.

Hi all,
Who have example for use this migration from D7 to D8 site for paragraph entity?

As far as I know, this patch is for Drupal 8, and you can see an example in the article posted a few comments above https://www.mtech-llc.com/blog/charlotte-leon/migration-csv-data-paragraphs

Hi all, thanks for your work on it.

I can not found this syntax/structure documented in the migration docs (https://www.drupal.org/docs/8/api/migrate-api/migrate-api-overview). Any one knows if is documented somewhere?

If not, makes sense to get a page with an example of how to migrate into paragraphs?If yes, I would like to work on it :)

Hi all, It's a quick question but does this work with PHP 7 or later??

@ohthehugemanatee,
Thank you so much, and good to hear that! (btw, sorry for the late reply.)