We need a migrate destination which stores a compound destination key of entity_id and revision_id .

Keep in mind here that migrating into an ERR *field* is a two step process. First you migrate into paragraphs or whatever that requires ERR. The paragraphs migration is where you use the ERR destination entity. Then you follow that up with a 2nd migration that migrates the relationship into the node or whatever is the content that has the ERR field. Until #2767643: Scalar to array migration returns NULL is committed, you'll need to apply that core patch if you want to get this working. The tests are going to fail here until then.

The patch here let's you run the following scenarios:
Single paragraph:

#source data:
id,photo
1,Photo1 here
2,Photo2 here
id: single_paragraph
  source:
  plugin: <insert source plugin name here>
  ids:
    id
      type: integer

process:
  field_file: <photo>
  .
  .
  .
  Other field mappings as needed
destination:
  plugin: 'entity_reference_revisions:paragraph'

Multi-valued paragraph destination:
Here there are two migrations that are both migrating in data from different sources. You don't need two, but I did it to demonstrate the possibilities.

#source data 1:
id,author
1,Author 1
2,Author 2
id: multiple_paragraphs_1
source:
  plugin: <insert source plugin here>
  ids:
    author
      type: text
    
process:
  field_name: author
  .
  .
  .
  Other field mappings as needed
destination:
  plugin: 'entity_reference_revisions:paragraph'
#source data 2:
id,author
1,Author 3
2,Author 4
id: multiple_paragraphs_2
source:
  plugin: <insert source plugin name here>
  ids:
    author
      type: text
    
process:
  field_name: author
  .
  .
  .
  Other field mappings as needed
destination:
  plugin: 'entity_reference_revisions:paragraph'

Destination migration
Lastly, this should pull those single and multi-valued paragraph migrations together:

#source data:
id,title,photo,author
1,Article 1,Photo1 here,['Author 1', 'Author 3']
2,Article 2,Photo2 here,['Author 2', 'Author 4']
source:
  plugin: <insert source plugin name here>
  ids:
    id
      type: integer
    
process:
  title: title
  type:
    plugin: default_value
    default_value: article
  field_paragraph/target_id
    -
      plugin: migration
      migration: single_paragraph
      no_stub: true,
      source: id
    -
      plugin: extract
      index:
        - '0' #very important to use the single quotes otherwise zero is treated as empty and ignored
  field_err_single/target_revision_id
    -
      plugin: migration
      migration: single_paragraph
      no_stub: true,
      source: id
    -
      plugin: extract
      index:
        - 1
  field_err_multiple
    -
      plugin: migration
      migration:
      multiple_paragraphs_1
      multiple_paragraphs_2
      no_stub: true
      source: author
    -
      plugin: iterator
      process:
        target_id: '0' #very important to use the single quotes otherwise iterator ignores these.
        target_revision_id: '1' #very important to use the single quotes otherwise iterator ignores these.
  .
  .
  .
  Other field mappings as needed
destination:
  plugin: 'entity:node
Files: 

Comments

ohthehugemanatee created an issue. See original summary.

heddn’s picture

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

ohthehugemanatee’s picture

Linking to the core issue

mglaman’s picture

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?

heddn’s picture

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.

heddn’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2.68 KB

Here's some initial thoughts.

heddn’s picture

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

heddn’s picture

  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.

ruloweb’s picture

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.

heddn’s picture

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

heddn’s picture

Status: Needs review » Needs work

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.

heddn’s picture

Status: Needs work » Needs review
FileSize
14.24 KB
4.98 KB

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

ruloweb’s picture

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

ruloweb’s picture

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 :)

ruloweb’s picture

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!

TrevorBradley’s picture

Status: Needs review » Reviewed & tested by the community

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)

Berdir’s picture

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

Adita’s picture

Assigned: Unassigned » Adita
Adita’s picture

Assigned: Adita » Unassigned
Status: Needs work » Needs review
FileSize
14.31 KB
3.95 KB
heddn’s picture

Status: Needs review » Needs work
  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.
Adita’s picture

@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.');
        } 
Adita’s picture

Status: Needs work » Needs review
heddn’s picture

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

quietone’s picture

Status: Needs review » Needs work

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?

heddn’s picture

Status: Needs work » Needs review
FileSize
13.52 KB
2.51 KB

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

heddn’s picture

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

Status: Needs review » Needs work

The last submitted patch, 26: entityreference_migrate-2809793-26.patch, failed testing.

heddn’s picture

Lowell’s picture

Status: Needs review » Reviewed & tested by the community

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
quietone’s picture

Status: Reviewed & tested by the community » Needs work

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

Charlotte17’s picture

Status: Needs work » Needs review
FileSize
14.25 KB
2.45 KB

Cleaned up the comments to #30

heddn’s picture

FileSize
19.4 KB

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.

heddn’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 33: entityreference_migrate-2809793-32.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
24.84 KB
2.49 KB

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

Status: Needs review » Needs work

The last submitted patch, 35: entityreference_migrate-2809793-35.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
23.62 KB
10.86 KB

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

Status: Needs review » Needs work

The last submitted patch, 37: entityreference_migrate-2809793-37.patch, failed testing.

heddn’s picture

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.

heddn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 39: entityreference_migrate-2809793-38.patch, failed testing.

heddn’s picture

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

Status: Needs review » Needs work

The last submitted patch, 42: entityreference_migrate-2809793-42.patch, failed testing.

heddn’s picture

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.

heddn’s picture

Issue summary: View changes
heddn’s picture

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

heddn’s picture

Status: Needs work » Needs review
heddn’s picture

Issue summary: View changes
heddn’s picture

Issue summary: View changes
Charlotte17’s picture

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

Berdir’s picture

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

heddn’s picture

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?

heddn’s picture

Status: Needs review » Needs work

The last submitted patch, 53: entityreference_migrate-2809793-53.patch, failed testing.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

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

Also, test fails look unrelated.

heddn’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 56: entityreference_migrate-2809793-56.patch, failed testing.

heddn’s picture

Status: Needs work » Reviewed & tested by the community
heddn’s picture

  • miro_dietiker committed d105e59 on 8.x-1.x authored by heddn
    Issue #2809793 by heddn, ruloweb, Adita, Charlotte17, Berdir:...
miro_dietiker’s picture

Status: Reviewed & tested by the community » Needs work

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';
+
heddn’s picture

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?

Berdir’s picture

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

heddn’s picture

Status: Needs work » Fixed

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.

ksemihin’s picture

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

Status: Fixed » Closed (fixed)

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

ruloweb’s picture

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