Problem/Motivation

The $migration argument of \Drupal\migrate\Plugin\MigrateDestinationInterface::fields is 

   * @param \Drupal\migrate\Plugin\MigrationInterface $migration
   *   Unused, will be removed before Drupal 9.0.x. Defaults to NULL.

This was added in #2543568: Remove the md_entity destination plugin hack.

Proposed resolution

Work out what to do.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 interdiff.txt1.19 KBandypost
#10 3114974-10.patch7.77 KBandypost
#5 3114974-5.patch7.41 KBandypost
#5 interdiff.txt1.21 KBandypost
#3 3114974-3.patch7.62 KBkishor_kolekar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

kishor_kolekar’s picture

kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commented
Assigned: Unassigned » kishor_kolekar
kishor_kolekar’s picture

kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commented
Status: Active » Needs review
FileSize
3114974-3.patch7.62 KB

Removed the argument from the method fields() from the interface MigrateDestinationInterface and from all its implementations.
please review the patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Needs work 
+++ b/core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php
@@ -133,4 +133,8 @@ public function getDestinationModule() {
+  public function fields(MigrationInterface $migration){
+
+  }

We don't need to add it :)

The big questions to be worked out is a) why does this exist - it does not appear to be called from core code and b) it is used by contrib migrate projects in anyway?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
Related issues: +#2543568: Remove the md_entity destination plugin hack
FileSize
interdiff.txt1.21 KB
3114974-5.patch7.41 KB

It was decided useless in #2543568: Remove the md_entity destination plugin hack
Checked in contrib and migrate tools does not use the argument

Here's a patch to re-roll and remove mentions

kishor_kolekar’s picture

kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commented

hi @alexpott I have found this has been used in migrate_plus and field_group contrib module .

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs release manager review

Shockingly, I think this is actually okay to go ahead and do. Yes, migrate_plus and field_group, and any other module that defines a destination plugin does have the argument in their method signature --- they have to, it's in the interface. However neither of them use the argument --- they can't, we never pass it when we call the method. This is all okay. You can add optional arguments to an method with a defined interface in php, so their function signatures are still valid if we remove the argument from the interface. Likewise, if someone somewhere for some reason happens to call this method and pass a migration, it's still fine, you can call a method with extra arguments and they will be ignored. If somewhere in contrib land someone used this parameter in the method and had code that called it with a migration, it's still fine. That would just keep working.

We lucked out in that the parameter is defined as optional in the original interface, because of that we can actually safely remove it. There really wouldn't be a way to trigger an error to deprecate removing a parameter from an interface anyway, so at the end of the day I say we commit this to 9.0 and move on if we can get RM blessing on the rather odd removal.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Assigned: kishor_kolekar » Unassigned
Issue tags: +Drupal 9

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3114974-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
3114974-10.patch7.77 KB

Fix CS

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt1.19 KB
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Status: Needs review » Reviewed & tested by the community

This was RTBC just got bumped back from testbot. Back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3114974-10.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Reviewed & tested by the community

  • catch committed 9cf5c69 on 9.1.x 
    Issue #3114974 by andypost, kishor_kolekar, mikelutz, alexpott: \Drupal\...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.0.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

If we had a base class here or similar, we could trigger a deprecation notice when the unnecessary parameter is passed. However we don't have that, and adding the logic to every implementation seems like overkill here.

One problem as pointed out by @alexpott in slack is that if we added a new parameter to the method in 10.x it would conflict with passing the old parameter in. In that case we'd need to do stage adding the method via func_get_args() with a deprecation notice, but I think we tackle that if we get there - adding a new param to an interface is tricky anyway.

Committed 9cf5c69 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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