Problem/Motivation

The original report (see below) pointed out that documentation, change record, and the code base were not in agreement about the location of the migration yml files.

The change record is correct. Migration yml files should be located in MODULE/migrations instead of MODULE/migration_templates.

Proposed resolution

The code task is to rename all directories of the form core/module_name/migration_templates to core/module_name/migrations.

When making the patch please be sure to use git diff -M and check that the patch is really doing renaming and not deleting and inserting new files.

Documentation is being done in #2918219: Replace 'migrate_templates' documentation references with 'migrations'

Remaining tasks

Write a patch
Review
Committ

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Original report by @TR

Original Report
What directory are contib modules supposed to use for migration templates?

Years ago it was migration_templates/ (see #2533886: [meta] Move module-specific migration support into the particular modules supported)

Then supposedly this was changed to migrations/ as of Drupal 8.1.x (Migrations are plugins instead of configuration entities)

But core still uses migration_templates/ ? And all the patches in the core queue that add new templates seem to be putting those templates into migration_templates/ still.

The documentation, such as it is, sometimes says one, sometimes says the other. See the newly-created https://www.drupal.org/docs/8/api/migrate-api/writing-migrations-for-con... for example.

I don't see any later change records which reverted this, and I don't see any open issue to fix core to use the new directory, so I'm confused as to where I should be putting my templates in my contrib modules.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TR created an issue. See original summary.

quietone’s picture

Status: Active » Needs review

Thanks for pointing out that the documentation needs updating. Fortunately, masipila is currently in the process of updating and improving the migration documentation.

To answer your question, the preferred directory for the migration yml files is migrations. And the page where this is stated correctly, is Migration API .

Migration plugin definitions are stored in a module's 'migrations' directory. For backwards compatibility we also scan the 'migration_templates' directory.

I've added a comment to the Meta documentation issue suggesting that all documentation be changed to use migrations instead of 'migration_templates'

@TR, does that answer your question? If so, please mark as Fixed.

TR’s picture

Status: Needs review » Active

Yes, that answers the question. @masipila's new documentation is helpful, but it's only 3 days old now. I posted this because I saw his mention of migrations/ and that was the only indication of the new location apart from the old change record and that seem to have been ignored.

So perhaps this should be changed to a Bug report? core still uses migration_templates/ more than 18 months after this change, and all the pending core patches that I found are continuing to put things in migration_templates/.

As you know, because of the sparse and many times outdated documentation, everyone has to use core as an example. Because core doesn't use migrations/ and there was no documentation about this before 3 days ago, everyone (including the migrate and the migrate_drupal modules) is using migration_templates/.

# find . -name migration_templates
./core/modules/action/migration_templates
./core/modules/field/migration_templates
./core/modules/system/migration_templates
./core/modules/node/migration_templates
./core/modules/contact/migration_templates
./core/modules/syslog/migration_templates
./core/modules/simpletest/migration_templates
./core/modules/forum/migration_templates
./core/modules/book/migration_templates
./core/modules/ban/migration_templates
./core/modules/update/migration_templates
./core/modules/block/migration_templates
./core/modules/migrate_drupal_ui/tests/modules/migration_provider_test/migration_templates
./core/modules/file/migration_templates
./core/modules/migrate_drupal/tests/modules/migrate_overwrite_test/migration_templates
./core/modules/menu_link_content/migration_templates
./core/modules/config_translation/migration_templates
./core/modules/user/migration_templates
./core/modules/block_content/migration_templates
./core/modules/language/migration_templates
./core/modules/dblog/migration_templates
./core/modules/filter/migration_templates
./core/modules/path/migration_templates
./core/modules/tracker/migration_templates
./core/modules/image/migration_templates
./core/modules/migrate/tests/modules/migrate_high_water_test/migration_templates
./core/modules/shortcut/migration_templates
./core/modules/search/migration_templates
./core/modules/menu_ui/migration_templates
./core/modules/comment/migration_templates
./core/modules/taxonomy/migration_templates
./core/modules/statistics/migration_templates
./core/modules/aggregator/migration_templates
./core/modules/text/migration_templates
./core/modules/locale/migration_templates

# find . -name migrations
./core/modules/migrate/tests/modules/migrate_external_translated_test/migrations
./core/modules/taxonomy/tests/modules/taxonomy_term_stub_test/migrations
masipila’s picture

Hi,

As @quietone mentioned above, the preferred directory modules should be storing their migrantion plugins is 'migrations'.

About the docs
This is documented here and on the new documentation page you were referring to. Both are saying that 'migration_templates' works for backwards compatibility.

There are for sure references to the 'migration_templates' in the old documentation pages. As @quietone mentioned, I'm currently reviewing them and as I find references to 'migration_templates', I will update the docs.

About the directory used by the core modules
You are absolutely correct that all core modules are still using the 'migration_templates' directory and this is confusing. Maybe the maintainers could comment here if there are any plans to move these to 'migrations' directory and would that cause any side effects to existing sites?

Cheers,
Markus

quietone’s picture

The current focus is the, currently 9, Critical issues. And the renaming of directories that currently work is not in that list, it is low priority now. We have backwards compatibility and whenever the change happens the necessary deprecation notices will be added to inform any using the migration system.

I don't see an existing issue for moving the migration yml files, therefor I suggest one be opened. That way this is more likely to be discussed at a migrate meeting.

masipila’s picture

Title: migrations/ or migration_templates/ ? » Rename 'migration_templates' directories in core modules to 'migrations'
Category: Support request » Task
Issue tags: +Needs issue summary update

Hi!

I propose that we use this issue for renaming the core directories from 'migrate_templates' to 'migrations'. I changed the title and changed this to a task to reflect this.

The issue summary needs to be updated. Tagging for that as I don't have time to do it right now.

Markus

quietone’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update +Novice

Updated the IS.

Renaming of the directories is a suitable novice task, tagging as so.

quietone’s picture

Issue summary: View changes

Fix format error in IS

masipila’s picture

Assigned: Unassigned » masipila

Working on this now.

masipila’s picture

Assigned: masipila » Unassigned
Status: Active » Needs review
FileSize
51.03 KB

Renamed the directories with git mv core/modules/foo/migration_templates core/modules/foo/migrations and diff created with git diff -M 8.5.x

find . -name migration_templates doesn't return anything.

Cheers,
Markus

masipila’s picture

Issue summary: View changes

Updated the issue summary slightly to help reviewers.

phenaproxima’s picture

Status: Needs review » Needs work

I like this a whole lot. Totally will make things clearer for developers, and I'm always in favor of that.

However, we do want to be sure that the migration_templates directory is still working, since that is a BC layer. For that, let's change all the test migrations (i.e., the ones included with the test modules in migrate, migrate_drupal, and migrate_drupal_ui) back to migration_templates. That way, we'll be guarding against the possibility of migration_templates no longer working.

phenaproxima’s picture

I think this is definitely a DX improvement, so tagging as such.

quietone’s picture

Tested the patch. Before applying the patch I used find, find . -type d -name migration_templates | nlto determine the number of directories that needed to be changed. There are 35. Applied that patch and did a find again,find . -type d -name migrations | nlAnd there are 37 directories named 'migrations'. Didn't expect that there would be 2 core 'migrations' directories.

So, removed the patch and searched for 'migrations'. There are 2 existing directories with that name, ./core/modules/migrate/tests/modules/migrate_external_translated_test/migrations and ./core/modules/taxonomy/tests/modules/taxonomy_term_stub_test/migrations.

I like using the existing tests for also checking that migration_templates still works. But since the tests are now doing 2 things, I think an explanation needs to be added to, at least, the class documentation for the test so the test files don't get moved.

TR’s picture

Issue tags: +Needs tests

Before applying the patch I used find, find . -type d -name migration_templates | nlto determine the number of directories that needed to be changed. There are 35. Applied that patch and did a find again,find . -type d -name migrations | nlAnd there are 37 directories named 'migrations'. Didn't expect that there would be 2 core 'migrations' directories.
...
There are 2 existing directories with that name, ./core/modules/migrate/tests/modules/migrate_external_translated_test/migrations and ./core/modules/taxonomy/tests/modules/taxonomy_term_stub_test/migrations.

Umm, that's what I said in #3? Just sayin' ...

However, we do want to be sure that the migration_templates directory is still working, since that is a BC layer. For that, let's change all the test migrations (i.e., the ones included with the test modules in migrate, migrate_drupal, and migrate_drupal_ui) back to migration_templates. That way, we'll be guarding against the possibility of migration_templates no longer working.

Yes, we absolutely want to make sure that migration_templates/ continues to work, since that's been the example for the past 18 months. But I don't think continuing to use migration_templates/ in core is the correct strategy. I think we should change EVERYTHING (I have applied the patch locally and I can confirm it does this), then ADD A TEST for \Drupal\migrate\Plugin\MigrationPluginManager::getDiscovery(), since that is where core looks in both migrations/ and migration_templates/.

masipila’s picture

@phenaproxima, do you have an opinion on how to ensure the test coverage for the 'migration_templates' BC compatibility:

1. Should we simply cover this by keeping the migrate, migrate_drupal, migrate_drupal_ui test migrations to use 'migration_templates' as you suggested in #12?

2. Or should we cover this by adding an explicit test coverage for MigrationPluginManager::getDiscovery() ad @TR suggested in #15?

I didn't try 2) yet but I had a quick look at the getDiscovery() method. It seems that we are instantiating objects with the 'new Foobar' syntax instead using dependency injection so mocking / writing PHPUnit coverage might be difficult if I'm not mistaken.

So should we just do 1) as @phenaproxima suggdsted in #12? Do we need to document somewhere that we left these test migrations to 'migration_templates' intentionally?

Markus

phenaproxima’s picture

I think I prefer @TR's idea in #15. If core is inconsistent -- even if it's just in test modules -- it sends mixed signals to developers who rely on it as an example. So let's add an explicit test to ensure the BC layer continues to work, then nix migration_templates everywhere in core.

quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
55.3 KB
4.27 KB

Added a test.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: -Novice

Wow! Thanks for the thorough testing -- unfortunately, I think it's a bit too thorough. We can remove some stuff here...

  1. --- /dev/null
    +++ b/core/modules/migrate/tests/modules/migrate_migration_directory_test/migrate_migration_directory_test.info.yml
    

    Can we rename this module to migration_directory_test, just to save our lazy wrists some typing?

  2. +++ b/core/modules/migrate/tests/modules/migrate_migration_directory_test/migrate_migration_directory_test.info.yml
    --- /dev/null
    +++ b/core/modules/migrate/tests/modules/migrate_migration_directory_test/migration_templates/migrate.migration.migration_templates_test.yml
    

    Migrations are not supposed to be prefixed with migrate.migration -- that's a Migrate Plus thing.

  3. +++ b/core/modules/migrate/tests/modules/migrate_migration_directory_test/migration_templates/migrate.migration.migration_templates_test.yml
    --- /dev/null
    +++ b/core/modules/migrate/tests/modules/migrate_migration_directory_test/migrations/migrate.migration.migrations_test.yml
    

    We don't need this migration. The fact that all other tests pass confirm that the 'migrations' directory works.

  4. +++ b/core/modules/migrate/tests/modules/migrate_migration_directory_test/migrations/migrate.migration.migrations_test.yml
    --- /dev/null
    +++ b/core/modules/migrate/tests/modules/migrate_migration_directory_test/migrations_not_found/migrate.migration.migrations_not_found_test.yml
    

    I'm not sure what value this migration adds -- searching the right directories is the responsibility of the plugin discovery mechanism, and testing that is squarely in its province. So let's remove this migration too.

  5. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationDirectoryTest.php
    @@ -0,0 +1,38 @@
    +  public function testMigrationDirectory() {
    +
    +    /** @var \Drupal\migrate\Plugin\MigrationPluginManager $plugin_manager */
    

    Nit: Extra blank line!

  6. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationDirectoryTest.php
    @@ -0,0 +1,38 @@
    +    $migration = $plugin_manager->createInstance('migration_templates_test');
    

    We don't need to bother creating an instance of the migration. This will be perfectly sufficient:

    $this->assertTrue($plugin_manager->hasDefinition('migration_templates_test'))

I'm also removing the Novice tag, now that we have a test of this too.

quietone’s picture

Status: Needs work » Needs review
FileSize
53.32 KB
4.18 KB

1. Renamed as suggested
2. Yea, that is from copy/pasting and I was surprised by the naming since it was already there. There are 3 such files.

./core/modules/migrate/tests/modules/migrate_high_water_test/migrations/migrate.migration.high_water_test.yml
./core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/migrate.migration.external_translated_test_node.yml
./core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/migrate.migration.external_translated_test_node_translation.yml

I'll make an issue to change them, unless there is objection.
3 and 4. Fixed. I was just being over zealous.
5. Fixed
6. Yes, that is simpler and better. Fixed too.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Looks great.

RTBC on the assumption that Drupal CI will pass it.

phenaproxima’s picture

Issue tags: +Needs followup

Tagging so that we remember to open a follow-up to remove the migrate.migration prefix from any core migrations which have it, as per #20.

quietone’s picture

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record, +Needs change record updates

I think we need to:

  1. Attach the previous change record to this issue.
  2. Add a new change record to this issue that documents that all these are removed but that we will still discover mirgration_templates folder for BC, but that it is deprecated. It should also reference the previous change record in the text.
  3. Add an explicit deprecation to the old migration_templates directory, with the @deprecated and @trigger_error() (reference: https://www.drupal.org/core/deprecation)

What is the disruption of moving the templates we move in the patch? What impact does it have on contrib code that might be using it (how are they used)? Does it need an upgrade path of some kind?

We've had upgrade issues from moving files around before. In our BC policy, we have procedural code locations as internal (meaning it can be changed in a minor release). But this isn't procedural code.

quietone’s picture

1. Added the previous change record to this issue.
2. Made a new change record.
3. TODO. I have looked at the deprecation policy and still don't know how to implement deprecation for this.

As for disruption and impact on contrib etc. There is zero disruption. Migration ymls in both the old and new directories will be found. That ability was added in 8.1.x., as stated in the previous change record,Core has been using migration_templates, and as an example, migrate_plus uses migrations. The migrations from both core and migrate_plus are found.

masipila’s picture

Re #24 / last questions. Contrib / custom modules can (and typically do) use the source / process / destination plugins provided by core modules but they don't have anything to do with this issue i.e. this patch doesn't have any impact on those plugins.

The migrations themselves are defined with the yml files which are scanned from both 'migration_templates' and 'migrations' directories. Both will work also after this patch. We're only making sure that core modules are using the same directory names that we've been recommending contrib modules to use since 8.1.x.

Hope this helps in addition to what @quietone wrote above.

Cheers,
Markus

phenaproxima’s picture

Add an explicit deprecation to the old migration_templates directory, with the @deprecated and @trigger_error()

I have looked at the deprecation policy and still don't know how to implement deprecation for this.

Here is an idea for how to do it:

Currently, MigrationPluginManager does a bunch of additional song and dance with discovered plugin definitions. (See its getDiscovery() method.) What we could do is create our own subclass of YamlDirectoryDiscovery, which overrides the findFiles() method and triggers a deprecation notice if basename(dirname($file)) == 'migration_templates'. Then we could change MigrationPluginManager to use that.

But, is it worth it? It means adding extra code, and possibly extra tests, to account for something that is a) not a normal case for deprecation, since it isn't code, and b) documented twice now.

I defer to @xjm and other committers on whether we should proceed with such an approach, or if it would be overzealous.

TR’s picture

I'd like to point out that there is no place in the class or function doxygen comments where it is documented that migrations will be discovered in the 'migrations/' and 'migration_templates/' directories. That is, there is no API 'contract' right now as to how the discovery takes place - this is currently a matter of convention. (*see my comment at bottom)

So as part of this issue, what we can do is make the discovery directory a part of the API for the first time ever. That makes this technically an API change, but really just a codification of current practice.

There is no API function being deprecated, so I don't see how @deprecated can be properly used anywhere.

However, I think the important part of @xjm's point 3 in #24 is that we need a way to detect and correct any future core patch that tries to put migration configurations into 'migration_templates/' instead of the "new" correct directory, and we need a way to warn contrib not do do this either. I think we can do that with a @trigger_error in MigrationPluginManager::getDiscovery() (to trigger an error when something is found in 'migration_templates/') and also add a function doc comment to specify where getDiscovery() is looking for the migration configurations.

I have reviewed the change record changes made by @quietone in #25 and I think these are correct and addressed @xjm's first two concerns. I think adding the @trigger_error and updating the function comment for getDiscover() would satisfy the one remaining concern from #24 and keep us from adding future uses of 'migration_templates/'.

*I see that migrate/migrate.api.php mentions that both migrations/ and migration_templates/ will be searched but that migrations/ is the correct place to put this. Perhaps stronger language here should be part of the patch as well

phenaproxima’s picture

Status: Needs review » Needs work

I see that migrate/migrate.api.php mentions that both migrations/ and migration_templates/ will be searched but that migrations/ is the correct place to put this.

I would suggest that we remove all mention of migration_templates from migrate.api.php (people won't use something they don't know exists, and that is not an obvious pattern in core), and add the trigger_error() to getDiscovery() to address people who are already using migration_templates. This will establish a convention for future deprecations like this one (although those are unlikely, since Migrate is a special snowflake in many ways, including this one.)

TR’s picture

Status: Needs work » Needs review
FileSize
55.55 KB
2.23 KB

This is the patch from #20, adding the suggestion I made in #28, and using the suggestion made in #29.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

That's a much more elegant way than what I was picturing, and it makes an awful lot of sense.

I think all concerns are now addressed, so this is preemptively RTBC pending Drupal CI. Nice work!

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, gotta kick this back for a few minor things:

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationDirectoryTest.php
@@ -0,0 +1,29 @@
+ * Tests that modules exist for all source and destination plugins.
+ *
+ * @group migrate
+ */
+class MigrationDirectoryTest extends MigrateDrupalTestBase {

The class description is inaccurate.

  1. +++ b/core/modules/migrate/tests/modules/migration_directory_test/migration_templates/migration_templates_test.yml
    @@ -0,0 +1,15 @@
    +  data_rows:
    +    -
    +      id: basic
    +  ids:
    +    id:
    +      type: string
    +  source_module: block
    

    Nit: We don't actually need this stuff. It's not hurting anything, merely adding extra lines to the patch.

  2. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -60,11 +60,21 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
    +   * This method overrides DefaultPluginManager::getDiscovery() in order to
    +   * search for migration configurations in the MODULENAME/migrations
    +   * directory.
    

    This documentation should be expanded. It searches in both migrations and migration_templates, and throws a deprecation notice for migration_templates; we should mention that.

jofitz’s picture

Status: Needs work » Needs review
FileSize
55.56 KB
2.18 KB

Corrected the class description.

  1. Removed the redundant lines.
  2. Expanded the documentation.
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @Jo Fitzgerald!

xjm’s picture

Status: Reviewed & tested by the community » Fixed
  1. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -60,11 +60,22 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
    +   * This method overrides DefaultPluginManager::getDiscovery() in order to
    +   * search for migration configurations in the MODULENAME/migrations and
    +   * MODULENAME/migration_templates directories. Throws a deprecation notice if
    +   * the MODULENAME/migration_templates directory exists.
    

    https://www.drupal.org/core/deprecation#how-codepath helpfully says we should add a note to the docblock about the deprecated behavior, and provides the useful example of: "// @todo". I think this is fine.

  2. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -60,11 +60,22 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
    +        // Check for use of the @deprecated /migration_templates directory.
    +        // @todo Remove use of /migration_templates in Drupal 9.0.0.
    +        if (is_dir($directory . '/migration_templates')) {
    +          @trigger_error('Use of the /migration_templates directory to store migration configuration files is deprecated in Drupal 8.1.0 and will be removed before Drupal 9.0.0. See https://www.drupal.org/node/2920988.', E_USER_DEPRECATED);
    +        }
    +        // But still accept configurations found in /migration_templates.
    

    Thanks, this looks like a totally fine way to trigger the deprecation message.

  3. I made some small changes to the change record so that it was a bit more assertive about the new best practice while clarifying that there was BC: https://www.drupal.org/node/2920988/revisions/view/10704755/10707923

Committed to 8.5.x and published the change record. Thanks!

  • xjm committed 33818be on 8.5.x
    Issue #2917883 by quietone, TR, Jo Fitzgerald, masipila, phenaproxima:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

neclimdul’s picture

Issue tags: +BC break