In the parent issue, we plan to add the methods addRequiredDependencies() and addOptionalDependencies() to Drupal\migrate\Plugin\Migration. This issue is to add those two methods now, instead of waiting for the parent issue.
The reason is that we have already removed the option of calling getMigrationDependencies() without expanding dependencies. Some contrib modules use the pattern
$dependencies = $migration->getMigrationDependencies() + ['required' => []];
$dependencies['required'] = array_unique(array_merge(array_values($dependencies['required']), [$migration_dependency]));
$migration->set('migration_dependencies', $dependencies);
in a deriver (or in code that gets called from a deriver). That can lead to an infinite loop.
See #3433816: Automated Drupal 11 compatibility fixes for paragraphs.
Copy the code for addRequiredDependencies() and addOptionalDependencies() from the parent issue.
Review
Review change record
None
None
Add two public methods to Drupal\migrate\Plugin\Migration.
For BC reasons, do not add the new methods to the interface in this issue, but add them in a follow-up issue or in the parent issue.
None
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 3464320-nr-bot.txt | 2.8 KB | needs-review-queue-bot |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !8962
Comments
Comment #2
benjifisherAnother possibility: the current code both deprecates calling
getMigrationDependencies()with the$expandparameter and ignores that parameter if it is provided. Maybe we should deprecate it but not ignore it. Then a contrib module could callgetMigrationDependencies(FALSE)to avoid the recursion.We might do this in addition to the proposed resolution in the issue summary.
Comment #4
benjifisherComment #5
benjifisherComment #6
catchThis looks fine to me, adding to the interface should be OK under the 1-1 rule, but is the intention to backport this to 11.0/10.4 too? If so it might make sense to be extra extra cautious.
Comment #7
benjifisherI think that calling
getMigrationDependencies(FALSE)is deprecated in 10.4.x but triggers an error in 11.0.x, so I would like to backport this to the 11.0.x branch. Or maybe it is only deprecated in 11.x, so module maintainers remove it so their tests can pass, but then they get the infinite loop. One way or another, it is worse in 11.x (and probably 11.0.x) than in 10.4.x.From the issue summary:
Comment #8
smustgrave commentedLeft some nitpicky threads on the MR.
But for the new methods should a CR be written.
With my limited migration eyes the rest looks good though.
Comment #10
quietone commentedThe test failure is MigrateNoMigrateDrupalTest.php which is soon to be fixed by #3466289: MigrateNoMigrateDrupalTest fails with missing classes in certain situations.
I added a CR, applied existing suggestions, and added 2 of my own.
Comment #11
benjifisherI added an example to the CR based on the changes in
core/modules/migrate_drupal/src/Plugin/migrate/field/ReferenceBase.php.The code changes look good to me, but I cannot declare the issue RTBC since I did the initial work on the PR. Since I extracted the new code from the old work on the parent issue, it is not surprising that I missed the return-type declarations. I am surprised that my initial version passed static analysis. I guess that is because many existing functions do not have return-type declarations, so we do not require them in static analysis.
Comment #12
smustgrave commentedMay be related?
Comment #13
quietone commentedAlthough it is a migration test it is not related to the additions made here.
Unfortunately, I can't RTBC this either as I wrote much of the code in the originating issue this was taken from, #2796755: [PP-1] Deprecate Migration::set().
Comment #14
smustgrave commentedDid another lap and believe all feedback has been addressed.
Comment #15
alexpottMigrateNoMigrateDrupalTest::testExecutionNoMigrateDrupal is failing.
Comment #16
benjifisherI rebased on the current 11.x branch. In particular, the feature branch now includes the fix for #3466289: MigrateNoMigrateDrupalTest fails with missing classes in certain situations. As @quietone predicted (#10, #13), that fixes the failing test.
Given that I did not do any new coding, I am making an exception to the usual practice: back to RTBC.
Comment #17
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #18
benjifisherI added the missing return types in the unit test (
voidfor the test methods andarrayfor the data providers). Back to NR.Comment #19
smustgrave commentedBelieve feedback for this one has been addressed, around test failure and review bot.
Comment #21
catchCommitted/pushed to 11.x, thanks!
Comment #24
heddnThe addition of this seems to have added an infinite loop via field discovery calling
getMigrationDependencies. Will start looking for a bug report, but wanted to mention this here in case someone else has already surfaced this.Comment #25
heddnI've opened #3537372: Calling $migration->getMigrationDependencies() from a field plugin results in an infinate loop.