Migrations should support non-installed default configurations (templates)

would #2431275: Move optional configuration in config/install to config/optional be a solution for this?

i understand that we don't want to have all possible migrations pollute the configuration system. basically unless you decide you want to migrate something (being d6 config, d7 config, ...) the system shouldn't add active configuration.

The database credentials ought to go into state and injected on load time instead of saving them into every migration. We already have a query system for config entities. Once D6/D7 is migrated, uninstall the module, your migrations are gone. I thought we already agreed on all this? Why do we have another issue?

1. +++ b/core/modules/migrate/src/Entity/Migration.php
   +  public static function templateQuery($tag) {
   

   Should this be on the Migration class? I feel like this maybe should go on the MigrationStorage handler, since it's about loading meta-migrations. Then again...Migration::load() exists, but that's to load a single migration, whereas this method has a wider reach.

   I also feel like this should be renamed. I find templateQuery() kind of vague. Maybe searchTemplatesByTag() or similar?

2. +++ b/core/modules/migrate/src/Entity/Migration.php
   +    $extension_install_storage = new ExtensionInstallStorage(\Drupal::service('config.storage'), ExtensionInstallStorage::MIGRATION_TEMPLATE_DIRECTORY, StorageInterface::DEFAULT_COLLECTION, TRUE);
   

   Except for the config.storage service, all the arguments of this call are ExtensionInstallStorage::__construct's defaults. Why repeat them? The constructor only calls parent::__construct() anyway, so another option is to just delete ExtensionInstallStorage's constructor entirely.

3. +++ b/core/modules/migrate/src/ExtensionInstallStorage.php
   +  /**
   +   * @inheritdoc
   +   */
   +  public function __construct(StorageInterface $config_storage, $directory = self::MIGRATION_TEMPLATE_DIRECTORY, $collection = StorageInterface::DEFAULT_COLLECTION, $include_profile = TRUE) {
   +    parent::__construct($config_storage, $directory, $collection, $include_profile);
   +  }
   

   If we keep this method for some reason, the doc comment should be {@inheritdoc}.

I chatted with mikeryan and phenaproxima on IRC about this patch, and I'm definitely impressed at the small size of it. Quite elegant. Great job, Mike - we're lucky to have such big brains working on this stuff.

I'm glad that this won't add any additional overhead (just slightly different overhead) to the use case of having to customize an existing migration config while also enabling future extensibility for custom migrations.

I much prefer findTemplatesByTag() over templateQuery().

I'm +1 for this!

-mike

What happened to groups? The issue summary mentions them quite a lot, #4 said:

> Database credentials would go into the group, not into every migration (what does go into every migration is a reference to the group).

but that's the last time any issue comment or patch contains the word "group". So either the patch is incomplete or the issue summary needs upgrade or the issue should simply be won't fixed (which I am not doing since I am not a migrate maintainer, I am just expressing a preference).

I agree with #10.1 we should move tempalteQuery() to MigrationStorage. From there I think we should instantiate the ExtenstionInstallStorage in the constructor and inject the rest of the dependencies like the config storage.

#10.2, The constructor is overridden so we can provider our own defaults, see the MIGRATION_TEMPLATE_DIRECTORY is the default now.

1. +++ b/core/modules/migrate/src/Entity/Migration.php
   @@ -534,4 +536,28 @@ public function calculateDependencies() {
   +    $extension_install_storage = new ExtensionInstallStorage(\Drupal::service('config.storage'), ExtensionInstallStorage::MIGRATION_TEMPLATE_DIRECTORY, StorageInterface::DEFAULT_COLLECTION, TRUE);
   

   The last three params are all defaults.

2. +++ b/core/modules/migrate/src/ExtensionInstallStorage.php
   @@ -0,0 +1,57 @@
   +  const MIGRATION_TEMPLATE_DIRECTORY = 'migration_templates';
   

   I wonder if we should use 'migration_template' inline with core that has install, optional and schema.

3. +++ b/core/modules/migrate/src/ExtensionInstallStorage.php
   @@ -0,0 +1,57 @@
   +    $folders = $this->getAllFolders();
   

   Will this do discovery of themes, profiles and modules? Eg, could we make it faster with a cut down version?

4. +++ b/core/modules/migrate/src/ExtensionInstallStorage.php
   @@ -0,0 +1,57 @@
   +      $first_dot = strpos($full_name, '.');
   +      $provider = substr($full_name, 0, $first_dot);
   +      $name = substr($full_name, $first_dot + 1);
   

   This could be replace with:
   list($provider, $name) = explode('.', $full_name, 2);

5. +++ b/core/modules/migrate/src/Tests/TemplateTest.php
   @@ -0,0 +1,44 @@
   +    $migration_templates = Migration::templateQuery("Template Test");
   

   It would be nice to have a few more migration templates in the test module, and then test that the templateQuery() is actually doing some filtering. Right now it could just be returning everything and this would still pass.

Other than that it's looking pretty good, amazing how simple it turned out!

Changes look good to me.

Looks good and makes sense. Testbot approves, so *taps patch on the shoulders with a sword* I declare this RTBC.

1. I'm loving the fact the migrations are not in configuration be default. Nice work. We're almost at the best of both worlds.
2. I think this change might need a CR because other people are working on migrations and this is a pretty big breaking change - what does everyone think?

3. +++ b/core/modules/migrate/src/ExtensionInstallStorage.php
   @@ -0,0 +1,55 @@
   +class ExtensionInstallStorage extends BaseExtensionInstallStorage {
   

   Let's call this MigrationTemplateStorage

4. +++ b/core/modules/migrate/src/MigrationStorage.php
   @@ -90,4 +136,26 @@ protected function addDependency(array &$graph, $id, $dependency, $dynamic_ids)
   +  public function findTemplatesByTag($tag) {
   

   Lets move this to MigrationTemplateStorage and make that a service so this method can be used. No point mixing template storage with the real storage.

5. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
   @@ -42,4 +45,28 @@ protected function getDumpDirectory() {
   +    $migration_templates = $migration_storage->findTemplatesByTag($version);
   

   I'm still concerned that version is in a tag as I think version is something more fundamental about a migration than a tag. I.e. it is says what can this migrate from.

Here's a patch addressing #2-#4 :) The service is called migrate.template_storage.

Sorry about the gigantic-ness of the patch; for some reason my git did not pick up the renames, and instead seems to think that the contents of config/optional were deleted, and then re-created entirely in migration_templates.

An easier-to-review version, after fixing my git config (thanks @benjy!)

Let's try that again...

Lets not use ExtensionInstallStorage in the Migrate full test, we have our own service now.

Fixed!

I'm still concerned that version is in a tag as I think version is something more fundamental about a migration than a tag. I.e. it is says what can this migrate from.

Version is specific to the migrate_drupal migrations, while the API being added here is meant to be generic. Maybe an alternative, without touching the API, would be for the migrate_drupal migrations to have a tag of 'Drupal' and source.version of 6, 7, etc., and do the version filtering after obtaining all the 'Drupal' templates?

I'd even go a step further. Migrations, as we know, aren't just Drupal to Drupal; they might also be WordPress to Drupal or Typo3 to Drupal. I think the version indicator should be called "api" or "platform" or something, and its value should be a meaningful constant like drupal-6.x, drupal-7.x, wordpress-3.x, typo3-2.x or what-have-you. I envision this key as being similar to hook_views_api()'s "api" key, only broader.

Example:

id: d6_node_type
# We filter on platform, not the tags. The tags are just for usability.
platform: drupal-6.x
migration_tags:
  - Drupal 6
  - Content
  - node

This isn't really a change in functionality -- we could filter on migration_tags -- but a change in semantics. I think it's a good idea for that reason alone.

With the name change to MigrateTemplateStorage, we don't need the "as BaseExtensionInstallStorage" anymore.

Fixed!

+++ b/core/modules/migrate/src/MigrateTemplateStorage.php
@@ -0,0 +1,77 @@
+  public function getAllTemplates() {

I do wonder if this and maybe even findTemplatesByTag() should cache the results on the class? They do YAML discovery after all which is probably expensive although this might not be called all that often.

@alexpott, what do you think? The rest is RTBC for me.

That being answered, it looks like this is RTBC as per #31.

+++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
@@ -42,4 +45,26 @@ protected function getDumpDirectory() {
+    $migration_templates = \Drupal::service('migrate.template_storage')->findTemplatesByTag($version);
+    foreach ($migration_templates as $template_name => $template) {
+      try {
+        $migration = Migration::create($template);
+        $migration->save();
+      }
+      catch (PluginNotFoundException $e) {
+        // Migrations requiring modules not enabled will throw an exception.
+        // Ignoring this exception is equivalent to placing config in the
+        // optional subdirectory - the migrations we require for the test will
+        // be successfully saved.
+      }

Hmmm looking at this i think we should do this differently. I think we should be using the ConfigInstaller here and setting the source storage to be the migrate.template_storage. We should set something on the migrate.template_storage so that listAll() only returns the migrations which have the desired version. That way migration config dependencies are properly used when they are in the template.

Forget the wrapper, have validateDependencies() pick up those bits itself if they're not provided...

+1 for that. The patch looks good to me.

For one last time: this is wrong.

> As installed, these configuration entities are incomplete - they cannot be actually used without adding configuration for the source database.

Put the database credentials in state. This is a classic case of state: if you have a development site it is likely the credentials will point to a development version of the source site and the production site will have production credentials.

> They pollute config export - putting them into your site config git repo - does that make sense

How is it better to put the installed config entities complete with db credentials in config export when, as I pointed out above, production will likely need different credentials?

I agree with @chx about the DB credentials being run-time state information, but that's not a reason to hold up this patch. There is nothing in the patch, as written, which forces the DB credentials to be in the migration configuration. Yes, that was the original imagined use case, but a template system would be useful for other reasons (I'm interested in generating migration configurations ahead of time, based on the source data -- this would clear up a lot of complex, knotty code).

It's not wrong.

In migrate_upgrade we're also using templates to merge source_base_path obtained from the UI/drush command into the file migrations.

To me, that is also state info since it could vary from environment to environment, but that's another discussion.

Since @mikeryan wrote 95% of this patch, I have it on good authority (Moshe) that it's ethical for me to RTBC it. It looks good to me as of #39.

Then you need to write a new IS because the current one is DB credential focused.

Upgrading to major. This is blocking work on the core migrate UI, as well as work on CCK/field handling for D6 and D7.

HEAD done broke our patch! This one regenerates the ConfigInstaller proxy, since the patch changes ConfigInstallerInterface.

Since the previous patch was just a boilerplate thing and didn't actually change any template-related code, I feel comfortable setting this back to RTBC.

I think I derailed this issue around #36. I incorrectly treated templates as whole configuration entities. Since looking at the changes and discussing further plans for templates in IRC with @mikeryan, @phenaproxima and @neclimdul I think we should take the approach in #29 with one change. I don't think that templates should contain config dependencies. Config dependencies are part of configuration and not the migration system.

Discussed with @mikeryan and @phenaproxima in IRC and we're all happy with the proposal in #55.

Fixed as per #55. None of the templates have the dependencies key anymore, and MigrateFullDrupalTestBase will no longer try to verify dependencies.

Let's get this thing in. :)

Looks good to me, nice to get rid of all those config dependencies.

@benjy - well we're not getting rid of the config dependencies - they are calculated from the templates when we convert them into Migration configuration entities.

Committed 74d72d1 and pushed to 8.0.x. Thanks!

I still feel a CR would be useful for people who have built migrations from their contrib or custom modules.

Yeah I understands that, but, out of site out of mind :) nicer for those of us who will hand write migrations