Add migrations for Drupal 7 ECK to Drupal 8/9

It would be great if this could also have some automated tests to prove that the functionality works and keeps working.

Thanks to vilepickle #5 it worked. I managed to migrate the entities and the field values from a D7 site but unfortunately the entity titles are still missing. I don't receive any error about this though. Any idea?

With core 8.6.x this throws the following error:

The website encountered an unexpected error. Please try again later.Drupal\migrate\Plugin\Exception\BadPluginDefinitionException: The d7_eck_entity_bundle plugin must define the source_module property. in Drupal\migrate_drupal\MigrationPluginManager->processDefinition() (line 104 of core/modules/migrate_drupal/src/MigrationPluginManager.php).

Drupal\migrate\Plugin\MigrationPluginManager->findDefinitions() (Line: 175)

Linking in #2906878: [Meta] Support for D7 -> D9 contrib migrate

Linking in #2906878: [Meta] Support for D7 -> D9 contrib migrate

https://www.drupal.org/node/2831566
and
https://www.drupal.org/node/2920988
both seem helpful here.

1. +++ b/migration_templates/d7_eck.yml
   @@ -0,0 +1,20 @@
   +migration_tags:
   +  - Drupal 7
   
   +++ b/migration_templates/d7_eck_bundles.yml
   @@ -0,0 +1,19 @@
   +migration_tags:
   +  - Drupal 7
   
   +++ b/migration_templates/d7_eck_types.yml
   @@ -0,0 +1,13 @@
   +migration_tags:
   +  - Drupal 7
   

   You might want to tag as content or configuration. See https://www.drupal.org/node/2944527

2. +++ b/src/Plugin/Derivative/MigrateECKEntity.php
   @@ -0,0 +1,68 @@
   +      $class = 'Drupal\eck\Plugin\migrate\destination\ECKInstance';
   

   Class::class syntax is preferred.

3. +++ b/src/Plugin/migrate/destination/ECKEntityBundle.php
   @@ -0,0 +1,53 @@
   +class ECKEntityBundle extends EntityConfigBase {
   
   +++ b/src/Plugin/migrate/destination/ECKEntityType.php
   @@ -0,0 +1,23 @@
   +class ECKEntityType extends EntityConfigBase {
   
   +++ b/src/Plugin/migrate/destination/ECKInstance.php
   @@ -0,0 +1,88 @@
   +class ECKInstance extends EntityContentBase {
   

   Why do we need special destinations? The config or content destinations should work from what I can tell.

Also of note, you might want to pend or add a TODO to use the work coming out of #2951550: Make service for field discovery for use in migrate entity derivers.

@heddn, would appreciate your review on this re-work of the patch.

I've simplified the destination plugins considerably, but I have to confess that I don't yet understand the derivers that well. I've taken into account your comments in #10 and #11.

Things still to do:

• Not hardcode the use of 'title'. The D7 eck_entity_type table includes a properties column, that can be used to add properties (fields, but not using the field API). One of these 'default properties' is the title, which when enabled sets the 'properties' column like: {"title":{"label":"Title","type":"text","behavior":"title"}}

. I was unable to change the d7_eck_types.yml migration to obtain the now boolean value from these properties.

• Move the migration configuration from migration_templates to migrations (https://www.drupal.org/node/2920988). I'll do that on the next iteration, where the interdiff should be way smaller.
• I've reduced the ECKEntityType destination plugin to an empty shell. How do I get rid of it? i.e. how can I use the EntityConfigBase plugin directly?
• Use the new traits. Specifically the new FieldMigrationTrait

For derivers, take a look at D7NodeDeriver in core or https://cgit.drupalcode.org/entity_reference_revisions/tree/src/Plugin/D....

Basically, a deriver is a generator. You want to generate new migrations on the content side for each entity type. So each derived migration can migrate all the fields on that content type.

Derivers use a delimitor. For entity destinations, migrate uses a deriver too. Look at MigrateEntity in core. It splits on a colon. So 'entity:node' means to use the derived node destination.

Sorry, I didn't have much more time this AM, but here are some drive-by, quick feedback points.

1. +++ b/migration_templates/d7_eck_bundles.yml
   @@ -0,0 +1,20 @@
   +  destination_module: eck
   

   This should be tagged on the destination plugin.

2. +++ b/migration_templates/d7_eck_bundles.yml
   @@ -0,0 +1,20 @@
   +  entity_type: entity_type
   

   This should get set in the process pipeline.

3. +++ b/src/Plugin/migrate/destination/ECKEntityBundle.php
   @@ -0,0 +1,25 @@
   +    $row->setDestinationProperty('entityTypeId', $row->getSourceProperty('entity_type') . '_type');
   

   This should happen in the process pipeline with a concat process plugin.

4. +++ b/src/Plugin/migrate/source/d7/ECKEntity.php
   @@ -0,0 +1,86 @@
   +    foreach (array_keys($this->getFields($this->entityType, $this->bundle)) as $field) {
   

   This should use a deriver. See ContentEntityDeriver and ContentEntity in core.

One other passing thought, you might try what paragraphs/field collections is doing. And break this down into smaller chunks that are easier to review. Instead of boiling the entire ocean at one time. See #2897021: [META] Migrate support for importing field collections as paragraphs. First the config for the types. Then the sources for the data. Then the destinations. Then the fields.

You'll want a good source db fixture to work with, so read up on https://www.drupal.org/docs/8/api/migrate-api/generating-database-fixtur...

@heddn, on #14, no. Don't have time to deal with one issue, much less 4.

As to #13, what do you mean with 'This should get set in the process pipeline.'?

I think I see what you mean. Sorry no speako migrationo. In any case, that line is useless. That and setting up some variables in the source 'pipelines' seems to be some remnant of previous attempts by @hernani.

As to #3, It can't happen in the process pipeline. There's an error "Call to undefined method Drupal\eck\Entity\EckEntityBundle::updateOriginalValues()" if I try to set entityTypeId via the migration config. Figuring it out seems to be above my knowledge of how to create bundles on the fly so I'll leave it as is, as it seems to work, even if it is a hack.

A new attempt, still monolithic (sorry...). I'm not sure if it makes any sense to read the interdiffs, or just go directly to the patch.

As to my comments in #12 and from @heddn in #13

12.1 I've now migrated the values of the properties array, using some code in the source prepareRow().

12.2 Moved all .yml files from migration_templates to migrations.

12.3 Simply removed the file, and no harm done, so that answered my question.

13.1 destination_module is now in the destination plugin annotations and not in the migration .yml files.

13.2 As mentioned in #16, those destination properties are irrelevant, and were deleted (same as with variables)

13.3 As mentioned in #16, nothing can be done there, without breaking functionality.

13.4 Tried to check other classes extending FieldableEntity in core. None use a deriver to get the fields in prepareRow().

As to the FieldMigrationTrait, that would block this while #2951550: Make service for field discovery for use in migrate entity derivers is not committed. I think it's better not to wait.

I just had a look through the patch. I think only the last item could be considered an actual issue, everything else is just me nitpicking.

1. +++ b/migrations/d7_eck.yml
   @@ -0,0 +1,19 @@
   +migration_tags:
   +  - Drupal 7
   +  - Content
   

   Shouldn't we add "Entity Construction Kit" to the list of tags?

2. +++ b/migrations/d7_eck.yml
   @@ -0,0 +1,19 @@
   +process:
   +  id: id
   +  title: title
   

   Doesn't the d7 version have more fields?

3. +++ b/migrations/d7_eck_bundles.yml
   @@ -0,0 +1,16 @@
   +migration_tags:
   +  - Drupal 7
   +  - Configuration
   

   Shouldn't we add "Entity Construction Kit" here?

4. +++ b/migrations/d7_eck_types.yml
   @@ -0,0 +1,28 @@
   +migration_tags:
   +  - Drupal 7
   +  - Configuration
   

   Add "Entity Construction Kit"?

5. +++ b/src/Plugin/migrate/D7ECKDeriver.php
   @@ -0,0 +1,178 @@
   +      // we'll create a migration just for the node properties.
   

   Copy->paste error?

   /s/node/eck

6. +++ b/src/Plugin/migrate/destination/ECKInstance.php
   @@ -0,0 +1,36 @@
   +    // We need an existing entity, or the migration will abort with the error:
   +    // "The "xxx" entity type does not exist.
   +    return 'node';
   

   So instead of returning 'node' shouldn't we actually make sure the target entity type exists?

+++ b/src/Plugin/migrate/destination/ECKEntityBundle.php
@@ -0,0 +1,26 @@
+class ECKEntityBundle extends EntityConfigBase {

+++ b/src/Plugin/migrate/destination/ECKInstance.php
@@ -0,0 +1,36 @@
+class ECKInstance extends EntityContentBase {

Having a destination like this is a code smell. There's something wrong that requires this. If there were specific things we needed to override, then yes. But we're just doing things that can normally happen in the process pipeline.

Any chance we can add a test fixture and some tests? Similar to how #2563649: Migrations: Metatag-D7 basic entities added some very basic data to test this thing.

re #19: 1,3,4 => these tags have significance to the migration system. Adding more tags is fine, but unless there is a good reason that we know of, don't add them.

#2: the extra fields are added via the field deriver, unless there are some 'created' and other properties that were always available directly on the ECK in D7. Then those should be added, yes.

#5/6: I think this could be fixed by getting rid of the destination.

Any chance we can add a test fixture and some tests? Similar to how #2563649: Migrations: Metatag basic entities added some very basic data to test this thing.

Yes please, I'd rather not commit this without at least a minimum of tests.

re #19: 1,3,4 => these tags have significance to the migration system. Adding more tags is fine, but unless there is a good reason that we know of, don't add them.

I'm not familiar enough with the migration system to be able to determine if more tags should be added then. I'll leave it up to you.

#2: the extra fields are added via the field deriver,

I guess that covers it then.

Needs to be reworked now that #2951550: Make service for field discovery for use in migrate entity derivers is in.

Running drush mim example_migration on a Drupal 8.8 throws this error:

[error] TypeError: Argument 6 passed to Drupal\eck\Plugin\migrate\source\d7\ECKEntity::__construct() must implement interface Drupal\Core\Entity\EntityManagerInterface, instance of Drupal\Core\Entity\EntityTypeManager given.

Attaching a re-roll.

#25 I tried out your patch but I'm still getting the TypeError: Argument 6

TypeError: Argument 6 passed to Drupal\eck\Plugin\migrate\source\d7\ECKEntity::__construct() must implement interface Drupal\Core\Entity\EntityManagerInterface, instance of Drupal\Core\Entity\EntityTypeManager given, called in /var/www/html/docroot/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php on line 104 in Drupal\eck\Plugin\migrate\source\d7\ECKEntity->__construct() (line 83 of modules/contrib/eck/src/Plugin/migrate/source/d7/ECKEntity.php).

Drupal\eck\Plugin\migrate\source\d7\ECKEntity->__construct(Array, 'd7_eck_entity', Array, Object, Object, Object) (Line: 104)
Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase::create(Object, Array, 'd7_eck_entity', Array, Object) (Line: 57)
Drupal\migrate\Plugin\MigratePluginManager->createInstance('d7_eck_entity', Array, Object) (Line: 490)
paragraphs_migration_plugins_alter(Array, NULL, NULL) (Line: 539)
Drupal\Core\Extension\ModuleHandler->alter('migration_plugins', Array) (Line: 366)
Drupal\Core\Plugin\DefaultPluginManager->alterDefinitions(Array) (Line: 266)
Drupal\migrate\Plugin\MigrationPluginManager->findDefinitions() (Line: 175)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 155)
Drupal\migrate\Plugin\MigrationPluginManager->expandPluginIds(Array) (Line: 114)
Drupal\migrate\Plugin\MigrationPluginManager->createInstances(Array, Array) (Line: 100)
Drupal\migrate\Plugin\MigrationPluginManager->createInstance('upgrade_d7_authmap') (Line: 143)
Drupal\migrate_tools\Controller\MigrationListBuilder->buildRow(Object) (Line: 235)
Drupal\Core\Entity\EntityListBuilder->render() (Line: 23)
Drupal\Core\Entity\Controller\EntityListController->listing('migration')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 694)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Hi Laura, how are you applying the patch?

I'd also make sure the patch is applied correctly and then do drush cr.

@Ahmed,

I update the composer.json and run composer update drupal/eck --with-dependencies then do drush cr and drush updb

Forgot to add, I ran into this upgrading from 8.7.11 to 8.8.1

@Ahmed_Samir, can you provide an interdiff between the patch in #17 and #25?

Attaching interdiff.

Has anyone considered migrating to block types instead?

I've spun this off as a separate module for migrating ECK entities to blocks: https://www.drupal.org/project/migrate_eck2blocks I've given everyone who has helped on this issue commit credit on the initial commit, I'm happy to give anyone comaintainer access if they're interested.

I'm using patch #25, but eck entities is not migrated. I need to use only drush to perform migration?

@DamienMcKenna, well eck exists in D7, and continues to exist in D8. an eck -> eck migration like the one here would be the best solution. Other than D8 eck entities, the next best thing would be a migration to generated-code entities.

@jcnventura: Agreed. My use case is a little specific, I figured some others might be able to benefit from it too and I'm grateful for everyone's work here .

Is it possible that the migration expects the database key to be "migrate"? I originally had a different database key and had problems getting any of the migrate commands to work until I changed the key to "migrate". There was a "database not found" that didn't identify where the problem was, so I'm not sure that it was eck, but thought it would be helpful to check, so that, if true, it could be documented or changed. Thanks!

Hey,

For D9 the interface MigrateCckFieldPluginManagerInterface has been removed/replaced to use only 'plugin.manager.migrate.field'

I updated your patch for D9 to prevent the error 'The plugin plugin.manager.migrate.cckfield doesn't exists'

Thanks.

I have read thorough this issue and looked at the patch. I started to do a review and then decided to stop in favor of offering to write tests for this. I will do that if someone creates a D7 text fixture and uploads it here. If you do, ping me in #migration.

Awesome! Thanks, @quietone!
I created a separate issue for the fixtures so it's easier to review/commit: #3188128: Add D7 fixture for ECK migration tests.

FYI this doesn't work with D9 as MigrateCckFieldPluginManagerInterface was deprecated; see https://www.drupal.org/node/2751897 for details.

Hey @DamienMcKenna check my Patch for D9 #37 that will fix it.

Moving to NW for tests. The database fixture is available in the 8.x-1.x branch: #3188128: Add D7 fixture for ECK migration tests.

@Matroskeen, thanks!

The goal of the first set of changes is to
a) have changes pass phpcs
b) add source, migration and functional tests
c) no code changes to existing plugins or migrations.

Things to do:

• At first look \Drupal\eck\Plugin\migrate\destination\ECKEntityBundle is not needed.
• The entity counts in MigrateUpgradeEckTests are a guess
• Rename files according to naming conventions
• Make it all work!

Re the derivier, the Cck plugins were deprecated in 8.3.x so all that has been removed and the field discovery service introduced in 8.7.x is now used. Writing the test for the deriver exposed that the original derivier was adding fields to the 2nd complex_entity bundle, another_bundle, that it should not have been added. I am basing that on looking at the d7 field_config_instance table.

Still to do : translations.

And find out why Drupal\Tests\eck\Functional\MigrateUpgradeReviewPageTest passes locally but not on testbot.

I was not able to create a query that would get entity without translations and those with translations and have two IDs, the id and the language. So, there are not two migrations, d7_eck and d7_eck_translation, ans two source plugins. The unusual thing is that they share a deriver and the query in each sets a value, eck_do_not_derive, in the result that will alert the deriver to skip this row. That way, the non translation migration will not generate migrations for entities with translation. It seems a bit hacky but works.

Still to do; cleanup and add comments.

Todo:
\Drupal\eck\Plugin\migrate\source\d7\EckEntityTranslation::fields needs descriptions

This is now almost finished. All the migrations, tests etc are here. It needs review and manual testing would be great.

There are two things to sort out

1. When the source entity reference target_bundle is an empty array it becomes a NULL on the destination. Doesn't an empty array mean that all the bundles can be a target? If so, that either needs documentation to let folks know how to fix that post update or we write some code.
2. The failing test passes locally on Drupal 8.9.x, ddev environment, PHP 7.3.25, MariaDB 10.3.25. I don't know why and am too tired to think about it.

I hope to do a self review but that won't be for a few days. So, if you want to review or test it go right ahead.

@quitone, thanks a lot!

I didn't test the migration itself, but I was managed to reproduce failed test in my local environment.

The following submit fails with the validation error:
$this->drupalPostForm(NULL, $this->edits, t('Review upgrade'));

<ul class="messages__list">
  <li class="messages__item"><h3>Resolve all issues below to continue the upgrade.</h3></li>
  <li class="messages__item">Failed to read from Document root for public files.</li>
  <li class="messages__item">Failed to read from Document root for private files.</li>
</ul>

I hope it'll help to figure it out.

Also, we recently got rid of drupalPostForm() and t() usage in #3175017: Remove deprecated drupalPostForm from functional tests, so we'll have to do the clean-up before commit.

Good, tests are passing now. Still have to cleanup, though.

@Matroskeen, what do you want to do about #49.1?

Green tests look nice! 👍

re: #49.1:
I think NULL is fine and we can keep it as-is.

I was wondering what can be different for ECK entities and took a look into Drupal core fixture. I found a field with empty target_bundles - field_user_entityreference and noticed that the destination config for this field has also NULL for target_bundles property. After a quick review, I found a comment in Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection:

// For the 'target_bundles' setting, a NULL value is equivalent to "allow
// entities from any bundle to be referenced" and an empty array value is
// equivalent to "no entities from any bundle can be referenced".

I think that's exactly what we're looking for.

We recently had a bug report in Slack claiming that "for some reason all image fields fail to migrate", so I wanted to give it a shot and add extra test coverage to verify whether file fields are migrated properly.

I added a new "field_file" to the "Complex entity" and updated the database fixture.
Then I was trying to add one more entry to MigrateEckTest::testEck, but I noticed that complex_entity entities are ignored by d7_eck migration because the table has langcode property. Finally, I begin to understand your comment #47, @quietone. I'll try to revisit it later and see what we can do about it.

I think I was able to create a query to retrieve only entity translations, so I had to update existing tests and also combine some of them.

Remaining items:
- figure out why testMigrateUpgrade is failing;
- add more fields to verify in MigrateEckTest::testEck();
- test "Upgrade" manually;

Ok, great. I discovered the follow-up migrations (https://www.drupal.org/node/2955658) and it worked great here.
I also had a chance to test "Upgrade" manually - it works, entity types and entities are created successfully.

Updating remaining items:

• add more fields to verify in MigrateEckTest::testEck();
• make sure entity translation settings are migrated (it seems they're not);
• add some asserts for the migrated entity translation;

Entity translations settings are not migrated automatically, because the d7_entity_translation_settings plugin supports only Drupal core entity types. We'll do it by providing our own migration in a follow-up #3202616: Add migration for ECK entity translation settings because the merge request is already huge and I don't want to postpone it any further.

Other remaining items were taken care of in the last commits.

I want to keep it for a while in case someone would like to review it 😋

@Matroskeen, I read through all the changes since I last looked at this. It was tidy work and easy to follow. Glad you made a source plugin for the translations and nice work on the deriver to remove the eck_bundle destination plugin.

The one question I can't resolve is why d7_eck and d7_eck_translation are Follow-up migrations. Why do these need to be done after all the other migrations?

Cheers

Thanks for the review and CS fixes!

When I removed a custom destination plugin in 71fc21f4, the entities were not migrated because of this piece of code in Drupal\migrate\Plugin\MigrateDestinationPluginManager:

if (substr($plugin_id, 0, 7) == 'entity:' && !$this->entityTypeManager->getDefinition(substr($plugin_id, 7), FALSE)) {
  $plugin_id = 'null';
}

During the discovery phase (when migrations are derived) the entity type doesn't exist yet, which leads to 'null' plugin.
Follow-up migrations did the trick because they're derived when the entity type already there 🤩

Of course, that makes perfect sense!

I do think 'why these are follow up migrations' should be documented. I'm thinking at least in the yml files, which is the first place one looks to know what a migration does. Something like 'This is a follow up migration so that all the ECK entities exist when the deriver executes'. or some such thing.

@quietone, it was done in the last commit.

I wanted to merge via drupal.org UI, but something went wrong there, so I just went ahead and committed the patch to 8.x-1.x branch.
Huge thanks to everyone for working hard to make this 4 years 5 month old issue done! 🙌

And also special thanks to @quietone for picking this up and adding massive test coverage.
I must admit, it motivated me to test it manually and learn about derivers and follow-up migrations.

Nice! Thanks a lot for adding this!

When I run `drush migrate:upgrage --configure-only`, the migrations for entities (`d7_eck`) are not created. Is it expected, should they be created manually? (The migration for the entity types and bundles are created as expected.)

Just echoing the above from @Alex. Migrating from Drupal 7 to 9, it seems that the types and bundle configurations migrate correctly but the actual entities themselves are not having migrations generated.

If I understand correctly, there should be the following migrations:
upgrade_d7_eck_type
upgrade_d7_eck_bundle

then there should be a migration per bundle (for all of the actual entities themselves.)

I think that these further migrations are the derived, follow up migrations but it does not look like they are being generated.

I'm wondering if I'm missing a step somewhere? Can anyone shed any light?

Edit:
It seems that the issue isn't that the bundles are not generated, it's that they are not available as yaml files when migrations are exported. This means that I can't see an easy way to add them to a custom migration module.

If you use the command
drush migrate-status

You can see them and they appear to be in the "default" migration group. However, they are not listed if you navigate to '/admin/structure/migrate/manage/default/migrations'

For anyone else struggling this, you can run the migrations and the naming convention is:
d7_eck:<eck entity_type>:<eck_bundle>

@emileacroweb I have found that all Follow-up migrations are skipped by \Drupal\migrate_drupal\MigrationConfigurationTrait::getMigrations() which is used to build a list of migrations to generate. I have commented out the check in in to generate the ECK (and other follow-up) migrations.