D6->D8 Blocks - Custom titles not imported

Role-based visibility settings is fixed with #2415399: D6->D8 migration: User role based block visibility settings not migrated properly.

I confirmed with a manual test that the D6 "Block title" isn't properly migrated to D8. The D6 "Block description" is copied (incorrectly) into the D8 title field, and while the D8 "Display title" checkbox is checked, the title isn't actually displayed. When I manually re-saved the block configuration (with no changes), the (incorrect) block title is displayed.

I'll go ahead and start working on a patch to:

1. Properly migrate the block title.
2. Set the D8 "Display title" checkbox properly depending on the incoming value of the D6 "Block title". I'll need to check to make sure that both custom and module-based block titles are handled properly.

-mike

Patch attached.

-mike

This looks good to me, how about a failing version of the patch to prove the tests work?

Good idea. Fail patch attached.

-mike

The failed patch was purposely created to fail, moving to needs review.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockSettings.php
@@ -24,8 +24,11 @@ class BlockSettings extends ProcessPluginBase {
+    if ($title != '<none>') {
+      $settings['label'] = $title;
+    }

This implies that $settings['title'] isn't set at all if title is < none>. Is that right?

Good catch. Actually, I totally missed the D8 block $settings['label_display'] setting. Updated patch includes support for this setting.

Also - I noticed that my re-roll of #2410623: D6-D8 Custom Block Migration not visible in region failed to include core/modules/migrate_drupal/src/Tests/.gitattributes, so I've added it to this patch as well.

-mike

Re-roll due to #2416111: Migrate Dump export needs to have a consistent order and #2413759: Move D6 dumps to avoid collisions with D7 dumps.

-mike

Setting to "Needs review" as the FAIL test was meant to fail.

-mike

Didn't get to do a full functional review, but it looks like one of your new lines in MigrateBlockTest.php got appended onto an existing line (for all three versions). I've new versions with that fixed.

Hmm let me try that again.

@mrjmd - good find, thanks! Not sure why your patch wasn't applying - did you create it with git diff --full-index --binary > filename.patch?

Patch updated.

-mike

Looks good.

+1 - Works really well.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockSettings.php
   @@ -24,8 +24,16 @@ class BlockSettings extends ProcessPluginBase {
   +      $settings['label_display'] = 'visible';
   

   There's a constant for this. See BLOCK_LABEL_VISIBLE

2. +++ b/core/modules/migrate_drupal/src/Tests/.gitattributes
   @@ -0,0 +1 @@
   +d6.gz -text -diff
   \ No newline at end of file
   

   New line needed

Seems to fix the concerns from #26, thanks.

Here's a straight reroll of #27. There was a minor conflict in BlockSettings.php caused by the switch to MigrateExecutableInterface, pretty sure I've resolved it properly.

Can we get an interdiff?

Interdiff.

Thanks, still RTBC for me

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockSettings.php
@@ -24,8 +25,16 @@ class BlockSettings extends ProcessPluginBase {
diff --git a/core/modules/migrate_drupal/src/Tests/.gitattributes b/core/modules/migrate_drupal/src/Tests/.gitattributes

diff --git a/core/modules/migrate_drupal/src/Tests/.gitattributes b/core/modules/migrate_drupal/src/Tests/.gitattributes
new file mode 100644

new file mode 100644
index 0000000000000000000000000000000000000000..2423bbb9253483cf409d2b03e81f0796c3af3fe1

index 0000000000000000000000000000000000000000..2423bbb9253483cf409d2b03e81f0796c3af3fe1
--- /dev/null

--- /dev/null
+++ b/core/modules/migrate_drupal/src/Tests/.gitattributes

+++ b/core/modules/migrate_drupal/src/Tests/.gitattributes
+++ b/core/modules/migrate_drupal/src/Tests/.gitattributes
@@ -0,0 +1 @@

@@ -0,0 +1 @@
+d6.gz -text -diff

Adding this means you can no longer produce migrate patches just using git diff without doing git diff --binary. This should be discussed in a different issue.

I think that we only need to use git diff --binary when there is a change to a test that involved a change to the source DB (d6.gz in this case), this has been the case since #2348875: Improving our dump files.

I guess I'm not clear on why that stops this patch from getting committed?

On a related note, I just created #2452709: migrate_drupal test dump generation documentation to improve the dump generation documentation.

-mike

Can you just switch from storing the file as a .gz and instead use a .sql?

Discussing the .gz issue over at #2469623: Process for creating migration source DBs for automated tests.

-mike

Waiting for #2281393: D6->D8 Blocks - Custom titles not imported.

-mike

trying a re-roll, using the now RTBC'd #2469623: Process for creating migration source DBs for automated tests. not sure about this diff yet, since this is a new process for me.

correcting for block id (bid) 8 and so we can diff patches in this new process.

This is a secondary diff correcting block id 8.

worked with @vendion to get this new process working. this patch is from both of us and is the re-roll of #30, using the script generated dump from #2469623: Process for creating migration source DBs for automated tests.

guessing this failed, because #2469623: Process for creating migration source DBs for automated tests is not yet in.

Reroll. The patch is much smalled because the patch in #42 had lots of changes to unrelated dump tables.

Rerolled.

• And added tests for the label and the label display.
• Changed the process plugin, BlockSettings, to check for and empty title column, instead on 'none' as the patch had. Both D6 and D7 have an empty field when there is not title.
• Made the same change to d7_block.yml.

Tested locally and worked fine.

Looks good to me.

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

this hunk seems to have been forgotten
D6's <none> labeled blocks must not have their label shown in D8

ping for reopen @catch