Problem/Motivation

When data for fields of type "link" are migrated from D6 to D8, if the link has no "attributes" value, it is incorrectly set to "false" in D8 (instead of an empty array) which causes an issue with LinkGenerator (see comment 16 of #2596937: Array merge failure on line 153 of core/lib/Drupal/Core/Utility/LinkGenerator.php).

Proposed resolution

Better handling of the attributes data in the d6_cck_link process plugin.

Remaining tasks

Review the patch, suggest better ways to handle (than the attached patch). Add a test.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#11 interdiff-2633568-9-11.txt697 bytesquietone
#11 2633568-11.patch2.37 KBquietone
#9 2633568-9.patch2.22 KBquietone
#9 2633568-9-fail.patch1.57 KBquietone
#4 interdiff-2633568-0-4.txt1.32 KBquietone
#4 2633568-4.patch1.57 KBquietone
#4 2633568-4-pass-do-not-commit.patch2.21 KBquietone
#4 2633568-4-fail.patch1.52 KBquietone
d6_link_process.patch648 bytesultimike
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ultimike created an issue. See original summary.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike at DrupalEasy commented
Related issues: +#2596937: Array merge failure on line 153 of core/lib/Drupal/Core/Utility/LinkGenerator.php
quietone’s picture

quietone CreditAttribution: quietone commented
Issue tags: +migrate-d6-d8
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
2633568-4-fail.patch1.52 KB
2633568-4-pass-do-not-commit.patch2.21 KB
2633568-4.patch1.57 KB
interdiff-2633568-0-4.txt1.32 KB

if the link has no "attributes" value

Testing showed that if the attribute valid is NULL or '', then this does fail. This failure will whenever the attribute data is not a serialized string causing unserialize to return FALSE. Not sure why there would be unserializable data in that field. I wasn't able to get erroneous data in the attributes field through the UI and the source isn't mentioned in the IS. Still, we need to test it and make sure that $route['options']['attributes'] has meaningful data.

Tests have been added to MigrateNodeTest for the link values. There are a couples of patches here because I added 'bad' data to the attribute field in the test fixture for testing.

The attached patches

  • 2633568-4-fail.patch - With tests and 'bad' data.
  • 2633568-4-pass-do-not-commit.patch - Same as above but with the fix.
  • 2633568-4.patch - The fix and test fixture untouched.

Note I never hit the breakpoint set in core/lib/Drupal/Core/Utility/LinkGenerator.php perhaps because I was testing with d6/MigrateNodeTest.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review

The last submitted patch, 4: 2633568-4-fail.patch, failed testing.

The last submitted patch, 4: 2633568-4-pass-do-not-commit.patch, failed testing.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Status: Needs review » Needs work

We need to leave the bad source data otherwise we're not actually testing the original issue. Also, super small issue I noticed below:

+++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeTest.php
@@ -57,6 +57,11 @@ public function testNode() {
+// Test that link fields are migrated.

Bad indent on this line.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
2633568-9-fail.patch1.57 KB
2633568-9.patch2.22 KB

Well, don't know what I was thinking, definitely took the wrong path. Anyway, this should be a fail patch and fix.
thx benjy.

The last submitted patch, 9: 2633568-9-fail.patch, failed testing.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
2633568-11.patch2.37 KB
interdiff-2633568-9-11.txt697 bytes

Added a comment.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to all three 8.x branches, thanks!

  • catch committed d6f5694 on 8.2.x 
    Issue #2633568 by quietone, ultimike: Improve method for migrating link...

  • catch committed 6c567ae on 8.1.x 
    Issue #2633568 by quietone, ultimike: Improve method for migrating link...

  • catch committed 27ede57 on 8.0.x 
    Issue #2633568 by quietone, ultimike: Improve method for migrating link...

Status: Fixed » Closed (fixed)

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