Issue #2569703 by phenaproxima, benjy, webchick: When migrating to the same theme with different region names, block will not migrate properly

core/modules/block/src/Plugin/migrate/process/BlockRegion.php

@@ -7,30 +7,68 @@
 
 namespace Drupal\block\Plugin\migrate\process;
 
+use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Drupal\migrate\MigrateExecutableInterface;
 use Drupal\migrate\ProcessPluginBase;
 use Drupal\migrate\Row;
+use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
  * @MigrateProcessPlugin(
  *   id = "block_region"
  * )
  */
-class BlockRegion extends ProcessPluginBase {
+class BlockRegion extends ProcessPluginBase implements ContainerFactoryPluginInterface {
+
+  /**
+   * List of regions, keyed by theme.
+   *
+   * @var array[]
+   */
+  protected $regions;
+
+  /**
+   * Constructs a BlockRegion plugin instance.
+   *
+   * @param array $configuration
+   *   The plugin configuration.
+   * @param string $plugin_id
+   *   The plugin ID.
+   * @param mixed $plugin_definition
+   *   The plugin definition.
+   * @param array $regions
+   *   Array of region maps, keyed by theme.
+   */
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, array $regions) {
+    parent::__construct($configuration, $plugin_id, $plugin_definition);
+    $this->regions = $regions;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
+    $regions = array();
+    foreach ($container->get('theme_handler')->listInfo() as $key => $theme) {
+      $regions[$key] = $theme->info['regions'];
+    }
+    return new static($configuration, $plugin_id, $plugin_definition, $regions);
+  }
 
   /**
    * {@inheritdoc}
-   *
-   * Set the destination block region, based on the source region and theme as
-   * well as the current destination default theme.
    */
   public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
+    // Set the destination region, based on the source region and theme as well
+    // as the current destination default theme.
     list($region, $source_theme, $destination_theme) = $value;
 
-    // Theme is the same on both source and destination, we will assume they
-    // have the same regions.
+    // Theme is the same on both source and destination, so ensure that the
+    // region exists in the destination theme.
     if (strtolower($source_theme) == strtolower($destination_theme)) {
-      return $region;
+      if (isset($this->regions[$destination_theme][$region])) {
+        return $region;
+      }
     }
 
     // If the source and destination theme are different, try to use the

core/modules/block/src/Tests/Migrate/d6/MigrateBlockTest.php

@@ -154,7 +154,7 @@ class MigrateBlockTest extends MigrateDrupal6TestBase {
     $visibility['request_path']['id'] = 'request_path';
     $visibility['request_path']['negate'] = FALSE;
     $visibility['request_path']['pages'] = '/node';
-    $this->assertEntity('block_1', $visibility, 'right', 'bluemarine', -4);
+    $this->assertEntity('block_1', $visibility, 'sidebar_second', 'bluemarine', -4);
 
     $visibility = [];
     $this->assertEntity('block_2', $visibility, 'right', 'test_theme', -7);

core/modules/block/src/Tests/Migrate/d7/MigrateBlockTest.php

@@ -23,7 +23,7 @@ class MigrateBlockTest extends MigrateDrupal7TestBase {
    *
    * @var array
    */
-  static $modules = array(
+  static $modules = [
     'block',
     'views',
     'comment',
@@ -33,7 +33,7 @@ class MigrateBlockTest extends MigrateDrupal7TestBase {
     'text',
     'filter',
     'user',
-  );
+  ];
 
   /**
    * {@inheritdoc}
@@ -50,7 +50,7 @@ class MigrateBlockTest extends MigrateDrupal7TestBase {
     $config->save();
 
     // Install one of D8's test themes.
-    \Drupal::service('theme_handler')->install(array('test_theme'));
+    \Drupal::service('theme_handler')->install(['bartik']);
 
     $this->executeMigration('d7_filter_format');
     $this->executeMigration('d7_user_role');
@@ -117,7 +117,7 @@ class MigrateBlockTest extends MigrateDrupal7TestBase {
 
     // Assert that disabled blocks (or enabled blocks whose plugin IDs could
     // be resolved) did not migrate.
-    $non_existent_blocks = array(
+    $non_existent_blocks = [
       'bartik_system_navigation',
       'bartik_system_help',
       'seven_user_new',
@@ -155,7 +155,7 @@ class MigrateBlockTest extends MigrateDrupal7TestBase {
       'seven_menu_menu-test-menu',
       'seven_statistics_popular',
       'seven_block_1',
-    );
+    ];
     $this->assertTrue(empty(Block::loadMultiple($non_existent_blocks)));
   }
 

core/modules/block/tests/src/Unit/Plugin/migrate/process/BlockRegionTest.php

@@ -0,0 +1,69 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Tests\block\Unit\Plugin\migrate\process\BlockRegionTest.
+ */
+
+namespace Drupal\Tests\block\Unit\Plugin\migrate\process;
+
+use Drupal\block\Plugin\migrate\process\BlockRegion;
+use Drupal\migrate\MigrateExecutableInterface;
+use Drupal\migrate\Row;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * @coversDefaultClass \Drupal\block\Plugin\migrate\process\BlockRegion
+ * @group block
+ */
+class BlockRegionTest extends UnitTestCase {
+
+  /**
+   * Transforms a value through the block_region plugin.
+   *
+   * @param array $value
+   *   The value to transform.
+   * @param \Drupal\migrate\Row|NULL $row
+   *   (optional) The mocked row.
+   *
+   * @return array|string
+   *   The transformed value.
+   */
+  protected function transform(array $value, Row $row = NULL) {
+    $executable = $this->prophesize(MigrateExecutableInterface::class)->reveal();
+    if (empty($row)) {
+      $row = $this->prophesize(Row::class)->reveal();
+    }
+
+    $regions = array(
+      'bartik' => array(
+        'triptych_first' => 'Triptych first',
+        'triptych_second' => 'Triptych second',
+        'triptych_third' => 'Triptych third',
+      ),
+    );
+    $plugin = new BlockRegion(['region_map' => []], 'block_region', [], $regions);
+    return $plugin->transform($value, $executable, $row, 'foo');
+  }
+
+  /**
+   * If the source and destination themes are identical, the region should only
+   * be passed through if it actually exists in the destination theme.
+   *
+   * @covers ::transform
+   */
+  public function testTransformSameThemeRegionExists() {
+    $this->assertSame('triptych_second', $this->transform(['triptych_second', 'bartik', 'bartik']));
+  }
+
+  /**
+   * If the source and destination themes are identical, the region should be
+   * changed to 'content' if it doesn't exist in the destination theme.
+   *
+   * @covers ::transform
+   */
+  public function testTransformSameThemeRegionNotExists() {
+    $this->assertSame('content', $this->transform(['footer', 'bartik', 'bartik']));
+  }
+
+}