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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

justafish created an issue. See original summary.

justafish’s picture

Status: Active » Needs work
FileSize
1.06 KB

Updated test attached (that should now fail)

justafish’s picture

Issue summary: View changes
alexpott’s picture

Status: Needs work » Needs review
FileSize
2.52 KB

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

alexpott’s picture

Found #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.

alexpott’s picture

Wow \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.

The last submitted patch, 8: 3067609-8.test-only.patch, failed testing. View results

heddn’s picture

Status: Needs review » Needs work

I'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'.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Issue summary: View changes
Issue tags: +Bug Smash Initiative
quietone’s picture

Status: Needs work » Needs review
FileSize
2.71 KB
9.63 KB

Start with a reroll

quietone’s picture

Start adding changes for d7 version.

quietone’s picture

+++ b/core/modules/link/config/schema/link.schema.yml
@@ -54,12 +54,15 @@ field.value.link:
+    attributes:
+      type: mapping
+      label: 'Link attributes'
+    uri:

Don't mappings need to declare the keys?

Status: Needs review » Needs work

The last submitted patch, 15: 3067609-15.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

quietone’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
14.97 KB

Needs a reroll

Status: Needs review » Needs work

The last submitted patch, 19: 3067609-19.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
497 bytes
15.05 KB

Add the title to the mapping for the link schema.

quietone’s picture

Title: D6 migration of link fields doesn't run any transform » Fix config schema for links and migration of link default values
Issue summary: View changes
Issue tags: +migrate-d6-d8, +migrate-d7-d8
quietone’s picture

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Overall changes look good. Nothing to object so RTBC.

quietone’s picture

Needed a reroll

jibran’s picture

Reroll looks good. RTBC++

  • catch committed c2cbb6f on 9.2.x
    Issue #3067609 by quietone, alexpott, justafish, sheanhoxie,...
catch’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/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.

  • catch committed 82e82fd on 9.1.x
    Issue #3067609 by quietone, alexpott, justafish, sheanhoxie,...

Status: Fixed » Closed (fixed)

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