Create the migrate builder plugin type

Initial review after a first read, will apply the patch tomorrow and review again.

1. +++ b/core/modules/migrate/src/MigrateTemplateStorage.php
   @@ -86,4 +93,39 @@ public function getAllTemplates() {
   +   *  The template ID.
   

   Should be indented by two spaces :)

2. +++ b/core/modules/migrate/src/MigrateTemplateStorage.php
   @@ -86,4 +93,39 @@ public function getAllTemplates() {
   +   *  The parsed template, or NULL if it wasn't found.
   +   */
   +  public function load($id) {
   +    return $this->getAllTemplates()[$id];
   +  }
   

   Although it would return NULL, it would also through a PHP notice if it didn't exist?

3. +++ b/core/modules/migrate/src/MigrateTemplateStorage.php
   @@ -86,4 +93,39 @@ public function getAllTemplates() {
   +   * @param array[] $templates
   

   No square brackets need after array here.

4. +++ b/core/modules/migrate/src/MigrateTemplateStorage.php
   @@ -86,4 +93,39 @@ public function getAllTemplates() {
   +   *  The parsed templates.
   

   Two spaces.

5. +++ b/core/modules/migrate/src/MigrateTemplateStorage.php
   @@ -86,4 +93,39 @@ public function getAllTemplates() {
   +  public function buildMigrations(array $templates) {
   

   a) I think this method should return the migrations. b) I'm still not sure about the name, mainly because we have another public method called buildMigrations() that does something different.

6. +++ b/core/modules/migrate/src/Plugin/migrate/builder/BuilderBase.php
   @@ -0,0 +1,46 @@
   +   *  The plugin ID.
   ...
   +   *  (optional) Additional configuration for the plugin.
   

   Two spaces again.

7. +++ b/core/modules/migrate/src/Plugin/migrate/builder/BuilderBase.php
   @@ -0,0 +1,46 @@
   +   * @return \Drupal\migrate\Plugin\MigrateSourceInterface
   

   Missing return description.

8. +++ b/core/modules/migrate/src/Plugin/migrate/builder/BuilderBase.php
   @@ -0,0 +1,46 @@
   +  protected function getSource($plugin_id, array $configuration = []) {
   

   We're returning $migration->getSourcePlugin(), so maybe this method should be called getSourcePlugin()? Maybe better yet, getEmptySourcePlugin(), oo, but we have an empty source plugin, another name? Is it just a source plugin without an idMap?

9. +++ b/core/modules/migrate/src/Plugin/migrate/builder/BuilderBase.php
   @@ -0,0 +1,46 @@
   +        'plugin' => 'null',
   

   Should be NULL, also no quotes?

10. +++ b/core/modules/migrate_drupal/migration_templates/d6_field.yml
    @@ -2,6 +2,9 @@ id: d6_field
    +  method: processField
    
    +++ b/core/modules/migrate_drupal/migration_templates/d6_field_formatter_settings.yml
    @@ -2,6 +2,9 @@ id: d6_field_formatter_settings
    +  method: processFieldFormatter
    
    +++ b/core/modules/migrate_drupal/migration_templates/d6_field_instance.yml
    @@ -2,6 +2,9 @@ id: d6_field_instance
    +  method: processFieldInstance
    
    +++ b/core/modules/migrate_drupal/migration_templates/d6_field_instance_widget_settings.yml
    @@ -2,6 +2,9 @@ id: d6_field_instance_widget_settings
    +  method: processFieldWidget
    

    This is a little confusing how these methods have nothing to do with the builder, but actually something to do with cck field plugin types?

11. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckMigration.php
    @@ -0,0 +1,47 @@
    +  private $processedFieldTypes = [];
    

    protected

12. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/Node.php
    @@ -0,0 +1,73 @@
    +   *  The field type (plugin ID).
    ...
    +   *  The migration, if any.
    

    Two spaces.

13. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/Node.php
    @@ -0,0 +1,73 @@
    +   * @return \Drupal\migrate_drupal\Plugin\MigrateCckFieldInterface
    

    Missing return comment.

14. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/Node.php
    @@ -0,0 +1,73 @@
    +  private function getCckPlugin($field_type, MigrationInterface $migration = NULL) {
    

    Lets use protected

15. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/Node.php
    @@ -0,0 +1,73 @@
    +      $values['id'] .= '__' . $node_type;
    

    Previously we were using a colon, eg d6_node:page, any reason to change? Not specifically against it, just asking, seems like an un-needed api change.

16. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/TermNode.php
    @@ -0,0 +1,88 @@
    +   * @return array
    

    missing comment.

17. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/TermNode.php
    @@ -0,0 +1,88 @@
    +  private function getVocabularyIdMap() {
    

    We don't use private, should be protected.

18. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/FieldInstance.php
    @@ -73,13 +57,9 @@ public function fields() {
    -    // Unserialize data.
    -    $widget_settings = unserialize($row->getSourceProperty('widget_settings'));
    -    $display_settings = unserialize($row->getSourceProperty('display_settings'));
    -    $global_settings = unserialize($row->getSourceProperty('global_settings'));
    -    $row->setSourceProperty('widget_settings', $widget_settings);
    -    $row->setSourceProperty('display_settings', $display_settings);
    -    $row->setSourceProperty('global_settings', $global_settings);
    +    foreach (['widget_settings', 'display_settings', 'global_settings'] as $key) {
    +      $row->setSourceProperty($key, unserialize($row->getSourceProperty($key)));
    +    }
    

    Un-needed noise, this patch is probably big enough. Much cleaner though.

19. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/FieldInstance.php
    @@ -87,7 +67,7 @@ public function prepareRow(Row $row) {
    -    $ids = array(
    +    return array(
    
    @@ -96,7 +76,6 @@ public function getIds() {
         );
    

    lol, please no.

MigrateTemplateStorage::buildMigrations() - How about createMigrations() or createAndSaveMigrations() ?

I haven't done a thorough code review yet, but found these issues when making use of this functionality in #2535228: Make use of proposed builder interface:

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/Node.php
   @@ -0,0 +1,74 @@
   +      $this->cckPluginCache = $this->cckPluginManager->createInstance($field_type, [], $migration);
   

   Missing [$field_type].

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/Node.php
   @@ -0,0 +1,74 @@
   +    foreach ($this->getSourcePlugin('d6_node_type') as $row) {
   

   Need to pass $template['source'] as the 2nd arg here, so the d6_node_type plugin can find the source data.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/Node.php
   @@ -0,0 +1,74 @@
   +      $fields = $this->getSourcePlugin('d6_field_instance', ['node_type' => $node_type]);
   

   As above, need to add $template['source'] into the 2nd arg.

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/TermNode.php
   @@ -0,0 +1,90 @@
   +  protected function getVocabularyIdMap() {
   

   Need to pass in the 'source' from the original migration and stuff it into $template, to be able to access the source data.

In addition, we have a dependency issue. With this patch, the d6_node template turns into d6_node__blog, d6_node__page, etc. migrations (no migration with ID d6_node is created). Templates like d6_cck_field_values have dependencies on d6_node, which are copied as-is into the generated migrations, which thus fail to run. So, we need a means to substitute dependencies on generated migrations for the original dependency on a "builder" migration. Note that such a substitution is going to be needed generally, even apart from the builder-generated migrations - although for the moment migrate_upgrade is simply using the template IDs as the migration IDs it generates, to support scenarios like migrating from multiple sites each site will need unique IDs for the migrations it generates from the shared templates. E.g., if I'm merging my D6 "blog" and "forum" sites, from the d6_taxonomy_vocabulary template I'll want to make two migrations with IDs like blog_d6_taxonomy_vocabulary and forum_d6_taxonomy_vocabulary (and make all dependencies on vocabularies work for each site).

Just adapting the issue title for a little bit less confusion.

As mentioned on IRC, any migration_dependencies on templates whose builders generate multiple migrations from one template (I'm looking at you, d6_node) need to append :* to the referenced template ID.

Still need to play with actual usage of this feature, but a couple of nits on code review:

1. +++ b/core/modules/migrate/src/Entity/Migration.php
   @@ -234,6 +234,13 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
   +   * The of the template from which this migration was derived, if any.
   

   The... ID?... of the template...

2. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -16,6 +22,72 @@
   +   * Constructs a ConfigEntityStorage object.
   

   MigrationStorage object

A couple quick notes as I integrate this with migrate_upgrade:

1. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,65 @@
   +        $variant->set('template', $template_id . ':*');
   

   I don't think the :* should be here, we just want to preserve $template_id as-is.

2. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,65 @@
   +      $migrations += $variants;
   

   Needs array_merge() instead of +=.

Beyond that, what I'm dealing with now to make migrate_upgrade run with this patch is the fact that it's exposing missing dependencies in migrate_drupal. For example, d6_vocabulary_field_instance needs a dependency on d6_node:* (without it, it ran before the node migrations - it did not go well). I'm inclined to toss missing dependencies in here, but it could be a separate patch if you prefer. I'll let you know what the missing dependencies are.

1. +++ b/core/modules/migrate/src/MigrateTemplateStorage.php
   @@ -36,8 +36,8 @@ class MigrateTemplateStorage {
   -    $this->moduleHandler = $module_handler;
        $this->directory = $directory;
   +    $this->moduleHandler = $module_handler;
   

   Why reorder here?

2. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,65 @@
   +        $migration->save();
   

   Should we check source/destination plugin requirements here? Right now migrate_upgrade needs to pass $save=FALSE so it can check requirements before saving the migrations.

3. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -16,6 +22,74 @@
   +              ->condition('template', $template_id)
   

   Just want to reference here our previous discussion of handling multiple site migrations. E.g., if one is defining migrations from two different D6 sites into one D8 site, then if the template ID is d6_node the dependencies in one site would pick up all node migrations in both sites. However, the front-end tools (migrate_upgrade and its ilk) can probably deal with this by rewriting IDs with per-site prefixes (necessary anyway to disambiguate the migrations themselves), so we're not going to worry about it here at this time.

With the array_merge() change, plus adding d6_node_type to d6_vocabulary_field_instance's dependencies, migrate_upgrade (with the patch at https://www.drupal.org/node/2535228#comment-10148594) works nicely - it looks like everything that was already working continues to work, plus terms are now getting assigned to nodes which was previously broken. Almost ready to rtbc this...

RE: the missing d6_vocabulary_field_instance dependency, forgot I had already opened an issue for it: #2535364: Missing dependency in d6_vocabulary_field_instance.

Needs a reroll, the d6_term_node* migrations moved to the taxonomy module.

Code looks good to me, and it works in practice: #2535228: Make use of proposed builder interface.

Escalating priority, based on discussion with phenaproxima and mikeryan. This will enable fixing #2499173: migrate_drupal highjacks the Migration and MigrationStorage classes, as well as pave the way for field migrations from D7.

I'd really like to review this before it gets committed. I'll try look later today.

Initial review, will have to try apply this and test it as well :)

1. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,65 @@
   +  public function __construct(MigratePluginManager $builder_manager) {
   +    $this->builderManager = $builder_manager;
   +  }
   

   Missing function docs.

2. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,65 @@
   +   * @param boolean $save
   

   should be bool. https://www.drupal.org/coding-standards/docs#types

3. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,65 @@
   +  public function createMigrations(array $templates, $save = TRUE) {
   ...
   +    if ($save) {
   +      foreach ($migrations as $migration) {
   +        $migration->save();
   +      }
   +    }
   

   I'm not sure if this method should be handling the save, seems like it's over-reaching. With other entities in Drupal, they have a "create" method and then you have to call save on the returned entity, there is no shortcut.

4. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,65 @@
   +        $variants = [ Migration::create($template) ];
   

   Additional space within the array.

5. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -16,6 +22,74 @@
   +   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
   +   *   The language manager.
   +   */
   +  public function __construct(EntityTypeInterface $entity_type, ConfigFactoryInterface $config_factory, UuidInterface $uuid_service, LanguageManagerInterface $language_manager, QueryFactoryInterface $query_factory) {
   

   Missing docs for the query factory.

6. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -16,6 +22,74 @@
   +      // Scan through the migration's dependencies for things that match the
   +      // pattern template_id:*. This signals that the migration depends on
   

   "for things". Can we reword that?

7. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -16,6 +22,74 @@
   +      foreach ($dependencies as &$group) {
   +        foreach ($group as $index => $dependency_id) {
   

   Bad things can happen to those who iterate over arrays by reference, especially twice. Lets use array_walk() or just use the key.

8. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -16,6 +22,74 @@
   +            // Generated variants should have the template key set. This is
   +            // the default behavior of MigrationBuilder::buildMigrations().
   +            $group += $this->queryFactory->get($this->entityType, 'OR')
   +              ->condition('id', $template_id)
   +              ->condition('template', $template_id)
   +              ->execute();
   

   We're merging into $group but $group isn't actually used anywhere? Is it trying to grow the variable we're currently looping over? So complicated.

9. +++ b/core/modules/migrate/src/Plugin/MigrateBuilderInterface.php
   @@ -0,0 +1,27 @@
   +   * @param array $template
   +   *  The parsed template.
   +   *
   +   * @return \Drupal\migrate\Entity\MigrationInterface[]
   +   *  The unsaved migrations generated from the template.
   +   */
   

   Both @param and @return docs should have two space indent.

10. +++ b/core/modules/migrate/src/Plugin/migrate/builder/BuilderBase.php
    @@ -0,0 +1,52 @@
    +   * Returns a fully initialized instance of an arbitrary source plugin.
    

    Is it arbitrary if we're specifying the plugin_id?

11. +++ b/core/modules/migrate/tests/src/Unit/MigrateTestCase.php
    @@ -28,29 +28,41 @@ protected function getMigration() {
    -    $migration->expects($this->any())
    -      ->method('checkRequirements')
    -      ->will($this->returnValue(TRUE));
    -    $migration->expects($this->any())
    -      ->method('getIdMap')
    -      ->will($this->returnValue($this->idMap));
    +
    +    $migration->method('checkRequirements')
    +      ->willReturn(TRUE);
    +
    +    $migration->method('getIdMap')
    +      ->willReturn($this->idMap);
    

    Is there a relevant change here?

12. +++ b/core/modules/migrate_drupal/migration_templates/d6_cck_field_revision.yml
    @@ -14,4 +14,4 @@ destination:
    +    - d6_node_revision:*
    
    +++ b/core/modules/migrate_drupal/migration_templates/d6_cck_field_values.yml
    @@ -16,6 +16,6 @@ destination:
    +    - d6_node:*
    
    +++ b/core/modules/migrate_drupal/migration_templates/d6_comment.yml
    @@ -37,7 +37,7 @@ destination:
    +    - d6_node:*
    

    OK, so i the concept of the multiple bundle dependencies has moved from the runner to the migration template?

13. +++ b/core/modules/migrate_drupal/migration_templates/d6_node.yml
    @@ -2,6 +2,8 @@ id: d6_node
    +builder:
    +  plugin: d6_node
     source:
    

    What is the default method? I wonder if we should make it explicit?

14. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
    @@ -0,0 +1,42 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigratePluginManager $cck_manager) {
    +    parent::__construct($configuration, $plugin_id, $plugin_definition);
    +    $this->cckPluginManager = $cck_manager;
    

    Missing constructor docs.

15. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckMigration.php
    @@ -0,0 +1,47 @@
    + * @PluginID("d6_cck_migration")
    + */
    +class CckMigration extends CckBuilder {
    

    If this is the D6 cck migration plugin, should the class be the same? I see we also have the D6 namespace, these builders won't be re-usable?

16. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckMigration.php
    @@ -0,0 +1,47 @@
    +      if (in_array($field_type, $this->processedFieldTypes)) {
    +        continue;
    +      }
    +      elseif ($this->cckPluginManager->hasDefinition($field_type)) {
    

    If we continue, the next block doesn't need to be elseif().

    There is no else block, what does it mean if neither of these are TRUE, maybe a comment?

17. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/Node.php
    @@ -0,0 +1,74 @@
    +   * Returns a cckfield plugin.
    

    I think we've standardised on "Gets" in core

18. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/Node.php
    @@ -0,0 +1,74 @@
    +            ->processCckFieldValues($migration, $data['field_name'], $data);
    

    O, the method is just hard coded here for d6_node?

19. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/TermNode.php
    @@ -0,0 +1,95 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrateTemplateStorage $template_storage) {
    +    parent::__construct($configuration, $plugin_id, $plugin_definition);
    

    Missing docs.

20. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateNodeBuilderTest.php
    @@ -0,0 +1,61 @@
    +  public static $modules = ['migrate', 'migrate_drupal'];
    +
    +  public function setUp() {
    ...
    +  public function testCreateMigrationsNoSave() {
    

    Just tests but still missing docs.

1. +++ b/core/modules/migrate/src/MigrateTemplateStorage.php
   @@ -36,8 +36,8 @@ class MigrateTemplateStorage {
   -    $this->moduleHandler = $module_handler;
        $this->directory = $directory;
   +    $this->moduleHandler = $module_handler;
   

   Nitpick, but why'd this move? Seems unnecessary and changes the order of properties being set to be different from the parameters passed.

2. +++ b/core/modules/migrate/src/Plugin/migrate/builder/BuilderBase.php
   @@ -0,0 +1,52 @@
   +    // Source plugins are tightly coupled to migration entities, so we need
   +    // to create a fake migration in order to properly initialize the plugin.
   +    $values = [
   +      'id' => uniqid(),
   +      'source' => $configuration,
   +      // Since this isn't a real migration, we don't want a real destination.
   +      // The 'null' destination plugin is perfect for this! :)
   +      'destination' => [
   +        'plugin' => 'null',
   +      ],
   +    ];
   +    return Migration::create($values)->getSourcePlugin();
   

   YUUUUCK. Not your fault, but the fact that the Source plugins require the Migration entity to operate is a bad sign in general.

3. +++ b/core/modules/migrate_drupal/migration_templates/d6_field.yml
   @@ -2,6 +2,9 @@ id: d6_field
   +builder:
   +  plugin: d6_cck_migration
   +  method: processField
   
   +++ b/core/modules/migrate_drupal/migration_templates/d6_field_formatter_settings.yml
   @@ -2,6 +2,9 @@ id: d6_field_formatter_settings
   +builder:
   +  plugin: d6_cck_migration
   +  method: processFieldFormatter
   
   +++ b/core/modules/migrate_drupal/migration_templates/d6_field_instance.yml
   @@ -2,6 +2,9 @@ id: d6_field_instance
   +builder:
   +  plugin: d6_cck_migration
   +  method: processFieldInstance
   
   +++ b/core/modules/migrate_drupal/migration_templates/d6_field_instance_widget_settings.yml
   @@ -2,6 +2,9 @@ id: d6_field_instance_widget_settings
   +builder:
   +  plugin: d6_cck_migration
   +  method: processFieldWidget
   

   This is really confusing. These methods don't exist on this plugin so... ????

This seems a pretty big improvement over the existing load plugins for certain. It draws into question a number of the guiding architectural aspect of Migrate for me, but this patch should definitely move ahead and replace what's in core now.

Eclipse

Testing with migrate_upgrade, this fails in the configuration step because it's trying to generate d6_node_revision migrations using the same d6_node__ prefix as the node migrations were already created. I suggest replacing

      $values['id'] = 'd6_node__' . $node_type;

with

      $values['id'] = $template['id'] . '__' . $node_type;

which would also be a step towards making this plugin version-agnostic (need to look at d6_node_type and d6_field_instance references as well).

With that one-line change, migrate_upgrade works with this patch.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
   @@ -0,0 +1,54 @@
   + * Contains \Drupal\migrate_drupal\Plugin\migrate\builder\d6\CckBuilder.
   

   I don't see anything specific to D6/CCK here - could this be moved up to the builder namespace and renamed FieldBuilder?

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckMigration.php
   @@ -0,0 +1,53 @@
   + * Contains \Drupal\migrate_drupal\Plugin\migrate\builder\d6\CckMigration.
   

   Ditto - is there any reason not to move it up to the builder namespace and rename as FieldMigration? Would it not be reusable for D7 fields?

3. +++ b/core/modules/node/src/Plugin/migrate/builder/d6/Node.php
   @@ -0,0 +1,75 @@
   +    foreach ($this->getSourcePlugin('d6_node_type', $template['source']) as $row) {
   

   Can we generalize the d6_node_type and d6_field_instance references so this can be reused for D7 migrations? Or should that be a followup issue?

4. +++ b/core/modules/node/src/Plugin/migrate/builder/d6/Node.php
   @@ -0,0 +1,75 @@
   +      $values['id'] = 'd6_node__' . $node_type;
   

   As previously stated, replacing 'd6_node' with $template['id'] should fix the revision collision issue.

Test template needed an 'id'...

Forgot to upload the interdiff for my tweak.

I think this patch is about ready, i've not tested it in practice but it sounds like Mike has with migrate_upgrade and i've reviewed the code a couple of times. Big +1 to generating the migrations up front rather than LoadEntity magic, that should make debugging much simpler.

One thing I want to note, dependencies are now conceptually more confusing because a template specifies dependencies on another templates "built" migrations for the migrations that this template builds. Previously, a dependency just depended on another migration.

Additionally, templates don't depend on other templates? A template builder doesn't know if the other template has already been processed? Therefore migrations could be generated from one template although the migrations they depend on don't exist yet because that template hasn't been processed? @mike, with migrate_upgrade i'm guessing this isn't a problem because every template is processed and the output is presumably fine, regardless of which order they were generated?

Since migration_dependencies are purely a run-time concept - they say "don't run this migration until these other migrations have completed" - there's no requirement to construct migrations in dependency order, they "just" need to all be present when it comes time to run them. So, it is possible to end up with migrations that can't be run because necessary migrations don't exist. I don't think this is generally solvable in the core API, it's the responsibility of the tools built on it to work things out. For example, consider a wizard UI for constructing the set of migrations for a given scenario - the best ordering of the steps from a UX perspective may be different from the required technical ordering of the generated migrations.

There is a significant issue around dealing with migration dependencies in templates, mentioned in #7 above:

Note that such a substitution is going to be needed generally, even apart from the builder-generated migrations - although for the moment migrate_upgrade is simply using the template IDs as the migration IDs it generates, to support scenarios like migrating from multiple sites each site will need unique IDs for the migrations it generates from the shared templates. E.g., if I'm merging my D6 "blog" and "forum" sites, from the d6_taxonomy_vocabulary template I'll want to make two migrations with IDs like blog_d6_taxonomy_vocabulary and forum_d6_taxonomy_vocabulary (and make all dependencies on vocabularies work for each site).

The "substitution" referenced was solved for the builder case with the introduction of the :* syntax in dependencies. As for the general case, that's something I'll be trying to work out on the contrib side - if I feel there's something core can do to help support that, I'll open a new issue for it.

Anyway, the whole issue of maintaining dependencies among the migrations created from templates is inherent in the template implementation, it's not an issue created by the builders patch here.

/me thinks dependency management should be added to the "two hard problems in computer science"

OK, I think we're ready here.

Sorry, but I found several small problems, and lots of missing docs standards violations.

1. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,63 @@
   +class MigrationBuilder {
   

   Missing docblock.

2. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,63 @@
   +  /**
   +   * @var \Drupal\migrate\Plugin\MigratePluginManager
   +   */
   +  protected $builderManager;
   

   Incomplete docs.

3. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,63 @@
   +   * @param array $templates
   +   *   The parsed templates.
   

   This can't be typehinted to \Some\Class[]?

4. +++ b/core/modules/migrate/src/MigrationBuilder.php
   @@ -0,0 +1,63 @@
   +    $migrations = array();
   

   Why not use [] instead of array() in completely new code?

5. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -16,6 +22,95 @@
      /**
   +   * @var \Drupal\Core\Entity\Query\QueryFactoryInterface
   +   */
   +  protected $queryFactory;
   

   Docs.

6. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -16,6 +22,95 @@
   +   *   The entity type definition.
   

   There are many entity type definitions, so "the" doesn't make sense here.

7. +++ b/core/modules/migrate/src/Plugin/MigrateBuilderInterface.php
   @@ -0,0 +1,27 @@
   +/**
   + * Defines the builder plugin type, which implements custom logic to generate
   + * migration entities from templates.
   + */
   

   Violates the rule that this should be a single line, with potentially more detailed explanations after a blank line.

8. +++ b/core/modules/migrate/src/Plugin/MigrateBuilderInterface.php
   @@ -0,0 +1,27 @@
   +   * @param array $template
   +   *   The parsed template.
   

   Hrm, a parsed template is simply an array? That feels weird. Is it not a specific type, or not even string[]?

9. +++ b/core/modules/migrate/src/Plugin/migrate/builder/BuilderBase.php
   @@ -0,0 +1,52 @@
   +      // The 'null' destination plugin is perfect for this! :)
   

   We avoid exclamation points and smileys in our docs :)

   (I know this because @jhodgdon told me several times :P)

10. +++ b/core/modules/migrate/tests/src/Unit/MigrationStorageTest.php
    @@ -0,0 +1,137 @@
    +   * @var \Drupal\migrate\MigrationStorage
    ...
    +   * @var \Drupal\Core\Entity\Query\QueryInterface
    

    The docs say these are actual instances, but they really are mocked instances. Then this is wrong. Then we write something like:

    /**
     * @var \Drupal\Core\Cache\CacheFactoryInterface|\PHPUnit_Framework_MockObject_MockObject
     */
    

11. +++ b/core/modules/migrate/tests/src/Unit/MigrationStorageTest.php
    @@ -0,0 +1,137 @@
    +  /**
    +   * Calls a protected method of $this->storage by using the reflection hack.
    +   *
    +   * @param string $method
    +   *   The method to call.
    +   * @param array $arguments
    +   *   (optional) Arguments to pass.
    +   *
    +   * @return mixed
    +   *   The return value of the called method.
    +   */
    +  protected function invokeMethod($method, array $arguments = []) {
    +    $method = $this->reflector->getMethod($method);
    +    $method->setAccessible(TRUE);
    +    return $method->invokeArgs($this->storage, $arguments);
    +  }
    

    Ugh. We don't ever do this in core AFAIK.

    This is a new pattern, and therefore potentially a bad precedent. I think this is only acceptable if you can point to existing unit tests that do this.

12. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
    @@ -0,0 +1,54 @@
    +  /**
    +   * @var \Drupal\migrate\Plugin\MigratePluginManager
    +   */
    +  protected $cckPluginManager;
    

    Incomplete docs.

13. +++ b/core/modules/taxonomy/src/Plugin/migrate/builder/d6/TermNode.php
    @@ -0,0 +1,107 @@
    +  /**
    +   * @var \Drupal\migrate\MigrateTemplateStorage
    +   */
    +  protected $templateStorage;
    

    Incomplete docs.

Ugh. We don't ever do this in core AFAIK.

We've done this before to avoid having to extend the class we're testing when trying to set information onto that class, eg MigrateSqlSourceTestCase uses this approach to set the database and moduleHandler on the class we're testing to mocks. This is all because they're not injected.

However in this case I feel like we should be testing loadMultiple() instead, that's the public method. Testing methods using reflection is usually a code smell. Maybe the argument could be made that testing loadMultiple() wasn't easy/possible and therefore the reflection is needed here as well?

If we do end up needing it, lets at least use \ReflectionMethod which has an invoke() method.

+++ b/core/modules/migrate/src/MigrationBuilder.php
@@ -31,14 +36,15 @@
-   *   The parsed templates.
+   *   The parsed templates (each of which is an array parsed from YAML), keyed
+   *   by ID.

Much clearer :) But this is apparently an array of arrays, so it should actually be array[] in the docblock? That's a tiny nit though. Back to RTBC!

Okay!

@phenaproxima was so kind as to walk me through this patch for an hour or so.

- My first concern was that this sounded like a fairly fundamental change to the underlying Migrate API. However, it's really isolated to migrations with dependencies on dynamic information, which in core is basically just nodes/fields and vocabs/terms. So this patch should not have widespread consequences for existing sites/migrations. It also makes the Migrate API much more grokkable.

- The change record is more expansive than change records usually are, but OTOH you kind of need all that background in order to understand what you need to change and why. Just a note though that in general, where such background is necessary, it's best to put it under https://www.drupal.org/developing/api since those docs will stand the test of time. (Versus change records are only for people upgrading from 7->8 which will more or less dry up after ~2 years.)

- Had some questions about this:

+  protected function expandDependencies(array $dependencies) {
+    $expanded_dependencies = [];
+
+    foreach (array_keys($dependencies) as $group) {
+      $expanded_dependencies[$group] = [];
+
+      foreach ($dependencies[$group] as $dependency_id) {
+        if (substr($dependency_id, -2) == ':*') {
+          $template_id = substr($dependency_id, 0, -2);
+          $variants = $this->queryFactory->get($this->entityType, 'OR')
+            ->condition('id', $template_id)
+            ->condition('template', $template_id)
+            ->execute();
+          $expanded_dependencies[$group] = array_merge($expanded_dependencies[$group], $variants);
+        }
+        else {
+          $expanded_dependencies[$group][] = $dependency_id;
+        }
+      }
+    }

This looks a lot like we're re-creating the config dependencies system. Adam explained that this is not the case, because the config dependencies system only happens at install time, and this is something that happens during runtime.

We also talked a bit about this gem. :P

+  protected function getSourcePlugin($plugin_id, array $configuration = []) {
...
+    // Source plugins are tightly coupled to migration entities, so we need
+    // to create a fake migration in order to properly initialize the plugin.
+    $values = [
+      'id' => uniqid(),
+      'source' => $configuration,
+      // Since this isn't a real migration, we don't want a real destination --
+      // the 'null' destination is perfect for this.
+      'destination' => [
+        'plugin' => 'null',
+      ],
+    ];
+    return Migration::create($values)->getSourcePlugin();
+  }

Ugh. Adam explained this is caused the close coupling between source plugins and migrations. There's an issue to clean this up at #2543536: [meta] Reduce/remove tight coupling of migration classes though it's quite invasive and may be D9 material at this point.

So those things aside, I couldn't find anything to complain about. Sounds like this results in a much more predictable Migration API for everyone, and has +1s from benjy, EclipseGc, mikeryan, etc.

Therefore!

Committed and pushed to 8.0.x. WOOHOO! :)

Oops, the d6_node:* dependency in d6_comment.yml got lost in the final patch, quick little patch to fix it on its way once I complete a manual test.

This works with migrate_upgrade.

Can we not test for this stuff? Seems the d6_comment template is broken without it?

#66 should be a separate issue and have tests.

@mikeryan I have trouble with config dependencies in #2550309: Unable to import migration config-entities generated by migrate_upgrade module. Maybe thats related to your patch in #66?

I noticed the change record was not published, so I published it.