Problem/Motivation
Link fields migrated from D6 should get transformed, but the transform function never runs. The resulting field ends up with a url property and no uri property.
The current test is checking for the wrong field.
Discovered whilst working on https://www.drupal.org/project/drupal/issues/3066751#comment-13179287
There are two problems here that need to fixed together
1) The schema field.value.link is incorrect.
2) The d6 and d7 FieldInstanceDefault process plugin do not transform the link default value correctly.
Proposed resolution
1) Update field.value.link
2) Fix the process plugins and update the migration tests.
The original proposal
If I change the id here:
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/link/s...
to match the d7 version:
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/link/s...
then it runs the transform function. However I then get all these nice errors when running modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldInstanceTest.php
2x: LinkField is deprecated in Drupal 8.3.x and will be be removed before Drupal 9.0.x. Use \Drupal\link\Plugin\migrate\field\d6\LinkField instead.
1x in MigrateFieldInstanceTest::testFieldInstanceMigration from Drupal\Tests\field\Kernel\Migrate\d6
1x in MigrateFieldInstanceTest::testMigrateFieldIntoUnknownNodeType from Drupal\Tests\field\Kernel\Migrate\d6
2x: CckFieldPluginBase is deprecated in Drupal 8.3.x and will be be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Plugin\migrate\field\FieldPluginBase instead.
1x in MigrateFieldInstanceTest::testFieldInstanceMigration from Drupal\Tests\field\Kernel\Migrate\d6
1x in MigrateFieldInstanceTest::testMigrateFieldIntoUnknownNodeType from Drupal\Tests\field\Kernel\Migrate\d6
2x: MigrateCckFieldInterface is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead.
1x in MigrateFieldInstanceTest::testFieldInstanceMigration from Drupal\Tests\field\Kernel\Migrate\d6
1x in MigrateFieldInstanceTest::testMigrateFieldIntoUnknownNodeType from Drupal\Tests\field\Kernel\Migrate\d6
2x: CckLink is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\link\Plugin\migrate\process\FieldLink instead.
1x in MigrateFieldInstanceTest::testFieldInstanceMigration from Drupal\Tests\field\Kernel\Migrate\d6
1x in MigrateFieldInstanceTest::testMigrateFieldIntoUnknownNodeType from Drupal\Tests\field\Kernel\Migrate\d6
Remaining tasks
Fix it
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#25 | 3067609-25.patch | 12.93 KB | quietone |
#25 | diff-21-25.txt | 3.41 KB | quietone |
#21 | 3067609-21.patch | 15.05 KB | quietone |
#21 | interdiff-19-21.txt | 497 bytes | quietone |
#19 | 3067609-19.patch | 14.97 KB | quietone |
Comments
Comment #2
justafishUpdated test attached (that should now fail)
Comment #3
justafishComment #4
alexpottFun. So at first i though woah link field migrations are broken... but that's not it. It's the default values of link fields that are broken for D6 -> d8 migrations. This reveals more bugs - the config schema for default link fields is borked too! Amazing. Here's a start that fixes D6 link field default value migrations. I can't find the equivalent test for D7 yet - maybe this test run will find it. Also we obviously we have no test coverage for default link values because if we did then we'd have config schema errors in HEAD.
Comment #7
alexpottFound #2823679: link.schema.yml is outdated as a duplicate for the schema part. As this fix is slightly more comprehensive going to close the other one and bring the test from that issue over here.
Comment #8
alexpottWow
\Drupal\Tests\link\Functional\LinkFieldUITest
is confusing. Creates a field via the UI and then doesn't use and creates fields via the API. Patch fixes that to use the UI for everything - it is a UI test and thereby makes it a bit easier to understand and more realistic. Patch also adds to the test to test setting a default value via the UI.So now all this patch is missing is to ensure the d7 to d8 default link value is tested somewhere (I suspect it is not) and a rewrite of the issue summary to explain what the bugs are. There are two here and they have to be fixed together.
Oh one more thought is do we need an upgrade path for sites with broken migrations? If possible I think we should fixed the bugs here and file a follow to write an upgrade path to fix sites because the problem is easy to work around for real sites and they problem have done that already.
Comment #10
heddnI'm not sure we need an update path. Process plugin ids haven't changed. It will just run the fixed plugin when this is resolved.
For D7, we will need to do something extra. From https://git.drupalcode.org/project/link/blob/7.x-1.x/link.module#L64, it looks like we need to do similarly since it uses 'url' instead of 'uri'.
Comment #13
quietone CreditAttribution: quietone commentedComment #14
quietone CreditAttribution: quietone commentedStart with a reroll
Comment #15
quietone CreditAttribution: quietone commentedStart adding changes for d7 version.
Comment #16
quietone CreditAttribution: quietone commentedDon't mappings need to declare the keys?
Comment #19
quietone CreditAttribution: quietone commentedNeeds a reroll
Comment #21
quietone CreditAttribution: quietone commentedAdd the title to the mapping for the link schema.
Comment #22
quietone CreditAttribution: quietone commentedComment #23
quietone CreditAttribution: quietone commentedFound another issue that changes link schema, #2397233: Link module has untested features to store additional configuration per link
Comment #24
jibranOverall changes look good. Nothing to object so RTBC.
Comment #25
quietone CreditAttribution: quietone commentedNeeded a reroll
Comment #26
jibranReroll looks good. RTBC++
Comment #28
catchCommitted/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!
This doesn't cherry-pick cleanly to 8.9.x - I think we could probably backport it there but overall we're trying to backport less and less to 8.9.x, if someone wants to take it on, please re-open with a reroll.