The Drupal 7 ThemeSettings source plugin does not check that the destination site has a valid theme to migrate settings into

Discovered an edge case:
An installed Drupal (7) theme not necessarily has a theme settings variable. The settings variable is created only when the theme settings form has submitted.

We also have to make sure that the unnecessary block configs (the blocks that are used by the not migrated theme) aren't migrated to the destination.

Attached a test-only patch that updates the drupal7 database fixture with an enabled Garland theme (and with its block configs), and adds the theme_garland_settings variable as well.

1. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -1811,6 +1811,381 @@
   +->values(array(
   +  'bid' => '51',
   ...
   +->values(array(
   +  'bid' => '52',
   

   🤔 This adding blocks? Why?

2. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -56101,6 +56476,10 @@
   +->values(array(
   +  'name' => 'theme_garland_settings',
   

   👍 These are the theme settings for garland, great!

3. +++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateThemeSettingsTest.php
   @@ -11,6 +11,16 @@
   +    // to test that 'garland.settings' isn't exist, and not whether it has the
   

   🤓 "isn't" → "doesn't"

Re #5:

#5.1:
This happens if you enable a theme in Drupal7 (and even in Drupal8, but there only some relevant block plugins get a block config entity).
Initially I enabled the theme with drush, but after you asked this question I re-rested this and enabled the theme by using only the Drupal 7 appearance UI – and I got the same result.

#5.2
Yepp, this is what we don't want to be imported.

#5.3
😶I'll fix this.

One more failing test expected: Drupal\Tests\block\Kernel\Migrate\d7\MigrateBlockTest.
Garland blocks shouldn't been migrated.

Adding an initial fix.

1. +++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php
   @@ -124,6 +128,29 @@ public function testBlockMigration() {
   +    $this->assertEquals([], $missing_block_ids, 'There are missing block configuration entities.');
   +    $this->assertEquals([], $unexpected_block_ids, 'There are unexpected block configuration entities that should not exist.');
   

   🤓 In the past we used to specify that third parameter with a helpful message. But ever since Drupal migrated from its own SimpleTest infrastructure onto PHPUnit, we've been waning ourselves off of that optional message. Why? Because if you specify an optional message, you don't get actionable test failures: you get *that* error message that you specified. If you don't specify an error message, PHPUnit will automatically show the difference in the two arrays.

   So, we should remove that optional message.

2. +++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php
   @@ -164,6 +191,12 @@ public function testBlockMigration() {
   +      // Garland blocks shouldn't been migrated.
   

   🤓 "shouldn't been migrated" → "shouldn't have been migrated"

3. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -1811,6 +1811,756 @@
   +->values(array(
   +  'bid' => '51',
   +  'module' => 'system',
   +  'delta' => 'main',
   +  'theme' => 'garland',
   ...
   +->values(array(
   +  'bid' => '52',
   +  'module' => 'search',
   +  'delta' => 'form',
   +  'theme' => 'garland',
   

   +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -56101,6 +56851,10 @@
   +->values(array(
   +  'name' => 'theme_garland_settings',
   +  'value' => 'a:15:{s:11:"toggle_logo";i:1;s:11:"toggle_name";i:1;s:13:"toggle_slogan";i:1;s:24:"toggle_node_user_picture";i:1;s:27:"toggle_comment_user_picture";i:0;s:32:"toggle_comment_user_verification";i:1;s:14:"toggle_favicon";i:1;s:16:"toggle_main_menu";i:1;s:21:"toggle_secondary_menu";i:0;s:12:"default_logo";i:1;s:9:"logo_path";s:0:"";s:11:"logo_upload";s:0:"";s:15:"default_favicon";i:1;s:12:"favicon_path";s:0:"";s:14:"favicon_upload";s:0:"";}',
   +))
   

   👍 This is expanding the D7 fixture to place blocks for the Garland theme and configure theme settings. Goal: to test that a source-only theme does not get its settings migrated into the destination (theme settings + blocks).

   🙏 Can you confirm this interpretation is correct?

4. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -1811,6 +1811,756 @@
   +->values(array(
   +  'bid' => '76',
   +  'module' => 'system',
   +  'delta' => 'main',
   +  'theme' => 'stark',
   ...
   +->values(array(
   +  'bid' => '77',
   +  'module' => 'search',
   +  'delta' => 'form',
   +  'theme' => 'stark',
   

   👍 This is placing only blocks specific to the Stark theme. Goal: test that a theme that exists in both source and destination but does not have settings does get migrated to D8, but the destination theme settings will be based on the global theme settings, thereby proving that this architectural difference between D7 and D8/D9 is respected correctly.

   🙏 Can you confirm this interpretation is correct?

6. +++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateThemeSettingsTest.php
   @@ -11,14 +11,29 @@
   +  protected static $configSchemaCheckerExclusions = [
   +    // Garland may have settings if the theme settings migration fails. We want
   +    // to test that 'garland.settings' doesn't exist, and not whether it has the
   +    // proper schema.
   +    'garland.settings',
   +  ];
   

   👍 Thanks for this great comment — otherwise I wouldn't have grokked it :)

7. +++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateThemeSettingsTest.php
   @@ -52,6 +67,26 @@ public function testMigrateThemeSettings() {
   +    $this->assertFalse($config->isNew(), 'Stark settings migration was skipped.');
   

   👍 Here you specify an optional failure message too, but in this case there are only two possible values so you're not masking anything, plus the comment truly makes it easier to understand what you're testing here.

8. +++ b/core/modules/system/tests/src/Kernel/Plugin/migrate/source/d7/ThemeSettingsTest.php
   @@ -54,7 +70,7 @@ public function providerSource() {
   -        'name' => 'theme_bartik_settings',
   +        'name' => 'bartik',
   

   🤔 I don't understand the reason for this change. Could you explain that here on the issue?

9. +++ b/core/modules/block/src/Plugin/migrate/source/d7/Block.php
   @@ -0,0 +1,82 @@
   +   * @param \Drupal\Core\State\ThemeHandlerInterface $theme_handler
   +   *   The state storage object.
   

   🐛 The FQCN + comment are wrong.

10. +++ b/core/modules/block/src/Plugin/migrate/source/d7/Block.php
    @@ -0,0 +1,82 @@
    +    // Blocks that's target theme is not available shouldn't been migrated.
    

    🐛 Blocks whose target theme is not available should not be migrated.

11. +++ b/core/modules/system/migrations/d7_theme_settings.yml
    @@ -10,19 +10,11 @@ source:
    -  theme_name:
    -    -
    -      plugin: explode
    -      source: name
    -      delimiter: _
    -    -
    -      plugin: extract
    -      index:
    -        - 1
    +  name: name
       configuration_name:
         plugin: concat
         source:
    -      - '@theme_name'
    +      - '@name'
    

    🥳 Nice simplification!

    🤔 Just to be clear: this is not an essential change, right?

This definitely has tests now!

@Wim Leers, I'm still working on this, it seems that we can simplify the theme_settings migration with a deriver, and I also noticed few more things:

• I can simplify the block config migration even with the 'skip_on_empty' process plugin, without adding new classes for D7 and without modifying the source plugin.
• I have to 'fix' the D6 block config migration as well

Updated tests and assertions even for Drupal 6 block migration.

Explanation will follow soon.

Explaining the test changes

1. --- a/core/modules/block/migrations/d6_block.yml
   +++ b/core/modules/block/migrations/d6_block.yml
   
   @@ -12,15 +12,28 @@ process:
      id:
   -    # We need something unique, so aggregator, aggregator_1 etc will do.
   -    plugin: make_unique_entity_field
   -
   -    entity_type: block
   -    field: id
   -    postfix: _
   -    length: 29
   -    source: module
   +    # We need something unique prefixed with the target theme name (especially
   +    # for testing purposes), so we will generate ids like bartik_aggregator,
   +    # bartik_aggregator_1, test_theme_block_1 etc.
   +    -
   +      plugin: concat
   +      source:
   +        - '@theme'
   +        - module
   +      delimiter: _
   +    -
   +      plugin: make_unique_entity_field
   +      entity_type: block
   +      field: id
   +      postfix: _
   +      length: 29
   

   We shouldn't migrate block configs whose theme dependency is unavailable. To be able to test this somehow (the single bluemarine block in the Drupal 6 fixture should be skipped), I changed the way how the pre-existing block config ids are calculated.

   This means that from this point, block configurations which are migrated from Drupal 6 will get an id that is prefixed with the target theme's machine name.

   Migrate maintainers, please examine whether this change is acceptable or not

2. --- a/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   
   @@ -96,7 +96,30 @@ public function assertEntity($id, $visibility, $region, $theme, $weight, array $
      public function testBlockMigration() {
        $blocks = Block::loadMultiple();
   -    $this->assertCount(14, $blocks);
   +    $this->assertCount(13, $blocks);
   
   @@ -268,27 +291,13 @@ public function testBlockMigration() {
   -    // We expect this block to be disabled because '' is not a valid region,
   -    // and block_rebuild() will disable any block in an invalid region.
   -    $this->assertEntity('block_1', $visibility, '', 'bluemarine', -4, $settings, FALSE);
   +    // The Bluemarine theme does not exist in Drupal 8, and it isn't set as an
   +    // admin nor as a default theme in the Drupal 6 source database, so it
   +    // shouldn't been migrated.
   +    $block = Block::load('bluemarine_block');
   +    $this->assertNull($block);
   

   This is related to the point above: the bluemarine block shouldn't have been migrated.

3. --- a/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockContentTranslationTest.php
   +++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockContentTranslationTest.php
   @@ -44,6 +44,8 @@ protected function setUp() {
    
   +    $this->container->get('theme_installer')->install(['bartik']);
   +
   

   For testing the Block content translation, the block config's target theme should be installed.

4. --- a/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -1811,6 +1811,756 @@
   +->values(array(
   +  'bid' => '51',
   +  'module' => 'system',
   +  'delta' => 'main',
   +  'theme' => 'garland',
   +  'status' => '1',
   +  'weight' => '0',
   ...
   +->values(array(
   +  'bid' => '100',
   +  'module' => 'locale',
   +  'delta' => 'language_content',
   +  'theme' => 'stark',
   +  'status' => '0',
   ...
   +))
   

   Re #14.3; #14.3:

   Exactly! These are the block system related changes made by Drupal 7 after we enabled Garland and Stark themes.

5. --- a/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -31114,7 +31864,7 @@
   -  'access_arguments' => '...',
   +  'access_arguments' => '...',
   
   @@ -31189,7 +31939,7 @@
   -  'access_arguments' => '...',
   +  'access_arguments' => '...',
   
   @@ -36489,7 +37239,7 @@
   -  'access_arguments' => '...',
   +  'access_arguments' => '...',
   
   @@ -36539,7 +37289,7 @@
   -  'access_arguments' => '...',
   +  'access_arguments' => '...',
   
   @@ -36589,7 +37339,7 @@
   -  'access_arguments' => '...',
   +  'access_arguments' => '...',
   
   @@ -36689,7 +37439,7 @@
   -  'access_arguments' => '...',
   +  'access_arguments' => '...',
   

   Pages that got accessible after Garland and Stark were enabled. (A single status key in the serialized access_arguments changed from 0 to 1).

6. --- a/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -56101,6 +56851,10 @@
   +->values(array(
   +  'name' => 'theme_garland_settings',
   +  'value' => '...',
   +))
   

   This is also a test-related fixture: We only saved the theme settings form for Garland, so we will only have theme_garland_settings.

   Stark won't have specific configuration, and inherits the global theme_settings.

About the fix

1. --- a/core/modules/block/migrations/d6_block.yml
   +++ b/core/modules/block/migrations/d6_block.yml
   
   @@ -13,11 +13,15 @@ process:
   +    -
   +      plugin: skip_on_empty
   +      method: row
   
   --- a/core/modules/block/migrations/d7_block.yml
   +++ b/core/modules/block/migrations/d7_block.yml
   
   @@ -60,11 +60,15 @@ process:
   +    -
   +      plugin: skip_on_empty
   +      method: row
   
   --- a/core/modules/block/src/Plugin/migrate/process/BlockTheme.php
   +++ b/core/modules/block/src/Plugin/migrate/process/BlockTheme.php
   
   @@ -90,8 +90,11 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   -    // We couldn't map it to a D8 theme so just return the incoming theme.
   -    return $theme;
   +    // We couldn't map it to a D8 theme. Since a block configuration entity
   +    // depends on an enabled theme, we have to skip every block that doesn't
   +    // have a Drupal 8 theme. We return NULL here, and with the skip_on_empty
   +    // plugin, we will skip the entire row.
   +    return NULL;
   

   This is how the blocks without the required theme dependency will be skipped; even for Drupal 6 and Drupal 7 migrations.

2. --- a/core/modules/system/migrations/d7_theme_settings.yml
   +++ b/core/modules/system/migrations/d7_theme_settings.yml
   
   @@ -3,27 +3,10 @@ label: D7 theme settings
   +deriver: \Drupal\system\Plugin\migrate\D7ThemeDeriver
   ...
   -  constants:
   -    config_suffix: '.settings'
    process:
   -  # Build the configuration name from the variable name, i.e.
   -  # theme_bartik_settings becomes bartik.settings.
   -  theme_name:
   ...
   -  configuration_name:
   

   I added a D7ThemeDeriver deriver class for the theme migrations. It makes possible calculating the theme_name and config names both in the source plugin and in the destination plugin as well.

3. +++ b/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
   @@ -62,11 +62,12 @@ public static function create(ContainerInterface $container, array $configuratio
   -    $config = $this->configFactory->getEditable($row->getDestinationProperty('configuration_name'));
   +    $config_name = "{$this->configuration['theme']}.settings";
   +    $config = $this->configFactory->getEditable($config_name);
   ...
        // Remove keys not in theme settings.
        unset($theme_settings['configuration_name']);
   -    unset($theme_settings['theme_name']);
   +    unset($theme_settings['name']);
   

   Ooops... Well, we don't have configuration_name nor theme_name or name in the row... I have to remove these unnecessary unset()s asap.

4. --- a/core/modules/system/src/Plugin/migrate/source/d7/ThemeSettings.php
   +++ b/core/modules/system/src/Plugin/migrate/source/d7/ThemeSettings.php
   
   @@ -18,9 +18,27 @@ class ThemeSettings extends VariableMultiRow {
      public function query() {
   -    return $this->select('variable', 'v')
   +    $theme_name = $this->configuration['theme'];
   +
   +    $theme_specific_settings_query = $this->select('variable', 'v')
          ->fields('v', ['name', 'value'])
   -      ->condition('name', 'theme_%_settings', 'LIKE');
   +      ->condition('name', "theme_{$theme_name}_settings");
   +
   +    // D7 themes may be enabled even without a saved theme settings variable.
   +    // In this case, the theme inherits the global theme settings, which means
   +    // that we return an another query.
   +    $theme_specific_settings_count = (clone $theme_specific_settings_query)
   +      ->countQuery()
   +      ->execute()
   +      ->fetchField();
   +
   +    if (empty($theme_specific_settings_count)) {
   +      return $this->select('variable', 'v')
   +        ->fields('v', ['name', 'value'])
   +        ->condition('name', "theme_settings");
   +    }
   +
   +    return $theme_specific_settings_query;
      }
   

   I hope this change is acceptable: if the theme_stark_settings variable does not exist, the query will be made for the global theme_settings variable.

5. --- a/core/modules/system/tests/src/Kernel/Plugin/migrate/source/d7/ThemeSettingsTest.php
   +++ b/core/modules/system/tests/src/Kernel/Plugin/migrate/source/d7/ThemeSettingsTest.php
   
   +++ b/core/modules/system/tests/src/Kernel/Plugin/migrate/source/d7/ThemeSettingsTest.php
   @@ -18,6 +18,14 @@ class ThemeSettingsTest extends MigrateSqlSourceTestBase {
   +  protected function setUp() {
   +    parent::setUp();
   +    $this->container->get('theme_installer')->install(['bartik']);
   +  }
   +
   
   @@ -44,6 +52,14 @@ public function providerSource() {
   +    $tests[0]['source_data']['system'] = [
   +      [
   +        'name' => 'bartik',
   +        'type' => 'theme',
   +        'status' => '1',
   +      ],
   +    ];
   

   With this patch, the theme that we want to migrate to Drupal 8 should be enabled even on the source site and even on the destination.

1. +++ b/core/modules/block/migrations/d6_block.yml
   @@ -12,15 +12,32 @@ process:
   +  theme:
   +    -
   +      plugin: block_theme
   +      source:
   +        - theme
   +        - default_theme
   +        - admin_theme
   +    -
   +      plugin: skip_on_empty
   +      method: row
   
   @@ -56,12 +73,6 @@ process:
   -  theme:
   -    plugin: block_theme
   -    source:
   -      - theme
   -      - default_theme
   -      - admin_theme
   

   🤓 Nit: this change adds a few lines, but also moves some existing lines. AFAICT that move is not necessary. Reverting that move will minimize the changes and make this easier to land.

2. +++ b/core/modules/block/migrations/d6_block.yml
   @@ -12,15 +12,28 @@ process:
   +    # We need something unique prefixed with the target theme name (especially
   +    # for testing purposes), so we will generate ids like bartik_aggregator,
   

   Why "especially for testing purposes"? I think this is just a D8 convention?

3. #18.2:
   

   Migrate maintainers, please examine whether this change is acceptable or not

   Agreed.

   I think it is acceptable since any existing migrations continue to work as-is, they just result in different IDs for D8 block config entities. Identifiers are meant to be opaque, especially config entities' identifiers, since there is link rot risk like for content entities.

4. +++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateThemeSettingsTest.php
   -    $this->executeMigration('d7_theme_settings');
   …
   +    $this->executeMigrations(['d7_theme_settings']);
   

   🤓 This is a change that thanks to recent iterations can be reverted.

5. I hope this change is acceptable: if the theme_stark_settings variable does not exist, the query will be made for the global theme_settings variable.

   👍 Like I wrote in #14.4: this matches how the architecture of theme settings evolved in D8.

Unfortunately, the "complete" patch in #18 failed. Looks like it's mostly due to D6 expectations that have not yet been updated, so hopefully it'll be easy to get back to green, like #12 before it!

Attached the right patches (hopefully).

Re #21:

1. If I don't move that theme property process above the id's process, I cannot use the '@theme' reference (that will be the destination theme's name) – since it wont be calculated at the time the id is calculated.
2. Well, users are able to change the name for the block config entities they create. But in Drupal\Tests\block\Kernel\Migrate\d6\MigrateBlockTest::testBlockMigration, we should know what theme the (previous) block, block_1 or block_2 blocks belong to – otherwise we don't know what we are testing.
3. :)
4. This is a requirement. The latest patches added a D7ThemeDeriver for D7 theme settings migrations; and only MigrateTestBase::executeMigrations() creates the valid theme settings migrations for us. If I would use
   

   $this->executeMigration('d7_theme_settings:bartik');
   $this->executeMigration('d7_theme_settings:seven');
   $this->executeMigration('d7_theme_settings:stark');
   

   instead of

   $this->executeMigrations(['d7_theme_settings']);
   

   , I wont be able to test that the garland settings migration was skipped.

   See MigrateTestBase::executeMigrations() and MigrateTestBase::executeMigration()

#23

1. ✅ Ohhhh! Makes sense! :)
2. Hm ok. I understand what you mean, but the wording makes it sound as if this is something artificial done solely for testing, even though the same pattern actually is the default in D8. No big deal though.
3. ✅
4. ✅ Ohhhhhhhhhh wow — whodathunk that those test methods behaved vastly differently, unlike their name suggests? 🙈🙈 Obviously not your fault, and thanks for explaining that!

Patch review

1. +++ b/core/modules/system/src/Plugin/migrate/D7ThemeDeriver.php
   @@ -45,18 +47,34 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
   +      // a  Drupal source database configured – there is nothing to generate.
   

   🔎 Übernit: s/a Drupal/a Drupal/ (double space instead of single)

   Could be fixed on commit, or … could be ignored. Obviously not important.

2. +++ b/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
   @@ -61,19 +61,11 @@ public static function create(ContainerInterface $container, array $configuratio
   -    // Remove keys not in theme settings.
   -    unset($theme_settings['configuration_name']);
   -    unset($theme_settings['name']);
   

   👍 Nice cleanup!

This is completely ready now IMHO — the only thing that remains is migration system maintainer review!

Thanks @huzooka, not just for the patch, but for your persistence on this patch that turned out to be far trickier than expected :)

With this patch, blocks that were migrated from Drupal7 will get the destination theme's machine name as ID-prefix, and not the source theme.

I manually tested #27 and it works great on real-world D7 sites, but let's wait and see if core's test coverage also passes 😊

Yay, #27 is still green! 👍

+++ b/core/modules/system/src/Plugin/migrate/D7ThemeDeriver.php
--- a/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
+++ b/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php

+++ b/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
+++ b/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
@@ -61,18 +61,11 @@ public static function create(ContainerInterface $container, array $configuratio

@@ -61,18 +61,11 @@ public static function create(ContainerInterface $container, array $configuratio
    * {@inheritdoc}
    */
   public function import(Row $row, array $old_destination_id_values = []) {
-    $imported = FALSE;
-    $config = $this->configFactory->getEditable($row->getDestinationProperty('configuration_name'));
-    $theme_settings = $row->getDestination();
-    // Remove keys not in theme settings.
-    unset($theme_settings['configuration_name']);
-    unset($theme_settings['theme_name']);
-    if (isset($theme_settings)) {
-      theme_settings_convert_to_config($theme_settings, $config);
-      $config->save();
-      $imported = TRUE;
-    }
-    return $imported;
+    $config_name = "{$this->configuration['theme']}.settings";
+    $config = $this->configFactory->getEditable($config_name);
+    theme_settings_convert_to_config($row->getDestination(), $config);
+    $config->save();
+    return TRUE;

I'm a little worried about the BC implications of this. The way I read this, for older sites that don't have the updated deriver, they will fall on their faces with these changes.

The way I read this, for older sites that don't have the updated deriver, they will fall on their faces with these changes.

I wonder if this is a case of my not being awake enough yet or whether this is just me not getting it, but … how can "older sites" get the updated destination plugin but not the updated deriver? 🤔

Here's what I'm looking at... And by the way, these things can be tricky. We don't actually have a new plugin id for the destination. So we use the same destination. But because of generated yml files that get exported, one-time, into migrate_plus... we have many cases where incremental migrations will have the updated destination but not the updated yaml that gets generated by the deriver.

Ah … this is a problem for migrate_plus users … aka users of migration config entities.

Interesting. 🤔 Has every single migration plugin improvement or bugfix so far taken the consequences for migration config entities into account?

Re #33: yup. It gets tricky. We haven't always done perfectly, but we try to keep the system stable. One option is to create a new destination that and point to it in the deriver. Then deprecate the old one for removal in 10.x. That would keep BC.

#27 had to be rebased to apply to 9.0.0-beta3.

Patch re-rolled for 9.1.x

I've skimmed the issue and reviewed the patch. I didn't apply the patch or run the tests.

This needs work for #34.

1. +++ b/core/modules/block/src/Plugin/migrate/process/BlockTheme.php
   @@ -90,8 +90,11 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +    // We couldn't map it to a D8 theme. Since a block configuration entity
   +    // depends on an enabled theme, we have to skip every block that doesn't
   +    // have a Drupal 8 theme. We return NULL here, and with the skip_on_empty
   +    // plugin, we will skip the entire row.
   

   Remove all references to D8. As well as the final phrase about skip_on_empty, there is no guarantee that every process pipeline using this plugin will be followed by a skip. I'm thinking something as simple as 'No theme found for this block.'

2. +++ b/core/modules/block/src/Plugin/migrate/process/BlockTheme.php
   @@ -90,8 +90,11 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +    return NULL;
   

   The process plugin has changed but there is no change to the corresponding test. And the reason is that this process plugin has no test. So, the process plugin isn't tested directly, it gets tested during a migration test.

3. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -268,27 +291,13 @@ public function testBlockMigration() {
   +    // The Bluemarine theme does not exist in Drupal 8, and it isn't set as an
   +    // admin nor as a default theme in the Drupal 6 source database, so it
   

   Let's make this read well for Drupal 9 as well. How about changing to say that Bluemarine was removed in Drupal 8.

4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php
   @@ -72,7 +72,9 @@ protected function getEntityCounts() {
   +      // The blocks of the bluemarine and the test_theme themes should be
   +      // skipped.
   

   Not necessary. At best we track changes to the entity counts as migrations are added. There is no need to attempt to keep a history of changes in the comments.

5. +++ b/core/modules/system/migrations/d7_theme_settings.yml
   @@ -3,27 +3,10 @@ label: D7 theme settings
   diff --git a/core/modules/system/src/Plugin/migrate/D7ThemeDeriver.php b/core/modules/system/src/Plugin/migrate/D7ThemeDeriver.php
   

   This deriver needs a test.

6. +++ b/core/modules/system/src/Plugin/migrate/source/d7/ThemeSettings.php
   @@ -18,9 +18,27 @@ class ThemeSettings extends VariableMultiRow {
   

   Had to read this twice, it is an atypical use of configuration values. Can we structure this the same as d7/node.php?

7. +++ b/core/modules/system/tests/src/Kernel/Plugin/migrate/source/d7/ThemeSettingsTest.php
   @@ -18,14 +18,22 @@ class ThemeSettingsTest extends MigrateSqlSourceTestBase {
   +    $value0 = [
   
   @@ -43,11 +51,47 @@ public function providerSource() {
   +    $value1 = [
   

   Can we have more meaningful names than value0 and value1?

Rerolled patch for 9.4.x.

Rerolled patch for 9.4.x , CS fix.

Rerolled for 9.4.x , test fix.

New patch with reroll for 9.4.x

Rerolled for 9.4.x , test fix.

More info on failing test. added assertion to print blocks

Rerolled for 9.4.x , text fix , changed assertion.

9.4.x rerolled patch , assertion fix.

fixed patch.

The scope of this issue is very different from the Issue Summary. The IS needs to be updated.

The BC concerns raised in #30 need to be addressed.

#3281427: Update Block and Theme setting migrations to not use Bartik and Seven broke this in 9.5.x.

Worse, #3219539: Update Drupal 7 migration database fixture also broke this 😬

This reroll already took me well over 30 minutes and I still nowhere near done.

This is the most painful rebase I've done in many months.

This does not yet address the BC concerns in #30 that were pointed to in #59. One change caused by #3281427: Update Block and Theme setting migrations to not use Bartik and Seven was impossible to make play nice with the test coverage that this issue was adding; left a @todo there.

FILE: ...tml/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 37 | ERROR | [x] Use of annotation @inheritDoc is forbidden.
    |       |     (SlevomatCodingStandard.Commenting.ForbiddenAnnotations.AnnotationForbidden)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

… this patch did not touch that file at all 🤷‍♂️😬

Ah, this was a regression in 9.5.x that has now been fixed: #3327853: [10.0.x and 9.5.x backports] Don't allow {@inheritDoc} annotation in PHPDocBlocks.

1 unrelated failure:

Testing Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest
.......F...............                                           23 / 23 (100%)

Time: 08:03.927, Memory: 6.00 MB

There was 1 failure:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testEditableCaption
Failed asserting that a NULL is not empty.

1 deprecation notice:

Testing Drupal\Tests\block\Kernel\Migrate\d7\MigrateBlockContentTranslationTest
.                                                                   1 / 1 (100%)

Time: 00:23.876, Memory: 4.00 MB

OK (1 test, 28 assertions)

Remaining self deprecation notices (1)

  1x: The theme 'bartik' is deprecated. See https://www.drupal.org/node/3223395#s-bartik
    1x in MigrateBlockContentTranslationTest::testBlockContentTranslation from Drupal\Tests\block\Kernel\Migrate\d7

⇒ this works fine, but the BC concerns in #30 still need to be addressed.

core/modules/aggregator is gone.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Comment #21 added the tag for maintainer review, referring to Comment #18. (I think it means #18.1, not #18.2). Before we respond to that question, it will help if the issue summary is updated to explain what the current patch actually does. From Comment #59:

The scope of this issue is very different from the Issue Summary. The IS needs to be updated.

Also, #30 raised concerns about BC, and #34 has a specific suggestion for addressing them.

I am removing the tag for maintainer response and adding a "Remaining tasks" section to the issue summary.

From Comment #32:

We don't actually have a new plugin id for the destination. So we use the same destination. But because of generated yml files that get exported, one-time, into migrate_plus... we have many cases where incremental migrations will have the updated destination but not the updated yaml that gets generated by the deriver.

Something similar can happen with any incremental migration, whether it uses core migration plugins or migrate_plus configuration. A developer creates some migrations using the core migrations as a starting point, or the derived migrations. These custom migrations include the destination ID. If we change what that destination ID does, then we can break the custom migrations.

Also, a single site migration (not an incremental one) can run into the same problem. A complex project might start today using a target site of Drupal 10.0 (or even 9.5) and the final migration might be into a 10.3 or 11.0 site.

Updated #67 for Drupal 10.2.0.

The Migrate Drupal Module was approved for removal in #3371229: [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3522602: [meta] Tasks to remove Migrate Drupal module and the removal work in #3522602: [meta] Tasks to remove Migrate Drupal module.

Migrate Drupal will not be moved to a contributed project. It will be removed from core after the Drupal 12.x branch is open.

The Migrate Drupal Module and Migrate Drupal UI are deprecated and they are not in Drupal 12.0.0.

Issues for these modules should now be on the 11.x branch. And the changes are limited to critical and major bug fixes. Other changes are allowed at the discretion of the core Release Managers in consultation with the Migrate subsystem maintainers.