Make migrations themselves plugins instead of config entities

IMHO this is a 8.1.x change, because well, 8.0.x is not meant to be for big changes. We have 5 months which is not a long time anyway, but more important, I think we all just care about getting something done, not whether its available directly.

A guarded +1 for this. I wrote some proof-of-concept code on GitHub to show that we can keep our existing YAML files as-is while quietly leveraging the plugin system and moving away from entities. I'm glad the builder system will be gone -- it was always a hack, and it solves the same problem that derivers do.

Overall I think this will allow us to shed a lot of existing code and make the migration system more understandable. @EclipseGc originally gave me the idea :)

I also think it's 8.1.x.

The only caveat is it'd be good to ensure that migration source and destinations between modules don't need any changes between 8.0.x and 8.1. so that modules only have to port once.

For actual migrations I'm more concerned with not breaking early use of migrations on 8.0.x in patch releases than keeping 8.0.x and 8.1.x in sync.

Wait, isn't one of the points of experimental modules that we can still make bigger changes however we want? We did that before the 8 release to migrate out of bounds of whatever process state for the rest of core. Should experimental modules now be in the same process bounds as the rest of core? (Bojhan just proposed last weekend that experimental modules be used to get fast feedback on UX changes / additions in core. If they are bound to a 6 month release / change cycle like this issue suggests, then that does not sound possible to me).

Contrib modules will hopefully be starting to write migration sources and destinations, and we know that people are using migrate in the wild to upgrade from 6.x-8.x regardless of experimental status.

Those are two of the main things that are going to ensure that when migrate stops being experimental, that people will be able to successfully upgrade sites - so we should try not to break them in patch releases.

So if the changes here come with a backwards compatibility layer, then I'd not have a problem committing them to 8.0.x - means less divergence and means that modules don't have to port twice. Then because it's @internal/experimental, we could remove that backwards compatibility layer in 8.1.x if we want to.

If we absolutely had to break things in a patch release by adding a method to an interface because it's necessary to fix a bug, then that's a different discussion to have. I also think new interfaces/modules that we might add as experimental are a very different category - there's not going to be an API exposed to contrib modules or the urgency to use them in production un the same way.

So moving back to 8.0.x for now - if there are changes that will actually break contrib modules and migrations we can split them out.

I spoke with chx about this when he started but I just wanted to comment publicly that i'm +1 here. I think removing the migration_templates which is a home grown migrate concept is a big win.

It would also be nice to get rid of builders although that might be more tricky. We'll likely have to go back to run-time generation of the sub-migrations, similar to how the load plugins used to work.

I disagree with committing this to 8.0.x; it completely breaks semver. We should commit the change to 8.1.x as soon as it's ready, but not to 8.0.x. The experimental designation is protection for the 8.0 to 8.1 upgrade, not a license to break semver in patch releases.

Just to reiterate "commit it to 8.1.x" does not mean "later". It means right now. 8.1.x is our development branch and we are committed to improving migrate right now.

I'm in favor of this being an 8.1 target. Moving off the entity system is a big enough change to Migrate (mostly under the hood, to be sure, but still a major change) that it doesn't seem appropriate to me to put it into 8.0.x.

I think we should yes, if this is going into 8.1 we can just write a change record here with the upgrade steps required. Obviously we're trying to keep it simple but updating some use statements falls within that for me.

So, I'm skeptical of making this change, but want to keep an open mind - @chx, please update when your sandbox is stable, and I'll try updating migrate_plus/migrate_tools/migrate_upgrade to use it and see how the DX is affected.

Specific notes:

We do lose the config entity query-ability. This does not seem to be such a hurdle as originally imagined; so far we did not need this ability for any UI, written or imagined.

We are using entityQuery() in migrate_tools, to filter on migration group (functionality provided by migrate_plus). And I for one am capable of imagining any number of query scenarios;P.

based on what core is doing I believe most everyone is shipping using templates

I hope that's not true! The template approach makes sense for the Drupal-to-Drupal case so we can, say, make multiple type-specific node migrations from one .yml file, but most custom migration scenarios should be putting migration configuration into config/install. See migrate_example on how this would normally work.

UI-wise and for BC again, we will pick up migrations from the active store probably with a deriver and an override mechanism

I'm a little vague on how migrations get into the active store in the first place, if not installed from config/install or building templates...

We'll likely have to go back to run-time generation of the sub-migrations...

That could work for the straight upgrade-as-is Drupal-to-Drupal case... But what about custom migrations? The builder creates concrete distinct migrations per content type, so you can then tweak field mappings and such for each migration.

We are using entityQuery() in migrate_tools

Yeah, migrate_ui is using that as well although I don't see why we can't just do what we did for migrate templates in MigrateTemplateStorage, that should be fine once we add some caching #2626488: MigrateTemplateStorage should implement some caching

The UI needs to save things somewhere. What about the config store? Or is it going to be the K-V store? I am wondering. Not sure yet.

Sure, a UI will save things... wherever. But what's the pattern for custom-developed migration modules? Instantiate the migration (in whatever store) in hook_install()? That's something we get for free now with config/install...

Ah, so the YAML files in the migrations directory are automatically installed, I didn't get that. Thanks.

I'm very curious about the DX we get with this change. Are there examples yet that will show the before/after picture of what converting to plugins will introduce? I'm already seeing a lot of traffic on migrate_source_csv and want to be ahead of the game if things are going to change significantly.

As far as I understand its basically moving all existing migrate.migration.yml files to a new subfolder, that's it.
Instead of then using config_devel or drush cedit or such, you'd just clear the plugin cache and have new versions of your migrations.

Nice work so far :)

1. +++ b/core/modules/field/src/Tests/Migrate/d6/MigrateFieldTest.php
   @@ -98,7 +98,7 @@ public function testFields() {
   +    $migration = $this->container->get('plugin.manager.migration')->createInstance('d6_field');
   
   +++ b/core/modules/field/src/Tests/Migrate/d7/MigrateFieldTest.php
   @@ -116,7 +116,7 @@ public function testFields() {
   +    $migration = $this->container->get('plugin.manager.migration')->createInstance('d7_field');
   

   Seems this is a common action, maybe we should add a getMigration or similar helper to the test base class?

2. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -81,7 +68,7 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
   -  protected $process = [];
   +  protected $process;
   

   bad merge.

3. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -571,28 +602,13 @@ public function getMigrationDependencies() {
   +    // While normal plugins do not change their definitions on the fly, this
   +    // one does so accomodate for that.
   

   Interesting, I guess this is a side-effect of the migration properties changing. Certainly could be un-expected that a definition could change during the life of a plugin. Guess we don't have much choice?

   Also, s/accomodate/accommodate

4. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,92 @@
   +    $plugin_ids = preg_grep('/^' . $id . ':/', array_keys($this->getDefinitions()));
   

   Does the id need a preg_quote? Not sure if plugin ids can have hyphens in their names maybe?

5. +++ b/core/modules/migrate/tests/src/Kernel/Entity/MigrationTest.php
   @@ -24,9 +24,11 @@ class MigrationTest extends KernelTestBase {
   -   * @covers ::calculateDependencies
   +   * @todo: this should be covers, fix when dependencies are fixed.
   +   * @-covers ::calculateDependencies
   ...
   +    return;
   

   debug code?

6. +++ b/core/modules/migrate_drupal/migrate_drupal.module
   @@ -19,3 +22,41 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
   +function migrate_drupal_migration_plugins_alter(&$definitions) {
   ...
   +        $plugin_ids = ['d6_term_node:' . $source_vid, 'd6_term_node_revision:' . $source_vid];
   +        foreach ($plugin_ids as $plugin_id) {
   

   Ouch, hopefully this won't be a common pattern but maybe we could create a follow-up to discuss other solutions?

7. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
   @@ -0,0 +1,115 @@
   +namespace Drupal\migrate_drupal\Plugin\migrate;
   +
   +
   

   Double space.

8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
   @@ -0,0 +1,115 @@
   +        catch (RequirementsException $e) {
   +          // Kill the rest of the method.
   +          $source_plugin = [];
   +        }
   

   Will this bubble up somewhere else?

9. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D6NodeDeriver.php
   @@ -0,0 +1,155 @@
   +    $source_plugin = static::getSourcePlugin('d6_field_instance');
   ...
   +      $source_plugin->checkRequirements();
   ...
   +      // Don't do anything; $fields will be empty.
   ...
   +      foreach (static::getSourcePlugin('d6_node_type') as $row) {
   

   We squash the requirement errors? Should we at least log these? If you had a schema mismatch, it would be hard to know?

   Also, we don't call checkRequirements on the d6_node_type source?

10. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,155 @@
    +    catch (\Exception $e) {
    +
    +    }
    

    Can we comment what exceptions we're squashing here.

11. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D7NodeDeriver.php
    @@ -0,0 +1,111 @@
    +      foreach (D6NodeDeriver::getSourcePlugin('d7_node_type') as $row) {
    +        $node_type = $row->getSourceProperty('type');
    +        $values = $base_plugin_definition;
    +        $derivative_id = $node_type;
    ...
    +        if (isset($fields[$node_type])) {
    +          foreach ($fields[$node_type] as $field_name => $info) {
    

    All this looks very similar to the D6 version, shall we add a base class?

12. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
    @@ -58,6 +58,7 @@ protected function loadFixture($path) {
    +    return;
    

    Debug?

13. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6TestBase.php
    @@ -61,9 +59,10 @@ protected function migrateUsers($include_pictures = TRUE) {
    +        #$manager->createInstance($id)->delete();
    

    Not sure what this means to delete the plugin instance? But we should remove the comment or something?

14. +++ b/core/modules/migrate_drupal/src/Tests/dependencies/MigrateDependenciesTest.php
    @@ -28,24 +27,26 @@ class MigrateDependenciesTest extends MigrateDrupal6TestBase {
    +    $migration_items = array('d6_comment', 'd6_filter_format', 'd6_node:page');
    +    // @TODO: fix when dependencies are working again.
    +    return;
    

    Debug code.

15. +++ b/core/modules/migrate_drupal/src/Tests/dependencies/MigrateDependenciesTest.php
    @@ -67,8 +68,10 @@ public function testMigrateDependenciesOrder() {
    +    // @TODO: fix when dependencies are working again.
    +    return;
    

    and here.

16. +++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeTest.php
    @@ -52,6 +57,25 @@ public function testNode() {
    +    // Test that link fields are migrated.
    +    $this->assertIdentical('https://www.drupal.org/project/drupal', $node->field_test_link->uri);
    

    Thanks for bringing this test coverage back!

17. +++ b/core/modules/user/src/Plugin/migrate/ProfileValues.php
    @@ -0,0 +1,49 @@
    +      catch (\Exception $e) {
    +      }
    

    Comments here would be good.

18. +++ b/core/modules/user/src/Plugin/migrate/User.php
    @@ -0,0 +1,60 @@
    +      if (\Drupal::moduleHandler()->moduleExists('field')) {
    

    Can we inject this into the migrations now.

I've added issues to POC this against my contrib modules: #2667366: Make migrate_plus work with Drupal 8.1.x, #2667368: Make migrate_tools work with Drupal 8.1.x, #2667370: POC of migrations-as-plugins. Hopefully, this coming weekend I'll have some time to see how it affects in particular migrate_plus (and the infamous beer example) and migrate_tools.

Could we have an explicit list of anticipated benefits in the issue summary?

Thanks.

This is looking great, few doc fixes and problems noted below. Tried to ignore any existing problems in code that was just moved, looking forward to the follow-ups where we can get rid of many uses of \Drupal, seems we have a lot from the config entity approach.

1. +++ b/core/modules/migrate/src/Entity/MigrationInterface.php
   @@ -99,6 +97,8 @@
   +  public function id();
   

   Missing docs.

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -186,7 +186,7 @@ public function import() {
   -      $this->migration->checkRequirements();
   +      #$this->migration->checkRequirements();
   

   Should this be un-commented? Or, add a TODO?

3. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -0,0 +1,614 @@
   + *
   

   Extra new line.

4. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -0,0 +1,614 @@
   +   * Gets the plugin_id of the plugin instance.
   +   *
   +   * @return string
   +   *   The plugin_id of the plugin instance.
   +   */
   +  public function id() {
   

   We can just move this to the interface and inheritdoc as mentioned in 1

5. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
   @@ -150,7 +149,7 @@ protected function prepareMigrations(array $id_mappings) {
   +      $this->migration = $this->container->get('plugin.manager.migration')->createInstance($migration);
   
   @@ -215,7 +218,7 @@ public function stopCollectingMessages() {
   +      $migration = $this->container->get('plugin.manager.migration')->createInstance($migration);
   

   We may as well use getMigration() where we can.

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
   @@ -0,0 +1,114 @@
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigratePluginManager $cck_manager, PluginManagerInterface $migration_plugin_manager) {
   

   $migratio_plugin_manager is type-hinted with the wrong interface. We have a migrate specific one.

7. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
   @@ -58,6 +58,7 @@ protected function loadFixture($path) {
   +    return;
   

   Add a todo here.

8. +++ b/core/modules/taxonomy/src/Plugin/migrate/D6TermNodeDeriver.php
   @@ -0,0 +1,58 @@
   +    catch (\Exception $e) {
   +
   +    }
   

   Need a comment here.

9. +++ b/core/modules/user/src/Plugin/migrate/User.php
   @@ -0,0 +1,60 @@
   +      catch (RequirementsException $e) {
   +      }
   

   Comment here as well please. Why do we hide the requirement failures.

Spoke through a few more points on IRC, hard to review anymore with the excitement of getting rid of templates. My main concern was removing the dependency handling but chx plans on working on those right after this issue and the current patch is already big enough. RTBC from me.

There has been some discussion around using change records as some sort of replacement for changelog.txt. Given that maybe this issue is a big enough change to be mentioned?

It'd be great to hear from @mikeryan again after the comment #48:

Hopefully, this coming weekend I'll have some time to see how it affects in particular migrate_plus (and the infamous beer example) and migrate_tools.

Especially as @mikeryan has been a supporter of keeping migrate in config.

BTW as maintainer of a lot of the config code - I'm super happy by this approach - I've not yet reviewed the code in detail.

At least https://www.drupal.org/node/2527568 should be updated cos templates are useless now

Overall looks great, mostly nits but some require to be added to CR

1. +++ b/core/modules/field/migration_templates/d6_field.yml
   @@ -2,9 +2,8 @@ id: d6_field
   -builder:
   -  plugin: d6_cck_migration
   -  cck_plugin_method: processField
   +class: Drupal\migrate_drupal\Plugin\migrate\CckMigration
   +cck_plugin_method: processField
   

   this should be added to change record as before-after

2. +++ b/core/modules/field/src/Tests/Migrate/d6/MigrateFieldTest.php
   @@ -98,7 +98,7 @@ public function testFields() {
   -    $migration = Migration::load('d6_field');
   +    $migration = $this->getMigration('d6_field');
   
   +++ b/core/modules/field/src/Tests/Migrate/d7/MigrateFieldTest.php
   @@ -116,8 +115,9 @@ public function testFields() {
   -    $migration = Migration::load('d7_field');
   +    $migration = $this->getMigration('d7_field');
   

   is this changes in scope?

3. +++ b/core/modules/file/src/Tests/Migrate/d7/MigrateFileTest.php
   @@ -34,8 +34,8 @@ protected function setUp() {
   -    /** @var \Drupal\migrate\Entity\MigrationInterface $migration */
   -    $migration = entity_load('migration', 'd7_file');
   +    /** @var \Drupal\migrate\Plugin\Migration $migration */
   +    $migration = $this->getMigration('d7_file');
   

   once there's no more entity_load() then change record should point a way to retrieve migrations

4. +++ b/core/modules/migrate/migrate.services.yml
   @@ -26,3 +26,6 @@ services:
   +  plugin.manager.migration:
   +    class: Drupal\migrate\Plugin\MigrationPluginManager
   

   probably this one should be used to load them.
   Is there any helpers for?

5. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -186,7 +186,8 @@ public function import() {
   -      $this->migration->checkRequirements();
   +      // @TODO https://www.drupal.org/node/2666640
   +      #$this->migration->checkRequirements();
   

   Why that code commented with hash?
   the related issue does not describe what to do

6. +++ b/core/modules/migrate/src/Plugin/MigrationConfigDeriver.php
   @@ -0,0 +1,94 @@
   +  public static function save($migration_id, $definition, PluginManagerInterface $manager = NULL, ConfigFactoryInterface $config_factory = NULL) {
   +    if (!$manager) {
   +      $manager = \Drupal::service('plugin.manager.migration');
   +    }
   +    if (!$config_factory) {
   +      $config_factory = \Drupal::configFactory();
   
   +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,91 @@
   +class MigrationPluginManager extends DefaultPluginManager implements MigrationPluginManagerInterface {
   

   strange way to pass deps, but arguments whould be properly type-hinted at least for MigrationPluginManagerInterface
   Why they optional?

7. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
   @@ -168,8 +167,12 @@ protected function executeMigration($migration) {
   -    $migrations = Migration::loadMultiple($ids);
   -    array_walk($migrations, [$this, 'executeMigration']);
   +    $manager = $this->container->get('plugin.manager.migration');
   +    array_walk($ids, function ($id) use ($manager) {
   +      // This is possibly a base plugin id and we want to run all derivatives.
   +      $instances = $manager->createInstances($id);
   

   This should be in change record too, new way to get migrations!

In an ideal world this issue would be 100% complete on commit - however it does make sense to cleanups in separate issues as longs this issue contains all the API changes.

1. I think we need to remove Drupal\migrate\Entity\MigrationInterface - as the issue summary hints at a good idea would be to do this is a followup. However I think we should put the correct interface in place in the issue and just make Drupal\migrate\Entity\MigrationInterface extend from it and add a @todo to remove it.

2. grep -R "Drupal\\\migrate\\\Entity\\\Migration\W" ./core
   ./core/modules/comment/src/Tests/Migrate/d7/MigrateCommentTypeTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/field/src/Tests/Migrate/d6/MigrateFieldFormatterSettingsTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/field/src/Tests/Migrate/d6/MigrateFieldTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadEntityDisplayTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadEntityFormDisplayTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadFieldTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadInstanceTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/image/src/Tests/Migrate/d6/MigrateImageCacheTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/migrate/migrate.api.php: * \Drupal\migrate\Entity\Migration, with interface
   ./core/modules/migrate/src/MigrationBuilder.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/migrate/src/Plugin/migrate/builder/BuilderBase.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckMigration.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/node/src/Plugin/migrate/builder/d6/Node.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/node/src/Plugin/migrate/builder/d7/Node.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/node/src/Tests/Migrate/d6/MigrateNodeTypeTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/node/src/Tests/Migrate/d6/MigrateViewModesTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/node/src/Tests/Migrate/d7/NodeBuilderTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/system/src/Tests/Migrate/MigrateMenuTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/taxonomy/src/Plugin/migrate/builder/d6/TermNode.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateTaxonomyVocabularyTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyEntityDisplayTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyEntityFormDisplayTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyFieldInstanceTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyFieldTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/user/src/Plugin/migrate/builder/d6/ProfileValues.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/user/src/Plugin/migrate/builder/d7/User.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/user/src/Tests/Migrate/d6/MigrateUserPictureFileTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/user/src/Tests/Migrate/d6/MigrateUserRoleTest.php:use Drupal\migrate\Entity\Migration;
   ./core/modules/user/src/Tests/Migrate/d6/MigrateUserTest.php:use Drupal\migrate\Entity\Migration;
   

   This class no longer exists Drupal\migrate\Entity\Migration

3. +++ b/core/modules/file/src/Tests/Migrate/d6/FileMigrationTestTrait.php
   @@ -0,0 +1,32 @@
   + * Contains
   

   Missing the rest of it... mind you these are all being removed.

4. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -186,7 +186,8 @@ public function import() {
   +      #$this->migration->checkRequirements();
   

   I think commenting out this is a mistake. Looking at Drupal\migrate\Plugin\Migration::checkRequirements() - that's still doing an entity load multiple.

5. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -252,6 +232,53 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
      /**
   +   * Construct a migration plugin.
   +   *
   +   * @param array $configuration
   +   *   A configuration array containing information about the plugin instance.
   +   * @param string $plugin_id
   +   *   The plugin_id for the plugin instance.
   +   * @param mixed $plugin_definition
   +   *   The plugin implementation definition.
   +   */
   +  public function __construct(array $configuration, $plugin_id, array $plugin_definition) {
   

   I don't think we can typehint $plugin_definition as there have been moves to make this an object. It is mixed in the docblock. Also isn't {@inheritdoc} appropriate here?

6. +++ b/core/modules/migrate/src/Plugin/MigrationConfigDeriver.php
   @@ -0,0 +1,94 @@
   +/**
   + * Expose migrations in the active config store as derivatives.
   + */
   +class MigrationConfigDeriver extends DeriverBase implements ContainerDeriverInterface {
   

   Does this whole thing exist as a bc layer for customised migrations? What happens when existing sites have a config for an existing plugin? And does this class have any test coverage?

7. +++ b/core/modules/migrate/src/Plugin/MigrationConfigDeriver.php
   @@ -0,0 +1,94 @@
   +      $prefix = 'migrate.migration.';
   

   So as the entity no longer exists with the prefix 'migrate.migration.' this is invalid configuration. What happens when the migrate UI reintroduces the config entity?

8. +++ b/core/modules/migrate/src/Plugin/MigrationYamlDiscovery.php
   @@ -0,0 +1,92 @@
   +        $contents = Yaml::decode(file_get_contents($file)) ?: [];
   

   We need to catch exceptions here and re-throw with the information of what file has caused the problem - otherwise this can be hard to debug. (And yes we should do this in YamlDiscovery too - but not in this issue.)

9. +++ b/core/modules/migrate/src/Plugin/MigrationYamlDiscovery.php
   @@ -0,0 +1,92 @@
   +      foreach (['migration_templates', 'migrations'] as $subdirectory) {
   

   Is the fact that both migration_templates and migrations are scanned tested anywhere? Also I think this could be clearly documented on the class doc.

10. +++ b/core/modules/migrate/tests/src/Unit/process/MigrationTest.php
    @@ -27,6 +27,9 @@ class MigrationTest extends MigrateProcessTestCase {
    +    // @TODO https://www.drupal.org/node/2667620
    +    $this->assertSame(TRUE, TRUE);
    +    return;
    
    @@ -55,6 +58,9 @@ public function testTransformWithStubSkipping() {
    +    // @TODO https://www.drupal.org/node/2667620
    +    $this->assertSame(TRUE, TRUE);
    +    return;
    

    This does not look good - I think we should try to make sure this test coverage is fixed in this issue.

11. +++ b/core/modules/migrate_drupal/migrate_drupal.module
    @@ -19,3 +22,41 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
    +  $vocabulary_migration_definition = [
    +    'source' => [
    +      'ignore_map' => TRUE,
    +      'plugin' => 'd6_taxonomy_vocabulary',
    +    ],
    +    'destination' => [
    +      'plugin' => 'null',
    +    ],
    +  ];
    +  $vocabulary_migration = new Migration([], uniqid(), $vocabulary_migration_definition);
    +  $executable = new MigrateExecutable($vocabulary_migration, new MigrateMessage());
    +  // This is why the deriver can't do this: the 'd6_taxonomy_vocabulary'
    +  // definition is not available to the deriver as it is running inside
    +  // getDefinitions().
    +  if (isset($definitions['d6_taxonomy_vocabulary'])) {
    

    Can't all the stuff before the if be inside the if

12. +++ b/core/modules/migrate_drupal/migrate_drupal.module
    @@ -19,3 +22,41 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
    +    }
    +    catch (\Exception $e) {
    +
    +    }
    

    Why are exceptions being thrown and why are we catching the generic exception and saying nothing - looks dangerous.

13. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,155 @@
    +    }
    +    catch (\Exception $e) {
    +      // @TODO https://www.drupal.org/node/2666640
    +    }
    

    Catching the generic exception and doing nothing looks super dangerous. Apparently this one is something about dependencies.

14. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D7NodeDeriver.php
    @@ -0,0 +1,111 @@
    +      foreach (D6NodeDeriver::getSourcePlugin('d7_field_instance') as $row) {
    ...
    +      foreach (D6NodeDeriver::getSourcePlugin('d7_node_type') as $row) {
    

    This looks like an abstract NodeDeriverBase could be useful. Or maybe even a more generic trait.

15. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
    @@ -58,6 +58,8 @@ protected function loadFixture($path) {
       protected function installMigrations($version) {
    +    // @TODO https://www.drupal.org/node/2668436
    +    return;
    

    Does this need a separate issue? It looks like installMigrations can just be removed.

16. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6TestBase.php
    @@ -61,9 +59,11 @@ protected function migrateUsers($include_pictures = TRUE) {
    -      Migration::load('d6_user_picture_file')->delete();
    -      Migration::load('user_picture_entity_display')->delete();
    -      Migration::load('user_picture_entity_form_display')->delete();
    +      $manager = $this->container->get('plugin.manager.migration');
    +      foreach (['d6_user_picture_file', 'user_picture_entity_display', 'user_picture_entity_form_display'] as $id) {
    +        // @TODO https://www.drupal.org/node/2666648
    +        #$manager->createInstance($id)->delete();
    +      }
    

    I think we can just delete this code and move on.

17. +++ b/core/modules/migrate_drupal/src/Tests/dependencies/MigrateDependenciesTest.php
    @@ -28,24 +27,26 @@ class MigrateDependenciesTest extends MigrateDrupal6TestBase {
    +    $migration_items = array('d6_comment', 'd6_filter_format', 'd6_node:page');
    +    // @TODO https://www.drupal.org/node/2666640
    +    return;
    

    I'm really wary of leaving the dependency / requirements work till later.

18. +++ b/core/modules/taxonomy/src/Plugin/migrate/D6TermNodeDeriver.php
    @@ -0,0 +1,58 @@
    +    catch (\Exception $e) {
    +      // @TODO https://www.drupal.org/node/2666640
    +    }
    

    This catch looks dangerous too.

I did start working on #2667366: Make migrate_plus work with Drupal 8.1.x (migrate_plus) and #2667368: Make migrate_tools work with Drupal 8.1.x (migrate_tools) with this patch yesterday, didn't have time to write up what my experience was so far until tonight...

The migrate_example module was simple enough - just moved the migration .yml files from config/install to the migrations directory (something that definitely needs to be in the change record). Well, I also had to hack the connection key into each migration, until I figure out how to implement the merge-the-migration-group's-config-into-each-migration with hook_migration_plugins(). But, pleased that at least a simple migration scenario works with no changes of substance.

First pass at implementing drush migrate-status, replacing the EntityQuery with plugin discovery, died a horrible death:

~/Sites/d8/modules/contrib/migrate_plus$ drush ms
exception 'Drupal\Component\Plugin\Exception\InvalidDeriverException' with message 'Plugin (d6_node) deriver "Drupal\migrate_drupal\Plugin\migrate\D6NodeDeriver" does not exist.' in         [error]
/Users/mryan/Sites/d8/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php:213

The problem here being that the d6_node migration plugin in the node module is referencing the D6NodeDeriver class in migrate_drupal, and I did not have migrate_drupal enabled. chx feels that all the Drupal migration plugins should be in migrate_drupal, although I still feel that the ideal model is that each module should be responsible for its own plugins. Anyway, either all the plugins should move to migrate_drupal, or D6NodeDeriver/D7NodeDeriver should move to the node module.

A more basic issue is, the drush migrate-status command is supposed to list any "available" (as in, "I can run them now") migrations. So, if I enable migrate_example and run drush ms, I expect to see the four migrations beer_term, beer_user, beer_node, and beer_comment listed. This is how it currently works - the migrations of interest to migrate-status are the active configuration entities, easily discovered with an entityQuery(). If I were doing a Drupal upgrade, the configuration process would create only those configuration entities that are relevant to the particular site being migrated (this is why we have templates), and migrate-status equally easily finds those.

So, I'm struggling to see how that works with migrations-as-plugins. Plugin-based discovery is going to discover all migration plugins - i.e., all the migrations defined in all enabled core modules. It's not clear to me how to filter out all the irrelevant plugins. This would apply in the upgrade use case as well - if I don't have the aggregator module enabled on my D6 site, I don't want to run the d6_aggregator_item (not to mention the d7_aggregator_item) migration when upgrading. Is this an issue that's going to be addressed in the dependency follow-up?

Apart from figuring out how that will work, though, the only change to the drush commands was to replace Migration::load($migration_id) calls with \Drupal::service('plugin.manager.migration')->createInstance($migration_id), so that's encouraging. The UI, based on ConfigEntityListBuilder and EntityForm, is going to be more work though.

The next step will be figuring out how this applies to the upgrade process (#2667370: POC of migrations-as-plugins). I won't have a good chunk of time to dive into that before this weekend (if even then), and I'm actually curious how chx and benjy see the plugin approach working in practice here (hint hint, if one of you has time to try a patch in the next few days...).

Also when I have a chance, I need to understand derivers better... I'm not quite clear on how one will, say, customize field mappings on a per-node-type basis as builders support now.

There is other contrib that might want the YAML discovery of one plugin per file. I think it is worth splitting that out into another patch so we can add tests and generalise.

The problem here being that the d6_node migration plugin in the node module is referencing the D6NodeDeriver class in migrate_drupal, and I did not have migrate_drupal enabled. chx feels that all the Drupal migration plugins should be in migrate_drupal, although I still feel that the ideal model is that each module should be responsible for its own plugins. Anyway, either all the plugins should move to migrate_drupal, or D6NodeDeriver/D7NodeDeriver should move to the node module.

This is already a problem in HEAD, see #2560795: Source plugins have a hidden dependency on migrate_drupal and #2600926: Allow annotations to inherit across namespaces

I've created #2671034: Create plugin per file YAML discovery to handle the generic plugin discovery.

Also I've discussed this issue with @catch. Whilst I'm disappointed that d.o does not yet support feature branches and that committing this will regress existing functionality, I concede that managing this in a mega-patch will be difficult. Therefore let's punt the dependency and checkRequirements stuff to a followup patch that should be a major and tagged with "migrate critical".

However I still think we need to address everything else from #71 and subsequent reviews.

I think we need to remove the entire config integration as providing this should be the remit of the module that provides the config entity type that configures migrate plugins. For me, leaving this in core is dangerous (because what about existing migration config in a live site) and confusing.

I think we need to remove the entire config integration as providing this should be the remit of the module that provides the config entity type that configures migrate plugins. For me, leaving this in core is dangerous (because what about existing migration config in a live site) and confusing.

I've been thinking about this and I think that does make sense. The config isn't needed to run the migrations, it is only really needed to support the tooling outside of core so that could quite easily be done from contrib. I could put the ConfigEntity definition into http://drupal.org/project/migrate_api which is a module specifically designed to be a dependency for migrate-contrib tooling.

Reviewing the interdiffs they look good to me, shouldn't we also remove all the schema to do with the config entity here?

@benjy yep you are right - core shouldn't have any remnant of the migrations as config entities at all.

Using the new #2671034: Create plugin per file YAML discovery and fixing some coding standards stuff.

I believes all of alexpott's feedback from #71 has been done and the per plugin file YAML patch was merged so back to RTBC.

The biggest worry about going forward here is the affect on the Drupal Upgrade module ie. #2667370: POC of migrations-as-plugins as that is the module we want to put into 8.1.0 as an experimental module.

I don't think the derivers should be reaching into the D6NodeDeriver to get code they need - let's encapsulate that in a trait.

@chx I was playing around with idea of move them but I didn;t mean to post a patch that did - sorry. I'm torn about the location of the derivers - but I can't work out what to do with the cck deriver and service. I guess we could work some of this out with the dependency stuff. But it feels off to move this stuff about again.

Interdiff with the last working patch - #84.

Ok It is official I can't roll patches... so as the D*NodeDerivers obviously want me to move them and this time I've run the tests locally. They definitely should be part of node and not migrate Drupal - and then we only have the cck one to work what to do with.

The latest patches just moved stuff about.

I'm going to commit this now to give us the chance to get #2666640: Readd dependencies and such to migration done and then make the necessary changes to migrate_drupal and get that in core as an experimental module. There is a chance that this patch will be reverted if we decide that getting migrate_drupal in core is more important. However at least doing this now gives us a chance to do this.

Committed 694994f and pushed to 8.1.x. Thanks!

Three of the follow-ups are green if someone wants to review:

#2666640: Readd dependencies and such to migration
#2676258: Nuke builder plugins and migration storage
#2667620: Rewrite Drupal\Tests\migrate\Unit\process\MigrationTest

Sorry folks but we need to revert this, for two reasons:

1. The beta for 8.1.x begins Wednesday, and as previously stated, Migrate UI is one of the top three release priorities for 8.1.0. The UI was very close to ready for a core commit prior to this patch. (I was told previously that this change would not affect existing migrations or the work being done on the UI, which apparently isn't correct.)
2. As far as I understand from @mikeryan and the followups above, this introduces critical regressions in e.g. migration dependencies, which does not seem consistent with beta-stability code (see #2656994: Experimental modules should have their own version numbers). Migrate is still experimental, but it's something of a special case also. So based on Dries' "Always Be Shippable" blog post and the related DrupalCon Barcelona keynote, I think this important API work needs to be added to the main branch as a complete thing so that the train can leave the station appropriately for the minors.

Based on the followups above, it sounds like this might actually be a case for an official feature branch of Drupal 8 in the core repository, despite Migrate being experimental. We also should do what we can to make sure this work can go forward too and potentially land in 8.2.x soon.

Also, I think this issue will need subsystem, framework, and release manager signoff to go in, because it significantly impacts the subsystem, the architecture, and the release timeline and goals. @alexpott's commit is an implicit framework manager signoff, but we should probably discuss more given that I'm reverting this for release management reasons. It also sounds as though there was not consensus within the Migrate subsystem maintainers so that needs to be sorted too.

Finally, @alexpott previously suggested that with this being outstanding, we should consider marking Migrate itself alpha instead of beta. That's something we can address in #2656994: Experimental modules should have their own version numbers.

Yeah this is really disappointing, especially given I spent most the weekend working on the follow-ups including the dependency patch which is pretty much ready.

I pinged alexpott offering to chip-in with any changes that would be required with the UI off the back of this issue, even though, I really didn't want anything to do with it.

It also sounds as though there was not consensus within the Migrate subsystem maintainers so that needs to be sorted too.

I don't know where those sounds are coming from, it would be nice if they spoke up directly to another migrate maintainer in #drupal-migrate

Just a note that we’d like to not add major new functionality during the beta, like migrate UI, so not breaking that just before beta1 was important.

However since this is clean-up and migrate will be in alpha generally, it should be fine to be re-committed during the beta - we just don’t want to break migrate UI or other key functionality at the same time.

Re-rolled, hopefully tests will run at least.

I've re-rolled again, the last re-roll left the migration class in the entity namespace which of course broke every test.

I had to bring in a a few changes from the dependency patch so we can load multiple migrations at once, I also added a way to load migrations based on a tag to the plugin manager as that was a feature of the entity storage. Most of the changes were against the migrate_drupal_ui, which needs some attention in general.

I had a failure on the UI test locally regarding not being able to set the "driver", i'm wondering if that's because I only have mysql available.

I think we can probably remove some of the "Needs xyz review" tags from this issue now? I'll let someone else do that.

OK, this is all I have time for tonight. It fixes all the obvious issues, but the UI tests are still failing. Didn't look into why and the UI just swallows the errors so there is nothing in the simpletest output.

OK, this brings back the dependencies stuff which I wrote in #2666640: Readd dependencies and such to migration

Fixing some of the fails...

So we have a chicken and egg situation - we need the source credential whilst instantiating the plugins but in the previous patch they were only set on the plugin after...

Fixing one of the test fails

I'm not sure what is going on in the file source but I removed that for now, it doesn't make any sense to me, its using the destination config to do something in the source but then I can't see anything been done with that data until the destination anyway? Alas, it will need some manual testing and inspection before commit.

The MigrationCreationTrait was manually calling checkRequirements() on the source and the destination rather than calling checkRequirements() on the migration itself?

These migrations are still failing:

Migration user_profile_field did not meet the requirements. Missing source provider profile source_provider: profile.
Migration user_profile_field_instance did not meet the requirements. Missing source provider profile source_provider: profile.
Migration user_profile_entity_display did not meet the requirements. Missing source provider profile source_provider: profile.

A backtrace from DrupalSqlBase::checkRequirements() would probably help figure these out.

The 10 files in the d6 migration is interesting... the files table in D6 looks like this...

+-----+-----+--------------------+----------------------------------------------+------------+----------+--------+------------+
| fid | uid | filename           | filepath                                     | filemime   | filesize | status | timestamp  |
+-----+-----+--------------------+----------------------------------------------+------------+----------+--------+------------+
|   1 |   1 | Image1.png         | core/modules/simpletest/files/image-1.png    | image/png  |    39325 |      1 | 1388880660 |
|   2 |   1 | Image2.jpg         | core/modules/simpletest/files/image-2.jpg    | image/jpeg |     1831 |      1 | 1388880664 |
|   3 |   1 | Image-test.gif     | core/modules/simpletest/files/image-test.gif | image/jpeg |      183 |      1 | 1388880668 |
|   5 |   1 | html-1.txt         | core/modules/simpletest/files/html-1.txt     | text/plain |       24 |      1 | 1420858106 |
|   6 |   1 | some-temp-file.jpg | /tmp/some-temp-file.jpg                      | image/jpeg |       24 |      0 | 1420858106 |
+-----+-----+--------------------+----------------------------------------------+------------+----------+--------+------------+

So 4 is the correct number as temp files are not migrated.

  public function getDerivativeDefinitions($base_plugin_definition) {
    // Read all CCK field instance definitions in the source database.
    $fields = array();
    $source_plugin = static::getSourcePlugin('d6_field_instance');
    try {
      $source_plugin->checkRequirements();

      foreach ($source_plugin as $row) {
        $fields[$row->getSourceProperty('type_name')][$row->getSourceProperty('field_name')] = $row->getSource();
      }
    }
    catch (RequirementsException $e) {
      // Don't do anything; $fields will be empty.
    }

This code is problematic because if we didn't add the database fallback thing that the source stuff needs to be somehow set on the source plugin created here.

The committers discussed this earlier in the week and as catch suggests in #101 we'd like to still try to get this into 8.1.x, especially if it can go in without regressions before a second beta. So moving back to the 8.1.x queue for visibility.

So 4 is the correct number as temp files are not migrated.

What makes you say temporary files are not migrated?

Yeah that files issue is a good one, heres what it looks like:

The same issue has been mentioned by others in IRC and on the support forums: https://www.drupal.org/node/2681475 so its a pre-existing in HEAD. You can see the issue in the image, it shows files belonging to other users, and the weird ones are the picture files for users who had a empty picture in the source data.

I can't explain yet how this ever passed unless for some reason, the d6_user_picture_file migration was not running previously which would imply an issue with dependencies.

I've opened #2681505: Arbitrary files are created when migrating users with an empty picture although the UI tests fail there as well with Found 0 file entities, expected 4. because the UI was never really working for files and it just so happened that the UI got four bogus profile files rather than the 4 actual files it was expecting. See screenshot after running the Upgrade UI test.

OK, this fixes MigrateUpgrade6Test, it includes #2681505: Arbitrary files are created when migrating users with an empty picture as well and you can see from the image in #125, that 6 is the correct amount of files to be asserting for.

@benjy the problem we have is that we're still just relying on the state fallback for the database - which in itself feels wrong. We're at an interesting place there is a tiny amount of configuration that is necessary for migrations... the source database and the source base path... and actually the list of migrations to perform.

@alexpott, yes I understand that, but that doesn't take away from the files issue i fixed in #127? Can we commit this with the fallback in place here and then remove it in a follow-up or do you want to do it all here?

Well the fallback will work for the database but for the source base path?

The only quick workaround I can think of is hard-coding the logic currently in getMigrations() to do with the source base path into the batch runner when the migrations are created :/

This patch should be green, fixed an issue in migration creation trait. Also reverted back to just manually checking the requirements on the source/destination which we should create a follow-up to fix.

+++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
@@ -91,7 +91,9 @@ public function prepareRow(Row $row) {
-    $path = str_replace($this->migration->get('destination')['source_base_path'], NULL, $path);
+
+    // @TODO, figure out why this is done here and not the destination.
+    // $path = str_replace($this->migration->get('destination')['source_base_path'], NULL, $path);

We need to remove this hack though...

Yes, this line of code is confusing and obviously doesn't have any test coverage. The destination simply sticks the two values back together before doing anything with it: $source = $this->configuration['source_base_path'] . $file;

I actually think we should just delete that line in the source and then add a new test to ensure that source_base_path gets into the destination and finally hardcode that logic for entity:file I mentioned above, into the batch runner when it does a createInstances(). That will get us finished here and we can fix properly in a follow-up?

OK, i've reverted the change in the D7 file source, we can fix that in #2577871: d7_file plugin should receive source_base_path configuration option

I've created an issue for the checkRequirements() calls I had to revert in the last patch: #2681867: Drupal Upgrade UI should validate the entire migration, not the source and destination individually

I've created #2681869: Provide clean way to merge configuration into migration plugins to remove the manual setting of the destination config in the batch runner and generically solve how we're going to allow sources and destinations to get their requirements.

I had to fix the source base path in the D6 test, that was why no files were been migrated for the D6 tests, as i reported in #2681539: MigrateUpgrade6Test does not migrate files

There is certainly plenty of ugly happening with the latest fixes required to get the UI green but this patch is nearly 200kb already. If we commit as is to 8.2.x then work on all the follow-ups and once it's all ready, back port to 8.1 that would be the best way forward IMO.

Re-rolled as #2681505: Arbitrary files are created when migrating users with an empty picture has been committed. @benjy nice job on creating the followups. I've closed #2666640: Readd dependencies and such to migration as that is now redundant. The patch added does a bit of clean up - but there are still two references to #2666640: Readd dependencies and such to migration that need removing.

I also discussed the issue with @benjy on IRC. I think the state fallback for source databases and the way source base path is set are both not ideal but also HEAD is not ideal. Given that we have no regressions here I think fixing this so any migration can declare and get its requirements in #2681869: Provide clean way to merge configuration into migration plugins makes sense.

Furthermore whilst reviewing MigrateCreationTrait I've realised two things:

1. The name is not correct with this patch - at most it is configuring migrations not creating them (it was prior to this patch) - address in #2682585: Rename MigrationCreationTrait as it no longer creates migrations - it configures them
2. The methods getConnection() and getSourceDatabaseConnection() are confusing - they both return the source connection and they both do more than just getting a connection.

The latter is addressed by the patch added because getSourceDatabaseConnection() was added by it.

I think once the incorrect @todo's are addressed this patch is ready for serious review and consideration for 8.2.x. I think that in order to get this in 8.1.x-beta we need to get a certain amount of the followups done so we can cherry-pick the lot in one go. This way the patch does not put 8.1.0 at risk and can go there if we get the followups done in time BUT also the patch does not become a 500k unreviewable monster. (@benjy mentioned that doing something like is @catch's idea).

The great thing is that we've got to the point were they are no regressions over what is in HEAD for the purposes of migrate UI. benjy++

Just starting using code sniffer in the new IDE and currently get way too much info. But spotted this in all that.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
@@ -0,0 +1,114 @@
+  /**
+   * Constructs a CckMigration.
+   *
+   * @param array $configuration
+   *   Plugin configuration.
+   * @param string $plugin_id
+   *   The plugin ID.
+   * @param mixed $plugin_definition
+   *   The plugin definition.
+   * @param \Drupal\migrate\Plugin\MigratePluginManager $cck_manager
+   *   The cckfield plugin manager.
+   */

Missing @param for MigrationPluginManagerInterface $migration_plugin_manager.
And needs a use statement for the MigratePluginManager.

Patch addresses #137 and also fixes a lot of other coding standards things noticed by phpcs that are in scope.

I refined it so we're only catching a database exception now instead of all exceptions and documented why that is so. I also added a test and fixed an issue todo with submitting the UI form with invalid database credentials.

Bad assumption about the driver.

sqlite doesn't use passwords... let's just change the database and assert on migrate ui's message. Also we need to add proper requirements checking to the FieldInstance source plugins.

Thanks for fixing the sqlite issue.

I'm not sure if the tableExists checks are correct here or not, we usually rely on the source_provider for the requirement check. If the source provider exists, then so should the tables?

@benjy but these are being used in the derivers it is bit of a different situation.

Also I think we need to get #2683579: Improve migrate UI tests in first because it properly tests the source_base_path setting through the UI and we're changing how this is configured on the migrations in this patch.

I added the source_provider rather than the table exist checks. interdiff is against #141.

Rerolled...

M	core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyFieldInstanceTest.php
M	core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyFieldTest.php
Falling back to patching base and 3-way merge...

I've merged this and #2683579: Improve migrate UI tests and the UI tests are still green so we've not broken the source_base_path UI here. Depending on which issue gets in first one will need a reroll.

Wwhat I don't understand is how this patch fixes the files migrate in the UI test - ah I see...

+++ b/core/modules/migrate_drupal_ui/src/Tests/d6/MigrateUpgrade6Test.php
@@ -30,7 +30,7 @@ protected function setUp() {
-    return __DIR__ . '/files';
+    return './';

But this is the bit is not quite right... however this is fixed in #2683579: Improve migrate UI tests.

I've merged the patches locally and can confirm that the UI tests still pass.

[Edit: for clarity]

I've deleted the comment #151 because it is a red herring and distracting. So I think it is okay if #148 goes in and we fix the UI test to use a custom source base path in #2683579: Improve migrate UI tests.

Given the effort that has go into this patch I think it deserves first bite.

Started reviewing this, didn't finish a full pass. I already reviewed the original patch here and was mostly happy with it, so not much new here mostly minor.

1. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -571,28 +603,13 @@ public function getMigrationDependencies() {
        }
   

   Sentence is a bit odd, and it could do with an explanation why.

2. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,230 @@
   +   *
   +   * We need to expand any derivative migrations. Derivative migrations are
   +   * calculated by migration derivers such as D6NodeDeriver. This allows
   +   * migrations to depend on the base id and then have a dependency on all
   +   * derived migrations. For example, d6_comment depends on d6_node but after
   +   * we've expanded the dependencies it will depend on d6_node:page,
   +   * d6_node:story and so on, for derived migration.
   

   Very minor but 'derivative migrations' and 'derived migrations' are used almost interchangeably here.

3. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,230 @@
   +      $plugin_ids += preg_grep('/^' . preg_quote($id, '/') . PluginBase::DERIVATIVE_SEPARATOR . '/', array_keys($this->getDefinitions()));
   

   preg_grep()++

4. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,230 @@
   +    // Migration dependencies defined in the migration storage can be
   +    // optional or required. If an optional dependency does not run, the current
   +    // migration is still OK to go. Both optional and required dependencies
   +    // (if run at all) must run before the current migration.
   +    $dependency_graph = array();
   +    $requirement_graph = array();
   

   It might help to explicitly point out that $dependency_graph will hold all dependencies, and $requirement_graph will hold only the required ones. Would almost call $requirement_graph $required_dependency_graph.

5. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,230 @@
   +      if (isset($migration_dependencies['optional'])) {
   +        foreach ($migration_dependencies['optional'] as $dependency) {
   +          $different = TRUE;
   +          $this->addDependency($dependency_graph, $id, $dependency, $dynamic_ids);
   +        }
   +      }
   +    }
   +    $graph_object = new Graph($dependency_graph);
   +    $dependency_graph = $graph_object->searchAndSort();
   +    if ($different) {
   +      $graph_object = new Graph($requirement_graph);
   +      $requirement_graph = $graph_object->searchAndSort();
   +    }
   +    else {
   +      $requirement_graph = $dependency_graph;
   +    }
   

   I can't see why $different = TRUE has to be tracked, this looks the same as if (!empty($migration_dependencies['optional'])) - which would also be more self-explanatory.

   Also $graph_object being set twice I had to read a couple of times. I think we could do $dependency_graph = (new Graph($dependency_graph)->searchAndSort()?

6. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -0,0 +1,230 @@
   +      // Populate a weights array to use with array_multisort later.
   

   Needs the parentheses.

7. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManagerInterface.php
   @@ -0,0 +1,36 @@
   +   *   instantiated. Also accepts an array of plugin ids and an empty array to
   

   IDs

8. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManagerInterface.php
   @@ -0,0 +1,36 @@
   +   *   plugin id.
   

   ID

9. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManagerInterface.php
   @@ -0,0 +1,36 @@
   +   *   If the instance cannot be created, such as if the ID is invalid.
   

   Since it's multiple instances should this be 'If an instance cannot be created' instead of 'If the...'

10. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
    @@ -168,8 +167,12 @@ protected function executeMigration($migration) {
    +      // This is possibly a base plugin id and we want to run all derivatives.
    

    ID again.

11. +++ b/core/modules/migrate/tests/src/Unit/process/MigrationTest.php
    @@ -27,6 +27,9 @@ class MigrationTest extends MigrateProcessTestCase {
    +    // @TODO https://www.drupal.org/node/2667620
    

    What about nuking it and putting it back again? Assuming we fix the @todo soonish don't really mind though.

12. +++ b/core/modules/migrate_drupal/migrate_drupal.module
    @@ -19,3 +22,41 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
    +    try {
    +      foreach ($vocabulary_migration->getSourcePlugin() as $row) {
    +        $executable->processRow($row, $process);
    +        $source_vid = $row->getSourceProperty('vid');
    +        $plugin_ids = ['d6_term_node:' . $source_vid, 'd6_term_node_revision:' . $source_vid];
    +        foreach ($plugin_ids as $plugin_id) {
    +          if (isset($definitions[$plugin_id])) {
    +            $definitions[$plugin_id]['process'][$row->getDestinationProperty('vid')] = 'tid';
    +          }
    +        }
    +      }
    +    }
    +    catch (\Exception $e) {
    +
    +    }
    

    Shouldn't we trigger_error() or something when there's an exception?

    Also shouldn't the try/catch be for each individual row? It's not clear at all from here what the implications are of the exception being tried/caught - but if we can eat the exception, I'd expect later rows to not be blocked on it. If we can't eat it, we should at least log an error.

13. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -64,73 +64,49 @@ protected function getSystemData(array $database) {
    +   *   The drupal version.
    

    Drupal Drupal Drupal Drupal, Drupal.

14. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
    @@ -0,0 +1,116 @@
    +        }
    +        catch (RequirementsException $e) {
    +          // Kill the rest of the method.
    +          $source_plugin = [];
    +        }
    

    Again no need to log anything here?

15. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -949,30 +947,32 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +        // Store the retrieved migration ids in form storage.
    

    IDs

16. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -949,30 +947,32 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +        // Store the retrived system data in from storage.
    

    retrieved data in form storage.

17. +++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,143 @@
    +    catch (RequirementsException $e) {
    +      // If checkRequirements() failed then the content module did not exist and
    +      // we do not have any CCK fields. Therefore, $fields will be empty and
    +      // below we'll create a migration just for the node properties.
    +    }
    

    Hmm so this isn't really exceptional, just means CCK was not installed?

18. +++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,143 @@
    +    catch (DatabaseExceptionWrapper $e) {
    +      // Once we begin iterating the source plugin it is possible that the
    +      // source tables will not exist. This can happen when the
    +      // MigrationPluginManager gathers up the migration definitions but we do
    +      // not actually have a Drupal 6 source database.
    +    }
    +
    

    Shouldn't we still show an error in this case even if not fatal?

1. Fixed
2. Fixed
3. n/a
4. Renamed variable, think that makes it clearer.
5. Updated both lines of code so we no longer store the graph.
6. Fixed
7. Fixed
8. Fixed
9. Fixed
10. Fixed
11. Left in place, I already have the patch in #2667620: Rewrite Drupal\Tests\migrate\Unit\process\MigrationTest written, just needs a re-roll.
12. Not sure about this exception, will need looking into.
13. Fixed
14. When we're catching requirements exceptions, there is nothing to log. It usually means that the source site didn't have the tables/modules enabled for the source to be iterated.
15. Fixed
16. Fixed
17. Correct, see source_provider on the sources and DrupalSqlBase::checkRequirements().
18. The problem here is the D6 derivers get loaded even if you're migrating from D7, we have a follow-up to solve this better. #2560795: Source plugins have a hidden dependency on migrate_drupal

I missed the first part of 5 regarding $different, will do that next. 12 needs more looking into as I'm not sure what is being caught there. Rest is done.

Remaining feedback resolved. The exception only needed to be the requirements exception for the same reason as the derivers.

Fixed the tests after a conflict with #2225477: Add migrate sources and destinations for D6 i18n variables

Fixes the two failing tests that were introduced in the last patch. I had to catch the database exception for the exact same reason as the derivers.

I may be misunderstanding things, but I tried this to get a list of migrations:

$migration_manager = \Drupal::service('plugin.manager.migration');
$migrations = $migration_manager->createInstances([]);

This currently seems to fail for me in most situations. If I enable just the migrate module, it fails because D6NodeDeriver tries to get the service "plugin.manager.migrate.cckfield", that lives in migrate_drupal. If I enable migrate_drupal, it fails because the migrate_drupal database is not configured. I suppose if I configured a DB it would likely work, but we have to assume some people will use migrations that have nothing to do with upgrades.

* Is my code snippet the right way to do this?
* How do we distinguish between migrations that exist, and migrations that are ready to run?
** Eg: Should we make the D6/D7 derivers simply yield nothing when the migrate database is not configured?
** Should dependencies factor into this?

@vasi, where are you doing that? Seems like something that would be done in a migrate contrib project. The dependency issue you describe already exists in core, see #2560795: Source plugins have a hidden dependency on migrate_drupal - we did discuss moving D6Deriver back to migrate_drupal earlier in this issue which would solve the problem.

Theres a follow-up to work out how sources/destinations get their configuration injected, as you mention, it currently throws an error connecting if you don't have a source database configured. We can discuss whether migrations that don't have their config should be silently skipped in #2681869: Provide clean way to merge configuration into migration plugins

@vasi - good question - see Drupal\migrate_drupal\MigrationCreationTrait::getMigrations() - although I'm inclined to agree that $migrations = $migration_manager->createInstances([]); should work and only create migrations that can be run - however I think we'll need to fix #2681869: Provide clean way to merge configuration into migration plugins to make this right.

@benjy's changes in response to @catch's review look great. Given that I haven;t worked on this since the last rtbc - I'm happy to re-rtbc it.