Migrate in core: Add a load system for migrate plugins

I don't fully understand why we don't change the Migration entity annotation to use this new controller.

The rest are nitpicks, the logic seems fine.

1. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrationStorageController.php
   @@ -0,0 +1,116 @@
   +      foreach ($ids as $id) {
   +        if (($n = strpos($id, ':')) !== FALSE) {
   +          $base_id = substr($id, 0, $n);
   +          $ids_to_load[] = $base_id;
   +          $sub_id = substr($id, $n + 1);
   +          if ($sub_id == '*') {
   +            $dynamic_ids[$base_id] = NULL;
   +          }
   +          elseif (!isset($dynamic_ids[$base_id]) || is_array($dynamic_ids[$base_id])) {
   +            $dynamic_ids[$base_id][] = $sub_id;
   +          }
   +        }
   +        else {
   +          $ids_to_load[] = $id;
   +        }
   +      }
   

   This could use some inline docs. especially around the str functions, please.

2. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrationStorageController.php
   @@ -0,0 +1,116 @@
   + * Contains \Drupal\migrate_drupal\MigrateStorageController.
   

   This should be\Drupal\migrate\MigrationStorageController

3. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrationStorageController.php
   @@ -0,0 +1,116 @@
   +  public function buildDependencyMigration($migrations) {
   ...
   +class MigrationStorageController extends ConfigStorageController {
   

   This calls for an interface

4. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateLoadInterface.php
   @@ -0,0 +1,35 @@
   +   * @param $sub_ids
   ...
   +   *   If NULL then load all sub-migrations.
   ...
   +   *   For example, when loading d6_node:article, sub_id will be article.
   

   Missing typehint and default values

5. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateLoadInterface.php
   @@ -0,0 +1,35 @@
   +   * @param $sub_id
   

   Missing typehint

6. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateLoadInterface.php
   @@ -0,0 +1,35 @@
   + * Contains
   + */
   +namespace Drupal\migrate\Plugin;
   

   ahem

7. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateLoadInterface.php
   @@ -0,0 +1,35 @@
   +   * @param EntityStorageControllerInterface $storage_controller
   ...
   +   * @param EntityStorageControllerInterface $storage_controller
   

   We use (and the rest of the patch mostly does) full class names here

8. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/load/LoadEntity.php
   @@ -0,0 +1,80 @@
   + * Contains \Drupal\migrate\Plugin\load\LoadEntityBase.
   ...
   +namespace Drupal\migrate_drupal\Plugin\migrate\load;
   

   migrate_drupal

As I got really confused it would be great to document why using an entity query would not solve the problem.

Color *me* confused. An entity query against *what* ? We are creating configuration entities out of thin air!

Or you mean why we dont run an entity query against the D6 database? o_O

6: 2190561-migrate-load-plugins-5.patch queued for re-testing.

Retesting, failures were unrelated.

I don't fully understand why we don't change the Migration entity annotation to use this new controller.

As I understand it, this patch is an attempt to keep patches at a reviewable size. The change isn't wired in because the only way to test the controller is to include the entire node or vocabulary migration, at which point the patch becomes so large that this might have been lost in the rest of it. Actually wiring it in without one of those two migrations would be asking to commit an untested storage controller, and that would be asking for trouble.

This patch

1. Removes mapArray as it is gone from core
2. Adds missing public qualifiers to methods
3. Separates dependencies and requirements.

This allows d6_block to have

dependencies:
  - d6_menu: false

implying that if d6_menu runs that then it needs to run first; but if it doesn't run; we are still good.

Note the difference between requirements and dependencies are explained in painstaking detail in the Migration entity in the $requirements and the $dependencies properties doxygen:

  /**
   * These migrations must be already executed before this migration can run.
   *
   * @var array
   */
  protected $requirements = array();

  /**
   * These migrations, if ran at all, must be executed before this migration.
   *
   *@var array
   */
  public $dependencies = array();

Added the docs inline as well. Interdiff against #6.

On careful consideration we will move this to migrate_drupal. It is not necessary for custom migrations, it is somewhat confusing and altogether quite a bit ugly. Whille for example WordPress has the concept of custom post types , nothing stops the WordPress - to - Drupal module from depending on migrate_drupal. Ugly to the ugly. Fits.

We now need to add this into migrate_drupal before we can post the D6-D8 patch so attached is the load entity patch.

I have been away from migrate for some weeks and I don't clearly understand the reasoning for changing your mind.
Anyway, I agree that this should be part of migrate.

Logic is already battle-ready because it is working for D6 migrations.
I checked docs and code standards and couldn't find anything wrong.
The lack of tests will be fixed once the D6 migrations are committed.

So... marking as RTBC for moving forward with IMP.

I don't clearly understand the reasoning for changing your mind.

We always needed it, we just decided to move it from migrate into migrate_drupal.

And the reason for that -- we tried to make the system generic but it turned out to be a no-go so then why burden the API module with it?

Looking forward to the d6 migration patch :)

Committed 23b38b7 and pushed to 8.x. Thanks!

diff --git a/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/load/LoadEntity.php b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/load/LoadEntity.php
index 1d377bc..113d473 100644
--- a/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/load/LoadEntity.php
+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/load/LoadEntity.php
@@ -56,7 +56,6 @@ public function load(EntityStorageInterface $storage, $sub_id) {
    * {@inheritdoc}
    */
   public function loadMultiple(EntityStorageInterface $storage, array $sub_ids = NULL) {
-    // This entity type has no bundles ('user', 'feed', etc).
     if (isset($this->configuration['bundle_migration'])) {
       /** @var \Drupal\migrate\Entity\MigrationInterface $bundle_migration */
       $bundle_migration = $storage->load($this->configuration['bundle_migration']);
@@ -67,6 +66,7 @@ public function loadMultiple(EntityStorageInterface $storage, array $sub_ids = N
       }
     }
     else {
+      // This entity type has no bundles ('user', 'feed', etc).
       $this->bundles = array($this->migration->getSourcePlugin()->entityTypeId());
     }
     $sub_ids_to_load = isset($sub_ids) ? array_intersect($this->bundles, $sub_ids) : $this->bundles;

Fixed during commit.