Follow-up to #2402919: D6-D8 Block Migration

Problem/Motivation

Fresh D6 install. Create one custom block. Run the migration. The custom block migrates to D8 and show up in the DB and even show in the correct region of the block layout admin screen. However, the block is not visible on the page.

CommentFileSizeAuthor
#25 2410623-25-REVIEW-do-not-test.patch8.93 KBultimike
#25 2410623-25.patch457.39 KBultimike
#23 2410623_23.patch496.46 KBchx
#20 2410623-20.patch400.81 KBultimike
#18 2410623-18.patch400.48 KBultimike
#15 2410623-13-REVIEW-do-not-test.patch6.54 KBultimike
#13 2410623-13-REVIEW.patch6.54 KBultimike
#13 2410623-13.patch400.48 KBultimike
#9 2410623-9.patch1.07 KBultimike
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Related issues: +#2221805: Manual Testing: D6 Block
lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

Do I need some sort of region mapping?

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

Which theme is active on the D6 site?

-mike

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

Garland. Then, I'm looking at Bartik on D8. I think that explains it. I'll try it again later and update the issue.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

Hmm - a simple migration from a D6 core theme to a D8 core theme _should_ work. Which D6 (Garland) region was the block in?

-mike

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

Yeah, I didn't do anything special other than place the Manifest file and run the migration. The custom block was in the header.

1. Installed D6
2. Added custom block to header with some text (via the interface.)
3. Ran the migration (with the block Manifest file.)

It shows in the admin screen, but not when visiting the page. I can remove the block and add it back and then it shows on the page.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

PHP 5.5.17

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

Confirmed that this is an issue - digging into it now.

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Active » Needs review
FileSize
2410623-9.patch1.07 KB

The issue was related to page visibility settings. Where there are none, the visibility array was still being populated, when it shouldn't. Patch attached.

-mike

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

That fixes the problem. Maybe we can add a test? It looks like one of the custom blocks doesn't have a visibility setting. Why did tests not fail?

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

Yeah, I was thinking about that last night after I posted the patch. The tests didn't fail because the assertions are only checking to see if the listed "pages" (from the visibility settings) are the same.

I'll take another look at it later today and see if I can come up with better tests.

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs review » Needs work

I'm going to look to improve the tests before this is ready for review.

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review
FileSize
2410623-13.patch400.48 KB
2410623-13-REVIEW.patch6.54 KB

I've added a new "page specific visibility settings" test and improved the existing ones, in addition to the fix to the BlockVisibility process plugin. The addition of the new test required an updated d6.gz dump file.

There are two patches attached - the one marked "-REVIEW" includes everything except for the changes to the d6.gz file.

Thanks,
-mike

Status: Needs review » Needs work

The last submitted patch, 13: 2410623-13-REVIEW.patch, failed testing.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review
FileSize
2410623-13-REVIEW-do-not-test.patch6.54 KB

Heh - just read the fine print on how to add a patch that the testbot will ignore. Attaching renamed patch.

-mike

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

Do you need to do something special for the binary file? Patch applied for me locally. I got some warning about whitespace, but the tests ran.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented 
+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateBlockTest.php
@@ -93,7 +93,7 @@ public function testBlockMigration() {
-    $this->assertTrue(empty($visibility['request_path']['pages']));
+    $this->assertTrue(empty($visibility));

So, I ran the test without the change in BlockVisibility.php to see the expected fails There were 5: the 2 user blocks, menu blocks, and block_2, and block_3. Seems right since those are the ones with no custom visibility.

Not required, but consider doing something like this:
$this->assertTrue(empty($visibility), 'Visibility settings.');
or you get "Value false is TRUE." which doesn't tell us what failed.

Tests work for me locally.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
FileSize
2410623-18.patch400.48 KB

Reuploading the patch in hopes that it gets tested this time. Note that this is the exact same patch at 13, just a new file name.

@lokapujya - I didn't update the assertions with the extra text - none of the other tests include this help text, and it is easy enough to find the issue from the line number. Thanks for testing it though!

Thanks,
-mike

Status: Needs review » Needs work

The last submitted patch, 18: 2410623-18.patch, failed testing.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review
FileSize
2410623-20.patch400.81 KB

Thanks @chx for pointing me to http://mediatribe.net/en/node/63 for instructions on how to create a patch with a binary file in it. Updated patch attached.

Thanks,
-mike

Status: Needs review » Needs work

The last submitted patch, 20: 2410623-20.patch, failed testing.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Version: 8.0.0-beta4 » 8.0.x-dev
chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review
FileSize
2410623_23.patch496.46 KB

You can't roll a binary patch since the core .gitattributes includes *.gz -text diff enforcing a normal diff. I added a .gitattributes to the Tests directory overriding this.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs review » Needs work

Probably needs a re-roll now that #2415399 is committed.

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review
FileSize
2410623-25.patch457.39 KB
2410623-25-REVIEW-do-not-test.patch8.93 KB

Re-rolled patch attached - some notes:

  1. The 2410623-25-REVIEW-do-not-test.patch is identical to 2410623-25.patch minus the d6.gz stuff.
  2. This patch contains a .gitattributes file for core/modules/migrate_drupal/src/Tests which helps with the d6.gz file being properly formatted in the patch.
  3. This patch also contains a fix for a small regression from #2406749: Use a link field for custom menu link where the author of that patch didn't take into account that core/modules/migrate_drupal/src/Tests/Table/MenuLinks.php is a generated file. The author manually (and incorrectly) updated the table dump and corresponding MigrateMenuLinkTest.

-mike

benjy’s picture

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

This patch also contains a fix for a small regression from #2406749

O dear, we really need committers to be aware of the Table's folder not been manually editable to stop this happening. I'm not sure what else we can do other than the big comment we already have at the top of the file?

Rest looks fine, RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Migrate is not blocked by beta. Committed 9dd8de4 and pushed to 8.0.x. Thanks!

  • alexpott committed 9dd8de4 on 8.0.x 
    Issue #2410623 by ultimike, chx: D6-D8 Custom Block Migration not...

Status: Fixed » Closed (fixed)

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