Deprecate migrate destination plugins needed only for site upgrades

Updated from discussion on the Migrate video call today. benjifisher, heddn, mikelutz and quietone were present. #3518542: [meeting] Migrate Meeting 2025-04-24 2100Z

Two of the blocking issues have been Fixed, and the third is Closed (won't fix).

I don't think we want to blindly deprecate all of the destinations. In some cases, we just want to remove the Drupal upgrade logic. Although that said, we might be able to completely deprecate because in theory the more generic entity destination should cover most cases. Let's see what this turns into with time.

Could we document in the summary why those not deprecated should remain?

Updating ID so that list of retained destinations is clearer. Seems reasonable to deprecate shortcut_set_users, d7_theme_settings, user_data.

I think the entity destinations should be retained, Migrate is such a good general purpose toolkit I expect there are integrations using many of those.

Unsure if anyone might be destinating data to all of them, eg entity:image_style or entity:date_format are harder to picture for non-upgrade migrations, but there's a scenario for almost everything!

I started looking at the individual destination plugins. I will have to finish my review some other time, but here are a couple of observations for now.

I think that EntityContentComplete.php was added to support the "complete" node migrations. Even though it is in the migrate module, I think we should consider deprecating it.

Several of the destination plugins use Row::getDestinationProperty(). That suggests that these plugins are coupled to the core migrations (intended for site upgrades). For example, in EntityImageStyle,

    // Need to set the effects property to null on the row before the ImageStyle
    // is created, this prevents improper effect plugin initialization.
    if ($row->getDestinationProperty('effects')) {
      $effects = $row->getDestinationProperty('effects');
      $row->setDestinationProperty('effects', []);
    }

I may be missing some, but at least these destination plugins call that method:

grep -ril getDestinationProperty core/modules/*/src/Plugin/migrate/destination
core/modules/ban/src/Plugin/migrate/destination/BlockedIp.php
core/modules/block/src/Plugin/migrate/destination/EntityBlock.php
core/modules/file/src/Plugin/migrate/destination/EntityFile.php
core/modules/image/src/Plugin/migrate/destination/EntityImageStyle.php
core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
core/modules/migrate/src/Plugin/migrate/destination/Config.php
core/modules/migrate/src/Plugin/migrate/destination/EntityContentComplete.php
core/modules/migrate/src/Plugin/migrate/destination/Entity.php
core/modules/migrate/src/Plugin/migrate/destination/ComponentEntityDisplayBase.php
core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
core/modules/migrate/src/Plugin/migrate/destination/EntityBaseFieldOverride.php
core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
core/modules/node/src/Plugin/migrate/destination/EntityNodeType.php
core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
core/modules/shortcut/src/Plugin/migrate/destination/ShortcutSetUsers.php
core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
core/modules/user/src/Plugin/migrate/destination/EntityUserRole.php
core/modules/user/src/Plugin/migrate/destination/UserData.php
core/modules/user/src/Plugin/migrate/destination/EntityUser.php

Ouch, that is a majority of them!

Some of those are false positives. There is no way we can remove core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php - I imagine that is the most used destination plugin?!

As with the process plugins issue, I think we should err on the side of caution here and only deprecate ones that we know are no use. Almost all migrations that I write are for content only, and config is set up first outside of migrate. Therefore I suspect that the config entity destinations are largely not much use, but the content entity ones are probably all useful to someone - but perhaps we can decide on that in a followup or two.

I personally think UserData is useful in niche edge cases and believe I might have used it once on a project. I agree that the custom ShortcutSetUsers and ThemeSettings ones are unlikely to be useful in any non-Drupal migration.

Almost all migrations that I write are for content only, and config is set up first outside of migrate.

Same here. But I think we still have to consider each one separately. For example, entity:taxonomy_vocabulary is a config migration, but I can see importing vocabularies (and then terms) from a CSV file (or other source) and the code looks like it is doing what a destination plugin is supposed to do: avoiding potential problems before saving the entity, in a way that does not depend on the source nor migration plugins:

  public function getEntity(Row $row, array $old_destination_id_values) {
    /** @var \Drupal\taxonomy\VocabularyInterface $vocabulary */
    $vocabulary = parent::getEntity($row, $old_destination_id_values);

    // Config schema does not allow description to be empty.
    if (trim($vocabulary->getDescription()) === '') {
      $vocabulary->set('description', NULL);
    }
    return $vocabulary;
  }

On the other hand, entity:comment is a content migration, but its import() method is there to reset, then restore, $this->state->get('comment.maintain_entity_statistics', 0). I think entity statistics are maintained only when migrating from D6 or D7. OTOH, this looks appropriate:

  protected function processStubRow(Row $row) {
    parent::processStubRow($row);
    // Neither uid nor name is required in itself, but it is required to set one
    // of them.
    $row->setDestinationProperty('name', 'anonymous_stub');
  }

Some of those are false positives. There is no way we can remove core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php

Of course. I only said that using Row::getDestinationProperty() "suggests that these plugins are coupled to the core migrations" (emphasis added). If we look more closely, it does not hard-code any destination properties. For example,

      $property = $this->storage->getEntityType()->getKey('langcode');
      if ($row->hasDestinationProperty($property)) {
        $language = $row->getDestinationProperty($property);

Maybe it is OK for a more specific destination plugin to hard-code a destination property, if that is also one of the entity keys.

I am fixing some copy/paste errors in the issue summary.

I am reviewing the 17 destination plugins, and I decided to start with the 5 that do not extend Drupal\migrate\Plugin\migrate\destination\Entity;

• \Drupal\ban\Plugin\migrate\destination\BlockedIp
• \Drupal\language\Plugin\migrate\destination\DefaultLangcode
• \Drupal\shortcut\Plugin\migrate\destination\ShortcutSetUsers
• \Drupal\system\Plugin\migrate\destination\d7\ThemeSettings
• \Drupal\user\Plugin\migrate\destination\UserData

Most of these use some module-defined service to import the data. (The exception is DefaultLangcode, which creates and saves a config object—not a config entity.) For example, BlockedIp includes

  public function import(Row $row, array $old_destination_id_values = []) {
    $this->banManager->banIp($row->getDestinationProperty('ip'));

    return ['ip' => $row->getDestinationProperty('ip')];
  }

Generally, a destination plugin following this pattern should be kept, not deprecated. Only 2 of the 5 deserve special attention:

ShortcutSetUsers

@quietone, you added this one to the Deprecate list without comment. I think we should keep it. It follows the same pattern as BlockedIp:

  public function import(Row $row, array $old_destination_id_values = []) {
    /** @var \Drupal\shortcut\ShortcutSetInterface $set */
    $set = $this->shortcutSetStorage->load($row->getDestinationProperty('set_name'));
    /** @var \Drupal\user\UserInterface $account */
    $account = User::load($row->getDestinationProperty('uid'));
    $this->shortcutSetStorage->assignUser($set, $account);

    return [$set->id(), $account->id()];
  }

We may decide to deprecate and remove the Shortcut module, but that decision is still pending: #3476880: [Policy] Move Shortcut module to contrib is still NR. Did you have another reason for deprecating it?

d7\ThemeSettings

TL;DR: I agree that we should deprecate this one, but the decision is not simple.

Despite the d7 in the namespace, this plugin is not closely tied to upgrades from D7. The import method does unset() a few "pseudo field" destination properties, but mostly it is a wrapper for theme_settings_convert_to_config(). At first, that seems fairly generic and possibly useful.

Looking more closely at theme_settings_convert_to_config(), its main job is to convert some legacy array keys to current config keys. For example:

    if ($key == 'default_logo') {
      $config->set('logo.use_default', $value);
    }

It looks as though, when the theme settings form was converted from D7 to D8, someone decided that it would be easier to use this helper function than to rewrite the form builder to match the desired config object. A little research (git log -S theme_settings_convert_to_config) confirms this:

• #1712250: Convert theme settings to configuration system added the helper function as part of a database update function (hook_update_N). Later, when we realized that database update functions were not practical for the D7-to-D8 transition, the update function was replaced with a migration using this destination plugin.
• #1992606: Convert system_theme_settings to FormInterface used the helper function as part of the submit handler for the theme-settings form.

These two uses are reflected in the parameter comment for the current version of the helper function:

 * @param array $theme_settings
 *   An array of theme settings from system setting form or a Drupal 7 variable.

IMO, we should have rewritten the form builder long ago so that it does not need this helper function. @sun expressed the same opinion in #1712250-39: Convert theme settings to configuration system:

Oh, I see. We might have to change all the keys in the theme settings form definition then… :-/

If we want to make that a follow-up issue, then I believe it has to be at least major, and we should precisely state a @todo D8: Remove this function after updating the theme settings form to use the new configuration object keys.

The @todo comment was added, but then it was removed. It never got to-done.

In the interest of eventually deprecating and removing theme_settings_convert_to_config(), let’s not preserve it in this destination plugin. If it is used in only one place (the form submit handler) then it will be simpler to remove it.

If anyone wants to use the migration system to import theme settings from a non-D7 source, then it is simpler to use the keys appropriate for the current config object. If they are already using the d7_theme_settings destination plugin, then they could copy the code to their custom module once we remove it.

In Comment #18, I suggested that using Row::getDestinationProperty() was a hint that a destination plugin was too closely coupled to a particular migration. Then, in Comment #21, I suggested,

Maybe it is OK for a more specific destination plugin to hard-code a destination property, if that is also one of the entity keys.

Looking at the base classes, I think that is correct. Ideally, a destination plugin lists the supported properties in its fields() method. For an entity plugin, those should be the entity properties. Then it is perfectly OK to use getDestinationProperty() with a supported property.

Only 3 of the 17 destination plugins are for content entities:

• Drupal\comment\Plugin\migrate\destination\EntityComment
• Drupal\file\Plugin\migrate\destination\EntityFile
• Drupal\user\Plugin\migrate\destination\EntityUser

EntityComment

I already looked at this one in Comment #21. I think we should keep it for processStubRow(), but I think we should have a followup issue to remove the import() method.

EntityFile

This one does two useful things. In import(), it loads the entity by uri (one of the entity properties) instead of fid. In processStubRow(), it creates an empty file for each stub file entity.

Keep it.

EntityUser

This one is mostly about handling passwords, and there are a few lines giving special treatment to user/1.

Keep it.

I finished my review with the 9 destination plugins for config entities:

• Drupal\block\Plugin\migrate\destination\EntityBlock
• Drupal\comment\Plugin\migrate\destination\EntityCommentType
• Drupal\image\Plugin\migrate\destination\EntityImageStyle
• Drupal\node\Plugin\migrate\destination\EntityNodeType
• Drupal\search\Plugin\migrate\destination\EntitySearchPage
• Drupal\shortcut\Plugin\migrate\destination\EntityShortcutSet
• Drupal\system\Plugin\migrate\destination\EntityDateFormat
• Drupal\taxonomy\Plugin\migrate\destination\EntityTaxonomyVocabulary
• Drupal\user\Plugin\migrate\destination\EntityUserRole

The base class implements fields() to return an empty array, and I do not think any of these plugins override that. Now that we have config validation, I think we should use it: the top-level keys under mapping should be the fields, and the corresponding labels can be the values. This idea is so far out of scope for this issue that I do not think it counts as a followup issue. It is just something that we should do to improve the migration system.

Drupal\block\Plugin\migrate\destination\EntityBlock

This plugin overrides getEntity() to load the block by plugin ID and theme instead of entity ID. I think we should keep it.

It also overrides import() to re-throw SchemaIncompleteException. This was added in #3265483: Handle block migration for modules moved to contrib to deal with blocks provided by modules that used to be in core but have been removed. I think we should remove this in a follow-up issue. It should be the responsibility of the source plugin and the migration to send only valid rows to the destination plugin.

Drupal\comment\Plugin\migrate\destination\EntityCommentType

This plugin overrides import() to add a Body field to the comment type:

    \Drupal::service('comment.manager')->addBodyField(reset($entity_ids));

As long as there is a comment module in core, and it provides a service to do that, I think we should keep this plugin.

Drupal\image\Plugin\migrate\destination\EntityImageStyle

This plugin adds image effects using $style->addImageEffect($effect) so that the effect plugin can be initialized. Keep it.

Drupal\node\Plugin\migrate\destination\EntityNodeType

This plugin overrides getEntity() because the config schema does not allow description or help text to be empty strings. Keep it.

It also overrides import() to add a Body field, if the create_body destination property is set. I think we should remove that in a followup issue.

Drupal\search\Plugin\migrate\destination\EntitySearchPage

This plugin overrides updateEntity() to invoke $entity->setPlugin() and $entity->getPlugin()->setConfiguration(). I think we should keep it.

It also overrides import() to check that the module providing the search page is enabled. Again, I think the source plugin and the migration should be responsible for checking that. Let’s remove this method in a followup issue.

Drupal\shortcut\Plugin\migrate\destination\EntityShortcutSet

This plugin overrides getEntity() in order to do this:

    // Set the "syncing" flag to TRUE, to avoid duplication of default
    // shortcut links.
    $entity->setSyncing(TRUE);

I am not sure what problem this is supposed to solve, nor how it works. It sort of looks like something that comes up only when upgrading from D6/D7, so maybe we should deprecate this one.

Drupal\system\Plugin\migrate\destination\EntityDateFormat

This plugin overrides updateEntityProperty(). In the parent class, that method handles nested properties using NestedArray::setValue(). In the override, it uses the dedicated DateFormatInterface::setPattern() method when appropriate. I think we should keep this one.

Drupal\taxonomy\Plugin\migrate\destination\EntityTaxonomyVocabulary

I described this plugin in Comment #21. I think we should keep it.

Drupal\user\Plugin\migrate\destination\EntityUserRole

This plugin overrides import() to filter out invalid permissions. If we do not do that in the destination plugin, then they will be filtered out when we save the entity (the role). The only difference is that the user module will log an error instead of the migration system adding a warning to the migration message table.

I think this is another case where the source plugin and the migration should be responsible for sending valid data to the destination plugin. Let’s deprecate this one.

Rebased with 11.x branch.

Let me summarize the recommendations from my previous comments:

Deprecate

• EntityContentComplete (in the migrate module)
• EntityShortcutSet
• d7\ThemeSettings
• EntityUserRole

Do not deprecate

• ShortcutSetUsers

Followup

In an issue targeting Drupal 12 (perhaps at the same time that the deprecated code is removed),

• Remove EntityComment::import().
• Remove EntityBlock::import().
• Remove EntityNodeType::import().
• Remove EntitySearchPage::import().

@quietone:

Yes, I put EntityShortcutSet on the wrong list. I just updated Comment #32, moving it to the Deprecate list. Thanks for catching that.

Good idea: let's deprecate the import() methods on the Followup list.

+1 for deprecating the deriver along with the EntityContentComplete plugin.

I will have a look at the code later today.

The deprecated classes look good to me.

This issue still NW to deprecate the four import() methods. (See Comments #32-#34.)

@quietone:

Thanks for the updates, including the change record. I am updating the issue summary, now that we have agreed on the deprecations.

In Comment #35, I missed something: when adding a constructor to a class that did not have one, we should include a doc block. I added suggestions to the MR for three files. NW for that.

I also reviewed the deprecations for the import() methods. They all look good.

In that case, I think the MR is ready.

This looks good, balance of which ones to keep vs. remove seems OK and we can always deprecate more individual plugins later.

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!