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.
Comment | File | Size | Author |
---|---|---|---|
#33 | interdiff-30-33.txt | 2.18 KB | jofitz |
#33 | 2917883-33.patch | 55.56 KB | jofitz |
#30 | interdiff-2917883-20-30.txt | 2.23 KB | TR |
#30 | 2917883-30.patch | 55.55 KB | TR |
#20 | interdiff.txt | 4.18 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedThanks 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 .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.
Comment #3
TR CreditAttribution: TR commentedYes, 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/.
Comment #4
masipila CreditAttribution: masipila as a volunteer commentedHi,
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
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedThe 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.
Comment #6
masipila CreditAttribution: masipila as a volunteer commentedHi!
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
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedUpdated the IS.
Renaming of the directories is a suitable novice task, tagging as so.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedFix format error in IS
Comment #9
masipila CreditAttribution: masipila as a volunteer commentedWorking on this now.
Comment #10
masipila CreditAttribution: masipila as a volunteer commentedRenamed the directories with
git mv core/modules/foo/migration_templates core/modules/foo/migrations
and diff created withgit diff -M 8.5.x
find . -name migration_templates
doesn't return anything.Cheers,
Markus
Comment #11
masipila CreditAttribution: masipila as a volunteer commentedUpdated the issue summary slightly to help reviewers.
Comment #12
phenaproximaI 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.
Comment #13
phenaproximaI think this is definitely a DX improvement, so tagging as such.
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedTested the patch. Before applying the patch I used find,
find . -type d -name migration_templates | nl
to 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 | nl
And 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.
Comment #15
TR CreditAttribution: TR commentedUmm, that's what I said in #3? Just sayin' ...
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/.
Comment #16
masipila CreditAttribution: masipila as a volunteer commented@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
Comment #17
phenaproximaI 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.
Comment #18
quietone CreditAttribution: quietone as a volunteer commentedAdded a test.
Comment #19
phenaproximaWow! Thanks for the thorough testing -- unfortunately, I think it's a bit too thorough. We can remove some stuff here...
Can we rename this module to migration_directory_test, just to save our lazy wrists some typing?
Migrations are not supposed to be prefixed with migrate.migration -- that's a Migrate Plus thing.
We don't need this migration. The fact that all other tests pass confirm that the 'migrations' directory works.
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.
Nit: Extra blank line!
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.
Comment #20
quietone CreditAttribution: quietone as a volunteer commented1. 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.
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.
Comment #21
phenaproximaLooks great.
RTBC on the assumption that Drupal CI will pass it.
Comment #22
phenaproximaTagging 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.
Comment #23
quietone CreditAttribution: quietone as a volunteer commentedFollow up created #2920963: Remove the migrate.migration prefix from core test migrations
Comment #24
xjmI think we need to:
mirgration_templates
folder for BC, but that it is deprecated. It should also reference the previous change record in the text.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.
Comment #25
quietone CreditAttribution: quietone as a volunteer commented1. 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.
Comment #26
masipila CreditAttribution: masipila as a volunteer commentedRe #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
Comment #27
phenaproximaHere 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.
Comment #28
TR CreditAttribution: TR commentedI'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
Comment #29
phenaproximaI 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.)
Comment #30
TR CreditAttribution: TR commentedThis is the patch from #20, adding the suggestion I made in #28, and using the suggestion made in #29.
Comment #31
phenaproximaThat'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!
Comment #32
phenaproximaSorry, gotta kick this back for a few minor things:
The class description is inaccurate.
Nit: We don't actually need this stuff. It's not hurting anything, merely adding extra lines to the patch.
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.
Comment #33
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrected the class description.
Comment #34
phenaproximaThanks, @Jo Fitzgerald!
Comment #35
xjmhttps://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.Thanks, this looks like a totally fine way to trigger the deprecation message.
Committed to 8.5.x and published the change record. Thanks!
Comment #38
neclimdul