Problem/Motivation
Right now, all migration destination plugins are implemented in the migrate module, and all support for Drupal-to-Drupal migrations (templates and source plugins) is in the migrate_drupal module. Most of these templates and plugins are specific to migration into a specific core module. We propose moving this migration support into the relevant modules which are the targets of the migrations. We did have some discussion of this in the past (alexpott references it at https://www.drupal.org/node/2460529#comment-9967447, most discussion was in IRC) and the various people involved with Migrate were on board with the concept at the time. We solved our immediate dependency issues at the time and never got around to this follow-up.
Advantages:
- Because they're implemented centrally rather than in the specific modules supported, explicit dependencies on those modules must be maintained, and clients of the migration support must make explicit requirements checks to determine which templates are instantiable. If the templates live in the relevant target modules, clients will not find templates for uninstalled modules, and the relevent plugins will not be instantiated. Dependencies on the core modules will be implicit rather than explicit.
- In the case of #2503049: Migrate D7 blocked IPs to D8, the explicit dependencies are not enough - this migration's dependency on the ban module brings an implicit dependency on the ban.ip_manager service. Tests for other migrations, which do not have the ban module installed, break because when all apparently-relevant migrations are installed, this migration will get installed and fail for lack of the service. Workarounds for this situation, as long as the ban support is in migrate_drupal, are rather hackish. This is the problem motivating us to finally open this issue.
- This is a better structure conceptually, and a better model for contrib - logically, it makes sense for a given module to own its own data and the means of accessing/manipulating it.
Disadvantages:
- Adds maintenance burden to the target modules
Proposed resolution
For the most part, this is a matter of moving files around and updating namespaces. Also, explicit destination module dependencies can be removed. The tests might need some thought - if we were to move them, it may be tricky.
It would probably be best to make this a meta issue and break out specific issues for each target module, so each module maintainer can review their own bit.
Remaining tasks
phenaproxima to implement #2503049: Migrate D7 blocked IPs to D8 according to this policy, serving as an example of what it looks like for a module to own its own migration support.Done.Agree on principle to do this in general.Done.Open child issues for each affected module.Done.- Fix the child issues.
User interface changes
N/A
API changes
Namespaces of affected plugins will change.
Data model changes
N/A
Comments
Comment #1
phenaproximaHUGE +1 from me. This is long overdue, and Migrate Drupal is becoming a lumbering kaiju of a module.
The maintenance burden is minimal. The migrations/plugins to be moved are already written, with passing tests -- as @mikeryan stated, all we need to do is move things into the relevant modules and change the namespaces. If anything, I'd argue this will speed up the process and lighten the overall maintenance burden, since the responsibility of reviewing further changes (not that I anticipate many -- once a migration is committed, it's usually a done deal) can fall upon different people (i.e., the module maintainers) instead of just the small number of folks working on migration. Many hands make light work, ya know. :)
Comment #2
alexpottIf I had such a tag I'd mark this as a "Favourite-of-alexpott". A huge +1
Core modules should (as far as possible) behave exactly the same as contrib... Views does not provide the content view - the Node module does. It should be the same for migrations.
Comment #3
mikeryanOne more comment on maintenance burden - it'll be a good thing for the module maintainers to see the migration code, and probably catch subtleties the migration developers miss. Right now the migration support is probably not getting viewed by the maintainers...
Comment #4
lostkangaroo commentedyes yes yes +1
Comment #5
phenaproximaComment #6
phenaproximaHere's how I think the existing migration templates (as well as all relevant source plugins and tests) should be organized:
action
aggregator
ban
Already handled in #2503049: Migrate D7 blocked IPs to D8.
block
block_content
book
comment
contact
field
file
filter
forum
locale
menu_link_content
node
search
simpletest
statistics
syslog
system
taxonomy
text
update
user
Comment #7
mikeryanComment #8
mikeryanComment #9
lostkangaroo commentedComment #10
benjy commentedDo we need a child issue for all of these? Can't we just move them in one hit?
Comment #11
mikeryanI know this was already discussed in IRC, but for anyone else with the same question (why not one big patch) - while most of these patches are very small (file moves plus a couple lines changed), some do require a bit more, such as moving source schemas. See the taxonomy patch (https://www.drupal.org/files/issues/move_module_specific-2534034-1.patch) for example.
Comment #12
mikeryanOK, all the move patches are ready - except they're all blocked on #2534158: MigrateFullDrupalTestBase must use dynamic test discovery (the full migration test has a laundry list of test classes and breaks when the classes move). When that's committed we'll set the rest of the children to "Needs review" and let testbot have its crack at them.
Comment #13
mikeryanAs I've been reviewing @phenaproxima's contributions here, it struck me that reviewing a patch can confirm that everything that's been moved has been correctly moved, but the patch itself won't reveal things that should move but haven't. That's difficult to do one-by-one, so I'm going to apply the full set of patches locally and give migrate and migrate_drupal a once-over to see if there's anything left in them that shouldn't be.
Comment #14
mikeryanAs I apply these patches, one issue is that we have multiple patches moving bits out of migrate_drupal.source.schema.yml - so, one patch will move its bit out and thus remove the context for the next patch to apply. As these patches are committed, there will be a few rerolls needed down the line.
Comment #15
mikeryanSo, I've found a few holes and set some of the child issues "Needs work". The rest I think are pretty much good to go. As for ordering, apart for the node moves needing to go before fields, the only issues are the migrate_drupal.source.schema.yml changes. This ordering of the involved modules seems semi-optimal (it avoids one or two rerolls, anyway):
menu_link_content- committedblock_content- committedtaxonomy- committedcontact- committedThe rest look well isolated and can be committed at any time.
Comment #16
mikeryanWe're pretty close now... Once all the existing children are committed, I'll do one last review to see if there's anything we missed before closing the meta.
Comment #17
webchickLast one is in: #2534012: Move module-specific migration support into the file module.
Go forth with your review. :)
Comment #18
mikeryanStragglers:
The file entity thing should be its own issue - should we have a single stragglers patch, or break it down my module? I'm hoping these should be pure file move/namespace update changes...
Comment #19
webchickI think one straggler patch which just moves all X to Y is completely fine. File entity as its own thing since it's trickier sounds good.
Comment #20
webchick#2546993: Move last stragglers of module-specific migrations into the appropriate modules got committed. #2548841: Remove entity:file destination schema from Migrate Drupal is the last one!
Comment #21
phenaproximaLook at all that green and red! Christmas came early.