Problem/Motivation

MigrationPluginManagerInterface has the following added to its interface:
createInstances
createStubMigration

But it is missing createInstancesByTag. I suggest this is an oversight and should be added.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#6 2912803-6.patch1.56 KBjofitz
#2 2912803-2.patch1.56 KBjofitz

Comments

heddn created an issue. See original summary.

jofitz’s picture

jofitz
he/him
commented
Status: Active » Needs review
StatusFileSize
new 2912803-2.patch1.56 KB
  • Added createInstancesByTag to MigrationPluginManagerInterface.
  • Updated docs of createInstancesByTag in MigrationPluginManager.
maxocub’s picture

maxocub commented
Title: MigrationPluginManagerInterface is missing createInstancesByTag » MigrationPluginManagerInterface is missing createInstancesByTag
Status: Needs review » Needs work
Issue tags: +Needs change record

I think this needs a change record.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

CR added.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2912803-2.patch, failed testing. View results

jofitz’s picture

jofitz
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new 2912803-6.patch1.56 KB

Patch in #2 no longer applies. Re-rolled.

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

Test passing so back to RTBC

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs review

Hm, this could potentially be a can of worms AKA it definitely looks like a backwards incompatible change. If contribs / custom modules don't implement this they would whitescreen right after updating. Any other options?

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

I agree wwith the sentiment in #8. However, if I stop and thing about how and why someone would alter out the plugin manager... then I realize that no one has done that. It is a pretty difficult activity for no real benefit. I doubt anyone has done that. Isn't there some docs about implementations of interfaces by a single class and that essentially being internal?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

It seems quite unlikely that anyone will have implemented their own MigrationPluginManagerInterface from scratch. The only example I can think of in core is over in #2908282: Throw exception for source plugins without a source_module property, in which Migrate Drupal does replace the plugin.manager.migration service -- but it does it by extending the base class.

I think, in practice, this is a BC-safe change, so I'm marking it RTBC. But if the framework/product managers disagree, then we'll see where we can go from here...

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Needs framework manager review, +Needs release manager review

I'm not sure on this either.

The alternative is to add a new interface and have MigrationPluginManager implement it.

We can then remove it and merge them in D9.

I've asked the other framework managers and the release manager.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2912803-6.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2912803-6.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Reviewed & tested by the community

Random failure

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs framework manager review, -Needs release manager review

Yes, our API policy allows us to add methods to interfaces that have a corresponding base class that would be used in most cases:
https://www.drupal.org/core/d8-bc-policy#interfaces

So this is OK with a change record (which it has).

Thanks!

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Issue tags: +Migrate January 2017 Sprint
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Issue tags: -Migrate January 2017 Sprint +Migrate January 2018 Sprint
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed d778f80 and pushed to 8.5.x. Thanks!

  • catch committed d778f80 on 8.5.x 
    Issue #2912803 by Jo Fitzgerald, heddn: MigrationPluginManagerInterface...

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented

Published change record.