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:

  1. 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.
  2. 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.
  3. 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:

  1. 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

  1. 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.
  2. Agree on principle to do this in general. Done.
  3. Open child issues for each affected module. Done.
  4. Fix the child issues.

User interface changes

N/A

API changes

Namespaces of affected plugins will change.

Data model changes

N/A

Comments

phenaproxima’s picture

HUGE +1 from me. This is long overdue, and Migrate Drupal is becoming a lumbering kaiju of a module.

Adds maintenance burden to the target modules

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. :)

alexpott’s picture

If 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.

mikeryan’s picture

One 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...

lostkangaroo’s picture

yes yes yes +1

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Here's how I think the existing migration templates (as well as all relevant source plugins and tests) should be organized:

action

  • d6_action_settings

aggregator

  • d6_aggregator_feed
  • d6_aggregator_item
  • d6_aggregator_settings
  • d7_aggregator_settings

ban

Already handled in #2503049: Migrate D7 blocked IPs to D8.

block

  • d6_block

block_content

  • d6_block_content_body_field
  • d6_block_content_type
  • d6_custom_block

book

  • d6_book
  • d6_book_settings

comment

  • d6_comment
  • d6_comment_entity_display
  • d6_comment_entity_form_display
  • d6_comment_entity_form_display_subject
  • d6_comment_field
  • d6_comment_field_instance
  • d6_comment_type

contact

  • d6_contact_category
  • d6_contact_settings

field

  • d6_field
  • d6_field_formatter_settings
  • d6_field_instance
  • d6_field_instance_widget_settings

file

  • d6_file
  • d6_file_settings
  • d6_upload
  • d6_upload_entity_display
  • d6_upload_entity_form_display
  • d6_upload_field
  • d6_upload_field_instance

filter

  • d6_filter_format

forum

  • d6_forum_settings

locale

  • d6_locale_settings

menu_link_content

  • d6_menu_links

node

  • d6_node
  • d6_node_revision
  • d6_node_setting_promote
  • d6_node_setting_status
  • d6_node_setting_sticky
  • d6_node_settings
  • d6_node_type
  • d6_view_modes

search

  • d6_search_page
  • d6_search_settings

simpletest

  • d6_simpletest_settings

statistics

  • d6_statistics_settings

syslog

  • d6_syslog_settings

system

  • d6_system_cron
  • d6_system_file
  • d6_system_filter
  • d6_system_image
  • d6_system_image_gd
  • d6_system_logging
  • d6_system_maintenance
  • d6_system_performance
  • d6_system_rss
  • d6_system_site
  • d6_url_alias

taxonomy

  • d6_taxonomy_settings
  • d6_taxonomy_term
  • d6_taxonomy_vocabulary
  • d6_term_node
  • d6_term_node_revision
  • d6_vocabulary_entity_display
  • d6_vocabulary_entity_form_display
  • d6_vocabulary_field
  • d6_vocabulary_field_instance

text

  • d6_text_settings

update

  • d6_update_settings

user

  • d6_profile_values
  • d6_user
  • d6_user_contact_settings
  • d6_user_mail
  • d6_user_picture_entity_display
  • d6_user_picture_entity_form_display
  • d6_user_picture_field
  • d6_user_picture_field_instance
  • d6_user_picture_file
  • d6_user_profile_entity_display
  • d6_user_profile_entity_form_display
  • d6_user_profile_field
  • d6_user_profile_field_instance
  • d6_user_role
  • d6_user_settings
mikeryan’s picture

Title: Move module-specific migration support into the particular modules supported » [meta] Move module-specific migration support into the particular modules supported
Issue summary: View changes
mikeryan’s picture

Issue summary: View changes
benjy’s picture

Do we need a child issue for all of these? Can't we just move them in one hit?

mikeryan’s picture

I 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.

mikeryan’s picture

OK, 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.

mikeryan’s picture

As 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.

mikeryan’s picture

As 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.

mikeryan’s picture

So, 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):

  1. comment
  2. menu_link_content - committed
  3. block_content - committed
  4. taxonomy - committed
  5. node
  6. field
  7. contact - committed
  8. file
  9. user

The rest look well isolated and can be committed at any time.

mikeryan’s picture

We'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.

webchick’s picture

Status: Active » Needs review

Last one is in: #2534012: Move module-specific migration support into the file module.

Go forth with your review. :)

mikeryan’s picture

Stragglers:

  • migrate.destination.entity:file should move to the file module, but currently that would break a number of tests - we need to clean up the whole migrate_drupal installMigrations() business in the tests before we can move this.
  • EntityDateFormat - the date_format entity is implemented in lib/Drupal/Core, so I think this should move to the system module.
  • EntityNodeType should move to the node module.
  • @phenaproxima - please remind me, the CCK field plugins like FileField, those will be going away in the aftermath of the builder patch, correct? If not, they should move to their respective modules.
  • Similar question on the CckFile and CckLink process plugins (although I'm pretty sure those won't be going away).
  • Some migrate_drupal D6 unit tests got left behind - TestComment, TestNode, TestNodeRevision, TestTerm.

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...

webchick’s picture

I 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.

webchick’s picture

phenaproxima’s picture

Status: Needs review » Fixed

Look at all that green and red! Christmas came early.

Status: Fixed » Closed (fixed)

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