When migrating blocks, the BlockRegion process plugin, given the source theme name, attempts to map the block to the same region in the destination theme. If the source and destination theme are the same, the region is unchanged; otherwise, the plugin falls back on a static map provided to it in configuration. The problem is, the static map is not theme-aware. It would be better if it would look something like this:

region_map:
  source_theme1:
    region1: destination_region1
    region2: destination_region2
  source_theme2:
    region3: destination_region1
    region4: destination_region3

...and so forth. This would make the BlockRegion plugin more powerful and capable of handling more complex block migration scenarios. It'd also be a pretty easy change, although it could break backwards compatibility depending on how we implement it.

CommentFileSizeAuthor
#28 interdiff-2753939-15-27-8.2.x.txt5.12 KBphenaproxima
#27 2753939-27-8.2.x.patch12.67 KBphenaproxima
#23 interdiff-2753939-19-23.txt3.06 KBphenaproxima
#23 2753939-23.patch12.7 KBphenaproxima
#19 2753939-19.patch12.63 KBphenaproxima
#15 interdiff-2753939-9-15.txt805 bytesphenaproxima
#15 2753939-15.patch9.55 KBphenaproxima
#9 2753939-9.patch9.29 KBphenaproxima
#5 2753939-5.patch2.71 KBphenaproxima
#2 2753939.2.patch1.32 KBalexpott

Comments

phenaproxima created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2753939.2.patch1.32 KB

Well actually block region migration is fundamentally broken...

Status: Needs review » Needs work

The last submitted patch, 2: 2753939.2.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: +Migrate BC break

This will break Migrate BC, but that's OK.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2753939-5.patch2.71 KB

Okay, let's try this approach on for size. I fully expect this to break several Migrate tests.

This patch changes BlockRegion so that it extends StaticMap. That way, if the block cannot be migrated directly from its source region to a destination region, it falls back onto using the static mapping functionality. This means you can map the regions based on what the source theme is, or what the source and destination themes are. And if the bypass flag is not set, it throws MigrateSkipRowException -- exactly as StaticMap would. IMHO this approach is ten times more flexible than BlockRegion previously was -- keeping all the benefits, and shedding the flaws.

Status: Needs review » Needs work

The last submitted patch, 5: 2753939-5.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Version: 8.3.x-dev » 8.2.x-dev

Is this a BC break? If so, we should try to get it into 8.2.x.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2753939-9.patch9.29 KB

This patch tries to make the improvement, touching as few things as possible. Truth is, block migration needs a lot of work -- it's really janky, rickety, and prone to strange behavior because it's concerned with migrating the legacy data with as few changes as possible, rather than migrating things in a way that will make sense when the user looks at their migrated site. Nonetheless, the improvements I'd like to make are well outside of the scope of this issue.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue tags: +Needs change record

Looks good to me, but I'd like someone with a better knowledge of block/theme layout to also look at it for RTBC.

There should be a change record for the block_region process plugin.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: -Needs change record

Change record written!

benjy’s picture

benjy commented

The patch looks good. Worth noting that we seem to have a split in core where sometimes we setup the map in the YAML as a pipeline when other times we extend StaticMap and make our plugin use the parent as the fallback.

+++ b/core/modules/block/src/Plugin/migrate/process/BlockRegion.php
--- a/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php

+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
@@ -38,7 +38,7 @@ protected function setUp() {
+    \Drupal::service('theme_handler')->install(['bartik', 'test_theme']);

Do we need to call block_rebuild() in this test? That's what alexpott was doing in #2 for the failing test.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Generally I'm in favor of extending StaticMap. In my experience, these specialized mapping plugins (BlockRegion and BlockPluginId leap to mind) essentially depend on static mapping, or are designed to fall back on to it (i.e., they more or less expect to precede or succeed a StaticMap instance in the same processing pipeline). Point is, they're not really meant to work independently of a static map; therefore, in those cases I prefer to cut out the middleman and simply make them extend StaticMap.

EDIT: I'll add a call to block_rebuild() to see what, if any, effect it has on the test. It's quite possible that a lot of things will break, because there are many more problems with block migration than just this snafu with the region mapping. But, fixing all block migrations is out of scope for this issue.

benjy’s picture

benjy commented

Yeah, lets add block_rebuild() call even if we don't fix it here at least we can open more follow-ups. After that, i'm +1 for RTBC.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 2753939-15.patch9.55 KB
new interdiff-2753939-9-15.txt805 bytes

Added block_rebuild() calls to the D6 and D7 versions of MigrateBlockTest. They both pass on localhost, so fingers crossed.

Will kick this issue to RTBC if it passed Drupal CI.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Great success! RTBC per #10 and #14.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll, +rc target

This patch needs a reroll and as a migrate BC break I think we should endeavour to fix this during the 8.2.0 RC window.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2753939-19.patch12.63 KB

So the patch in #15 still applies to 8.2.x...this patch is a re-roll against 8.3.x. No interdiff due to rejected hunks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
@@ -70,18 +71,17 @@ protected function setUp() {
-    $this->assertIdentical($status, $block->status());

+++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php
@@ -78,29 +79,28 @@ protected function setUp() {
-    $this->assertIdentical($status, $block->status());

Why are we removing these status checks? Surely we should be checking the status after the migration?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

EDIT: Never mind.

I had originally posted a hissy fit here, but having researched things a bit I see what's going on. There is a bit of a "conflict of intent" going on between the MigrateBlockTest classes in 8.3.x and the changes in #2513534: Remove the 'disabled' region from Block UI. If nothing else, they need to be better documented. I will post an updated patch.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

git archaeology has revealed that #2513534: Remove the 'disabled' region from Block UI introduced changes that require this patch not simply to be rerolled, but retooled for 8.3.x.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2753939-23.patch12.7 KB
new interdiff-2753939-19-23.txt3.06 KB

Restored the status assertions and added a touch of documentation explaining why we are asserting that.

We are going to need a follow-up issue to address the fact that both the d6_block and d7_block migrations are skipping over blocks that are disabled in the source database -- now that block status is meaningful in 8.3.x, it no longer makes sense to skip those blocks. This is good, since it means we can migrate more data coherently.

Truth is, block migration is in fairly rough shape, because the migrations (and especially the tests) are ensuring that we migrate the block data verbatim, regardless of whether or not it's coherent. In Dublin and beyond, I intend to do a lot of work to bring block migration up to par.

In the meantime, here is a new patch for 8.3.x that makes the status assertion official.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Kickin' it RTBC-style now that everything is passing on both branches.

alexpott’s picture

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

Committed 8d0d69f and pushed to 8.3.x. Thanks!

We now need a reroll of #23 for 8.2.x

  • alexpott committed 8d0d69f on 8.3.x 
    Issue #2753939 by phenaproxima, alexpott: BlockRegion process plugin...
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2753939-27-8.2.x.patch12.67 KB

Rerolled against 8.2.x.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new interdiff-2753939-15-27-8.2.x.txt5.12 KB

Just realized that I can, and therefore should, post an interdiff.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Now that the reroll has passed Drupal CI, 'tis back to RTBC.

alexpott’s picture

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

Committed 9f73d72 and pushed to 8.2.x. Thanks!

  • alexpott committed 9f73d72 on 8.2.x 
    Issue #2753939 by phenaproxima, alexpott: BlockRegion process plugin...

Status: Fixed » Closed (fixed)

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