Remove the md_entity destination plugin hack

+++ /dev/null
@@ -1,93 +0,0 @@
-  public function calculateDependencies() {

I'm surprised we can remove this, although it's good. Is this because of the builders?

No longer applies.

Interdiff fails.

Hah, I think I submitted while that webchick was doing a bunch of commits for migrate.

This one should be ok. And, as before the interdiff fails.

And again.

Yep, another commit was made, thus the error. That is fixed in the attached.

Another reroll.

Reroll for 8.1.x

Ok, I'm not much of a word smith and I've not done a change record but let's keep this moving. How is this for a start?

Title: $migration parameter removed from destination plugin declaration
Project: Drupal Core
Introduced in branch/version: 8.2.x
Issues: 2543568
Description: The migration destination plugins used to receive the $migration parameter, allowing full access to the migration configuration. This coupling has been removed so that the destination plugins are independent.

What else is needed?

This:

1. Everything to be saved in the destination object should be in the process section. The destination plugin should not copy anything from the source directly.
2. The configuration of the destination plugin should be in the destination section. The destination plugin should not rely on the value of any other top level key.

And if I were doing this (I am not) I would postpone this on #2695297: Refactor EntityFile and use process plugins instead.

Removing novice tag. Writing meaningful change notices are usually harder than you think. I did a review of the code. Nothing jumped out at me that seems strange. We have plenty of test coverage in this area. So I think this is RTBC.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
@@ -2,87 +2,19 @@
+ * @deprecated in Drupal 8.2.x and will be removed in Drupal 9.0.x. Use

Wise to keep it around. Who know who is doing what at this point. Providing an @deprecated is about the best we can do.

1. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationInterface.php
   @@ -39,17 +39,10 @@ public function getIds();
   -   * @return array
   

   Looks like this shouldn't be removed.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
   @@ -2,87 +2,19 @@
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public function calculateDependencies() {
   -    $this->dependencies = parent::calculateDependencies();
   -    // Add a dependency on the module that provides the field type using the
   -    // source plugin configuration.
   -    $source_configuration = $this->migration->getSourceConfiguration();
   -    if (isset($source_configuration['constants']['type'])) {
   -      $field_type = $this->fieldTypePluginManager->getDefinition($source_configuration['constants']['type']);
   -      $this->addDependency('module', $field_type['provider']);
   -    }
   -    return $this->dependencies;
   -  }
   

   Given #3 I'm surprised we can remove this without adding any dependencies anywhere. How come this is possible?

1. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationPluginManager.php
   @@ -54,11 +58,20 @@ public function __construct($type, \Traversable $namespaces, CacheBackendInterfa
   -    return parent::createInstance($plugin_id, $configuration, $migration);
   +    $plugin_definition = $this->getDefinition($plugin_id);
   +    $plugin_class = DefaultFactory::getPluginClass($plugin_id, $plugin_definition);
   +    // If the plugin provides a factory method, pass the container to it.
   +    if (is_subclass_of($plugin_class, 'Drupal\Core\Plugin\ContainerFactoryPluginInterface')) {
   +      $plugin = $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition);
   +    }
   +    else {
   +      $plugin = new $plugin_class($configuration, $plugin_id, $plugin_definition);
   +    }
   +    return $plugin;
   

   Since this now extends DefaultPluginManager, why do we need to bother copying and pasting the factory logic here? DefaultPluginManager uses ContainerFactory by default, which is aware of ContainerFactoryPluginInterface.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
   @@ -2,87 +2,19 @@
   + * @deprecated in Drupal 8.2.x and will be removed in Drupal 9.0.x. Use
   

   I imagine we'll probably remove this *before* D9. Can we s/in/before?

3. +++ b/core/modules/user/migration_templates/user_picture_field.yml
   @@ -18,4 +18,7 @@ process:
   +  dependencies:
   +    module:
   +      - image
   

   Where are these used? It seems to me that we should have a calculateDependencies() implementation on DestinationBase that actually makes use of this configuration.

re #41: newish but no, blame says they where both part of the d6 translation patch. Your patch changes the EntityContentBase constructor so it makes sense it would have to fix the tests instantiating it directly. That or keep the argument and ignore the value depending on how we want to handle BC.

+++ b/core/modules/migrate/src/Plugin/MigrateDestinationPluginManager.php
@@ -45,20 +46,11 @@ class MigrateDestinationPluginManager extends MigratePluginManager {
+    $plugin_interface = isset($plugin_interface_map[$type]) ? $plugin_interface_map[$type] : NULL;

What is $plugin_interface_map? Doesn't seem to be defined in this function, or as a parameter.

Apart from that, I think this is RTBC.

Preemptively RTBC. I see no reason why Drupal CI won't pass this.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
@@ -2,87 +2,19 @@
+ *
+ * @deprecated in Drupal 8.2.x and will be removed before Drupal 9.0.x. Use
+ *   \Drupal\migrate\Plugin\migrate\destination\EntityFieldStorageConfig
+ *   instead.
+ *
+ * @see \Drupal\migrate\Plugin\migrate\destination\EntityFieldStorageConfig
  */
-class EntityFieldStorageConfig extends BaseEntityFieldStorageConfig {

Given that migrate is still not out of alpha what is the advantage of deprecating this and not removing?

Nice diff stat btw.

And they won't need to make any changes if we leave it as is?

1. Why is MigrateDestinationPluginManager::createInstance removed?
2. This should be split into unhacking EntityFieldStorageConfig which is a good idea and removing the migrateinterface from the destination which has absolutely no justification given and given it relies on the Row it's only lip service to haughty ideas and zero gain in practice. Or I missed those.

In particular

Ideally, migration destination plugins (which wrap delivery of data to destinations such as Drupal entities) should be usable by themselves,

For what? These plugins are thin wrappers around perfectly usable APIs on their own. I fail to see how a migration destination plugin could be used for anything else but being the glue between Row and whatever CRUD API it wraps.

Is this BC breaking still?

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
@@ -2,87 +2,19 @@
+ * @deprecated in Drupal 8.2.x and will be removed before Drupal 9.0.x. Use

It cannot be "removed before Drupal 9.0.x" but it can be:

• "removed before Drupal 9.0.0"

OR

• "removed in Drupal 9.0.x"

Lets go with "removed in Drupal 9.0.x".

Applied mikeryan comment #58.

Assigning to myself to review. Reviews from others is fine in the mean time.

There are no more md_entity mentions in all of core, except for the BC layer. The migration is no longer passed into fields. Do we need to add a test of the BC layer? Me thinks no. Ready to go.

un-assigning.

1. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationInterface.php
   @@ -39,9 +39,6 @@ public function getIds();
   -   * @todo Review the cases where we need the Migration parameter, can we avoid
   -   *   that? To be resolved with https://www.drupal.org/node/2543568.
   -   *
       * @param \Drupal\migrate\Plugin\MigrationInterface $migration
       *   (optional) The migration containing this destination. Defaults to NULL.
   

   Does this need a new issue then? Rather than removing the comment.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
   @@ -2,87 +2,19 @@
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public function calculateDependencies() {
   -    $this->dependencies = parent::calculateDependencies();
   -    // Add a dependency on the module that provides the field type using the
   -    // source plugin configuration.
   -    $source_configuration = $this->migration->getSourceConfiguration();
   -    if (isset($source_configuration['constants']['type'])) {
   -      $field_type = $this->fieldTypePluginManager->getDefinition($source_configuration['constants']['type']);
   -      $this->addDependency('module', $field_type['provider']);
   -    }
   -    return $this->dependencies;
   -  }
   

   This is the only code that might result in different behaviour. I'm not sure why we are emptying this class.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
   @@ -2,87 +2,19 @@
   -  /**
   -   * {@inheritdoc}
   -   */
   -  protected static function getEntityTypeId($plugin_id) {
   -    return 'field_storage_config';
   -  }
   

   I think removing this would break things. Since it'll now do:

     protected static function getEntityTypeId($plugin_id) {
       // Remove "entity:".
       return substr($plugin_id, 7);
     }
   

   Which wouldn't return 'field_storage_config'. Again I just think we should be deprecating the class and not actually changing it. If we change it we definitely need a test to prove we've not broken it.

Assigning to myself for a final review.

+++ b/core/modules/migrate/src/Plugin/MigrateDestinationInterface.php
@@ -83,11 +83,9 @@ public function getIds();
+   * @deprecated in Drupal 8.2.x, will be removed before Drupal 9.0.x.

I can see what we are trying to do here, but I don't think we can mark a parameter deprecated via an annotation. But I also cannot find any docs on how or if this is allowed or possible.

Except for that small nit, this is good to go. Anyone know?

Random test failure against 8.3.x. All feedback addressed. Back to RTBC.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

I think this may have broken HEAD.

It was actually #2746671: CCK field data not available for D7 taxonomy term migrations so I left this issue alone. :)