Problem/Motivation

Ideally, the migrate source plugin should be nothing else but an iterator over the source data. We have piled a lot of code into SourcePluginBase (which previously was a Source wrapper though) especially next().

Proposed resolution

We do two things in prepareRow // next: add additional additional data and filter rows. So let's add two new plugin types and make source into a single pipeline (process is multiple pipelines, in comparison). The first step is always an iterator plugin exactly as now. The plugin would also define fields and ids as now. Instead of Iterator I'd have it implement IteratorAggregate . Note how little most (all?) source plugins would need to change.

And then we would have new plugin types: one for adding data to the source and another for filtering data. The current filter plugins are: id, highwater, status and changed. We could add an empty plugin as well, for example to immediately skip disabled blocks. This would be so much cleaner than hook_prepare_row, gosh.

It is an interesting question where do we put the invocation logic for this -- I guess MigrateExecutable would do. It's not a lot of code, after all.

For backwards compatibility, the iterator plugin will be inspected for implementing the old MigrateSourceInterface and if so, then its prepareRow will be treated as an addition plugin.

New syntax:

source:
  iterator: d7_node
  add:
    -
      plugin: d7_fields
  filter:
    -
      plugin: highwater

Remaining tasks

Code this. It's just reshuffling existing code.

User interface changes

None.

API changes

We add a NewMigrateSourceInterface which will only be an iterator (and fields and id) and two new plugin types.

Data model changes

None.

Original issue statement

Ideally, migration source plugins (which control iteration over and delivery of data from a data store) should be usable by themselves, without a containing migration - unfortunately, MigratePluginBase requires a MigrationInterface argument. We already have concrete use cases for this - #2530030: Create the migrate builder plugin type is forced to construct a dummy migration to use a source plugin, and in contrib we want to bring the D7 drush migrate-analyze command (doing data analysis on source data) forward.

This is an API breaker, and there are already source plugins in the wild being developed, so we recognize it may need to be postponed until 9.0, but let's at least get this on the table.

In a quick scan of SourcePluginBase, I identified these uses of the migration:

  1. idlist handling - I'm hoping that event support (particularly a prepareRow event) will mean we can take this out of core entirely and let the contrib tools handle it themselves. Done
  2. Highwater marks - We could have distinct APIs specifically for dealing with highwater marks, rather than going through the migration class.
  3. Id map plugins - This is an unnecessary coupling in itself, but thoroughly embedded. At least as a first pass we could pass the id map plugin instead of the migration class so we can at least decouple those, and deal with source/map decoupling in a separate issue.
  4. SqlBase also uses the migration in tagging the query - the "migrate_[migration_id]" tag could be addressed with a "query_tag" argument. The migration itself is added as metadata to the query itself - we should look into what use cases there are for this. If it is necessary, the containing migration could do a query alter to add it.

We also should scan specific source plugins for any other uses of the migration.

CommentFileSizeAuthor
#16 interdiff.txt853 bytesdawehner
#16 2543552-16.patch23.76 KBdawehner
#15 2543552-15.patch21.79 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Issue summary: View changes
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Version: 8.0.x-dev » 9.x-dev
Status: Active » Postponed

Not realistic in the 8.x timeframe.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.x-dev » 8.2.x-dev
Status: Postponed » Active

Moving back to 8.x, I don't see anything here that couldn't theoretically be done with a backwards compatibility layer.

chx’s picture

chx CreditAttribution: chx as a volunteer commented
Title: Reduce/remove tight coupling of migration source plugins » Modernize migration source plugins
Issue summary: View changes
Priority: Normal » Major
Issue tags: +Migrate critical
chx’s picture

chx CreditAttribution: chx as a volunteer commented
Issue summary: View changes
benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented

The proposal sounds good to me, I'm sure I've discussed this previously with chx. Not sure it's a Migrate Critical though, thought we were using that tag to track issues we needed in for Migrate to be stable?

chx’s picture

chx CreditAttribution: chx at Migrate Rocks commented

> Not sure it's a Migrate Critical though, thought we were using that tag to track issues we needed in for Migrate to be stable?

I'd rather not have the current migrate source structure marked stable.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan commented
Issue summary: View changes
chx’s picture

chx CreditAttribution: chx as a volunteer commented
Issue summary: View changes
chx’s picture

chx CreditAttribution: chx as a volunteer commented
Issue summary: View changes
chx’s picture

chx CreditAttribution: chx at Migrate Rocks commented
Assigned: Unassigned » chx

Working on this. Won't be quick. Want to help? It's in the sandbox, branch is migrate-next. If you need commit privileges just ask although most of the usual suspects already have.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

How does

For backwards compatibility, the iterator plugin will be inspected for implementing the old MigrateSourceInterface and if so, then its prepareRow will be treated as an addition plugin.

align with #7. If we can implement a BC layer then surely the core source plugins don't have to change?

chx’s picture

chx CreditAttribution: chx at Migrate Rocks commented
Assigned: chx » Unassigned

Actually, as it shows, not working on it. I do not have the mental fortitude required any more to pour untold amount of hours into core just because it'd be nicer some time in the future. Still, this hopefully needs to get done. By someone else.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Issue tags: +Drupalaton
FileSize
2543552-15.patch21.79 KB

Here is some form of start we (chx, dawehner) worked together.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
FileSize
2543552-16.patch23.76 KB
interdiff.txt853 bytes

Some progress.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: -Migrate critical

Discussed with @benjy. We agreed that this is a good idea and certain improves the migrate system. We also agreed that the current solution and problem scope is not on the critical path to getting migrate stable. We also agreed that this improvement could go into 8.x if it has a BC layer and we would deprecate the current behaviour.

@chx thanks for posting the issue, working on migrate, and pouring untold amounts of hours into core. I hope this does happen in 8.x it is just that getting migrate stable and all the existing bugs fixed is more important than good architectural improvements at this point.

@dawehner I hope this comment does not discourage you from working on this because if you can conjure up a BC layer it'd be great :)

chx’s picture

chx CreditAttribution: chx at Migrate Rocks commented

While I have my doubts whether it's possible to get migrate stable without this patch, here are some problems:

  1. At least highwater support is completely broken and while https://www.drupal.org/node/2485385 fixes it to an extent, this is the proper fix.
  2. The id list feature got removed and it would be really nice to have it back (and would be utter trivial after this).
  3. Finally if you remove the "experimental" flag from migrate with hook_prepare_row then that's part of the API and I want to remove it ASAP. It's a horrific hook.
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@chx - just looking at point 3 - well nothing in core is using hook_migrate_prepare_row() so if there's no use-case in contrib atm we could remove that in a very small patch no?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I spoke to @mikeryan and he said that migrate_plus exposes this as an event and custom migrations he has worked on use it. So if we remove it we'd need an adequate replacement. @chx what do you mean by horrific - hook_module_implements_alter() is horrific but otoh very powerful when you need it. Not all hooks are designed to be safe. Maybe they should be but we certainly are not there in D8.

chx’s picture

chx CreditAttribution: chx at Migrate Rocks commented

The problem is not hook vs event but the double role of hook_prepare_row of adding AND filtering and the filtering is really badly obscure.

And no, you can't remove it, many custom site migrations (like mine) use it to add additional data to the existing core plugins. Would need to extend the core plugin and write mine for each, such bother.

And I do not know how to express this in a polite way but anything in migrate_plus and migrate_tools should not steer core. Quite the contrary and even that is refused.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

@chx
Please be calm, see http://xjmdrupal.org/blog/someone-worked-hard-on-it

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented
Priority: Major » Normal

Migrate API is now beta, and it's hard to see how to do a BC layer for this. High priority when getting into refactoring for D9, but for now the current source plugin architecture works fine for our needs.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Active » Closed (won't fix)

I think this issue has served its purpose during the life span of D8. In D9 we might look into this more, but it will probably do things in entirely new and different ways than what we've discussed in here. And most likely start in a sandbox/contrib project. So for now, marking this closed.