Problem/Motivation

in Drupal 8 there was a Migration::get() method. It was appropriately deprecated but I missed that It was being used in Migrate Manifest to bypass the lack of an accessor for listing requirements. Since it is removed in Drupal 9 and there is not replacement, Migrate Manifest is not currently able to upgrade.

Proposed resolution

Add a "getRequirements()" method to Migration class and the MigrationsInterface

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3154398-12.patch1.85 KBquietone
#12 interdiff-10-12.txt463 bytesquietone
#10 3154398-10.patch1.86 KBquietone
#10 interdiff-7-10.txt958 bytesquietone
#7 3154398-7.patch1.84 KBneclimdul
#6 3154398-6.patch1.84 KBneclimdul
#4 3154398-3.patch1.01 KBneclimdul
#3 3154398-1.patch1 KBneclimdul
#2 3154398-1.patch1 KBneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
Status: Active » Needs review
FileSize
3154398-1.patch1 KB

MigrationInterface might not actually be the right place. Here's a patch with the method on RequirementsInterface.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
Issue tags: +Needs tests
FileSize
3154398-1.patch1 KB

yeah... nope. That's going to fail hard. Requirements interface has a bunch of different contexts its used in and most can't list plugin ids. Going to have to be specific to migrations.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
FileSize
3154398-3.patch1.01 KB

Lets see how testbot feels about this.

dinarcon’s picture

dinarcon CreditAttribution: dinarcon at UnderstandDrupal.com commented
Related issues: +#3117485: The "--execute-dependencies" option is currently broken due to the deprecation of the "get('requirements')" method

Thanks for getting a fix started @neclimdul. This is also affecting the --execute-dependencies flag in Migrate Tools and (probably) Migrate Run. Referencing a related issue where adding a new interface is mentioned.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
Issue tags: -Needs tests
FileSize
3154398-6.patch1.84 KB

I knew that looked too clever and I didn't remember writing it. I'm sure I stole the idea from Migrate Tools. :-D

Here's a patch with tests. The method doesn't really do much so the test is pretty trivial.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
FileSize
3154398-7.patch1.84 KB 
+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -420,6 +420,13 @@ public function getIdMap() {
+   * {@inheritDoc}}

You know, sometimes you don't see things until you see it in the patch review...
}}

Sorry for being lazy about the interdiffs, I have a couple patches i'm working on in the same branch so its a pain. #6 just added the tests. This just fixes the }} in the doc.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

Easy enough change here. +1.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

I think this is allowed under our 1-1 interface to class rule - but given we're adding a method to an interface I think we should have a change record.

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -420,6 +420,13 @@ public function getIdMap() {
+  public function getRequirements() {

+++ b/core/modules/migrate/src/Plugin/MigrationInterface.php
@@ -103,6 +103,13 @@ public function id();
+  public function getRequirements();

To prevent future re-work let's add a return typehint. Yes the mix of methods without a return hint and ones with is meh but if adding new things without out this is adding to future work.

quietone’s picture

quietone CreditAttribution: quietone commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
FileSize
interdiff-7-10.txt958 bytes
3154398-10.patch1.86 KB

#10 Added a CR and here is a patch with the return type declarations.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented 
+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -417,6 +417,13 @@ public function getIdMap() {
+  public function getRequirements() : array {

+++ b/core/modules/migrate/src/Plugin/MigrationInterface.php
@@ -103,6 +103,13 @@ public function id();
+  public function getRequirements(): array;

I'm not sure what our code style for return types is but I assume the spacing on these should be consistent?

quietone’s picture

quietone CreditAttribution: quietone commented
Issue tags: -Needs change record
FileSize
interdiff-10-12.txt463 bytes
3154398-12.patch1.85 KB

I couldn't find anything for return types in the coding standards but grep shows the convention is the second one, public function getRequirements(): array;. Updated the patch accordingly.

There is a change record, so removing tag.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
Status: Needs review » Reviewed & tested by the community

awesome. I think this can go back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 3154398-12.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs work » Reviewed & tested by the community

  • larowlan committed 90c7d4c on 9.1.x 
    Issue #3154398 by neclimdul, quietone, alexpott: Migrations don't have...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Reviewed & tested by the community » Fixed

Committed 90c7d4c and pushed to 9.1.x. Thanks!

Published the change-record.

Didn't backport this because there's an outside chance of disruption.

Status: Fixed » Closed (fixed)

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