Problem/Motivation
Currently, there is no support to migrate ECK entity types, bundles and ECK entities directly from Drupal 7 sites.
The following patch adds support to migrate entity types, bundles and entities.
To test:
- Install migrate_upgrade and migrate_tools module.
- Run drush migrate-upgrade --legacy-db-url=mysql://drupaluser@127.0.0.1:3306/OLD_DB --legacy-root=http://oldsite.com --verbose --configure-only
- You should have new migrations ready to run upgrade_d7_eck_types, upgrade_d7_eck_bundles and one migration per bundle
Proposed resolution
Do it!
Remaining tasks
Write migrations
Add source plugin, migration tests, functional tests and a deriver test.
Review
Manual testing - The current failing test can be ignored. It passes locally just not on testbot.
Commit
Comment | File | Size | Author |
---|
Issue fork eck-2815453
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 2815453-add-support-to changes, plain diff MR !2
Comments
Comment #2
hernani CreditAttribution: hernani commentedComment #3
legolasboComment #4
legolasboIt would be great if this could also have some automated tests to prove that the functionality works and keeps working.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedSo this MOSTLY seems to work. However when I migrate the eck instances it bombs out:
Error: Call to a member function getType() on null in Drupal\\migrate\\Plugin\\migrate\\destination\\EntityContentBase->getDefinitionFromEntity()
In the ECKInstance patch from this class, this is hardcoded:
Since our entity type was not 'node' in this case, it was failing. When I manually changed it to the eck type I'm using it worked...
Not sure how to fix it without diving in much further, maybe OP has an idea.
Comment #6
Costas Vassilakis CreditAttribution: Costas Vassilakis commentedThanks 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?
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedHmm not sure on that one. I ended up separating the D7 entities from our app and I don't recall if I noticed that or not.
Comment #8
DamienMcKennaWith core 8.6.x this throws the following error:
Comment #9
heddnLinking in #2906878: [Meta] Support for D7 -> D9 contrib migrate
Comment #10
heddnLinking 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.
Comment #11
heddnYou might want to tag as content or configuration. See https://www.drupal.org/node/2944527
Class::class syntax is preferred.
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.
Comment #12
jcnventura CreditAttribution: jcnventura at Wunder commented@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:
{"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.
Comment #13
heddnFor 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.
This should be tagged on the destination plugin.
This should get set in the process pipeline.
This should happen in the process pipeline with a concat process plugin.
This should use a deriver. See ContentEntityDeriver and ContentEntity in core.
Comment #14
heddnOne 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...
Comment #15
jcnventura CreditAttribution: jcnventura at Wunder commented@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.'?
Comment #16
jcnventura CreditAttribution: jcnventura at Wunder commentedI 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.
Comment #17
jcnventura CreditAttribution: jcnventura at Wunder commentedA 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().
Comment #18
jcnventura CreditAttribution: jcnventura at Wunder commentedAs 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.
Comment #19
legolasboI 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.
Shouldn't we add "Entity Construction Kit" to the list of tags?
Doesn't the d7 version have more fields?
Shouldn't we add "Entity Construction Kit" here?
Add "Entity Construction Kit"?
Copy->paste error?
/s/node/eck
So instead of returning 'node' shouldn't we actually make sure the target entity type exists?
Comment #20
heddnHaving 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.
Comment #21
heddnre #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.
Comment #22
legolasboYes please, I'd rather not commit this without at least a minimum of tests.
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.
I guess that covers it then.
Comment #23
jcnventura CreditAttribution: jcnventura commentedNeeds to be reworked now that #2951550: Make service for field discovery for use in migrate entity derivers is in.
Comment #24
jcnventura CreditAttribution: jcnventura commentedComment #25
Ahmed_Samir CreditAttribution: Ahmed_Samir as a volunteer and at Clearwater Development commentedRunning 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.
Comment #26
laura.gates#25 I tried out your patch but I'm still getting the TypeError: Argument 6
Comment #27
Ahmed_Samir CreditAttribution: Ahmed_Samir as a volunteer and at Clearwater Development commentedHi Laura, how are you applying the patch?
I'd also make sure the patch is applied correctly and then do drush cr.
Comment #28
laura.gates@Ahmed,
I update the
composer.json
and runcomposer update drupal/eck --with-dependencies
then dodrush cr
anddrush updb
Forgot to add, I ran into this upgrading from 8.7.11 to 8.8.1
Comment #29
jcnventura CreditAttribution: jcnventura commented@Ahmed_Samir, can you provide an interdiff between the patch in #17 and #25?
Comment #30
Ahmed_Samir CreditAttribution: Ahmed_Samir as a volunteer and at Clearwater Development commentedAttaching interdiff.
Comment #31
DamienMcKennaHas anyone considered migrating to block types instead?
Comment #32
DamienMcKennaI'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.
Comment #33
OFF CreditAttribution: OFF commentedI'm using patch #25, but eck entities is not migrated. I need to use only drush to perform migration?
Comment #34
jcnventura CreditAttribution: jcnventura commented@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.
Comment #35
DamienMcKenna@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 .
Comment #36
jsibley CreditAttribution: jsibley commentedIs 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!
Comment #37
Alejo DHey,
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.
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedI 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.
Comment #39
MatroskeenAwesome! 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.
Comment #40
DamienMcKennaFYI this doesn't work with D9 as MigrateCckFieldPluginManagerInterface was deprecated; see https://www.drupal.org/node/2751897 for details.
Comment #41
Alejo DHey @DamienMcKenna check my Patch for D9 #37 that will fix it.
Comment #42
MatroskeenMoving to NW for tests. The database fixture is available in the 8.x-1.x branch: #3188128: Add D7 fixture for ECK migration tests.
Comment #44
quietone CreditAttribution: quietone as a volunteer commented@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:
Comment #45
quietone CreditAttribution: quietone as a volunteer commentedRe 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.
Comment #46
quietone CreditAttribution: quietone as a volunteer commentedAnd find out why Drupal\Tests\eck\Functional\MigrateUpgradeReviewPageTest passes locally but not on testbot.
Comment #47
quietone CreditAttribution: quietone as a volunteer commentedI 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.
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedTodo:
\Drupal\eck\Plugin\migrate\source\d7\EckEntityTranslation::fields needs descriptions
Comment #49
quietone CreditAttribution: quietone as a volunteer commentedThis 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
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.
Comment #50
quietone CreditAttribution: quietone as a volunteer commentedComment #51
Matroskeen@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'));
I hope it'll help to figure it out.
Also, we recently got rid of
drupalPostForm()
andt()
usage in #3175017: Remove deprecated drupalPostForm from functional tests, so we'll have to do the clean-up before commit.Comment #52
quietone CreditAttribution: quietone as a volunteer commentedGood, tests are passing now. Still have to cleanup, though.
@Matroskeen, what do you want to do about #49.1?
Comment #53
MatroskeenGreen 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 alsoNULL
fortarget_bundles
property. After a quick review, I found a comment inDrupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection
:I think that's exactly what we're looking for.
I also added some comments after reviewing the merge request, so keeping it as NW.
Comment #54
MatroskeenWe 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 byd7_eck
migration because the table haslangcode
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.Comment #55
MatroskeenI 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;
Comment #56
MatroskeenOk, 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:
Comment #57
MatroskeenEntity 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 😋
Comment #58
quietone CreditAttribution: quietone as a volunteer commented@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
Comment #59
MatroskeenThanks 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
: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 🤩
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedOf 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.
Comment #62
Matroskeen@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.
Comment #64
jcnventura CreditAttribution: jcnventura commentedNice! Thanks a lot for adding this!
Comment #66
Alex Bukach CreditAttribution: Alex Bukach commentedWhen 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.)
Comment #67
emileacroweb CreditAttribution: emileacroweb commentedJust 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>
Comment #68
Alex Bukach CreditAttribution: Alex Bukach commented@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.