Drupal\migrate\Plugin\Migration should provide a way to add dependencies

Another possibility: the current code both deprecates calling getMigrationDependencies() with the $expand parameter and ignores that parameter if it is provided. Maybe we should deprecate it but not ignore it. Then a contrib module could call getMigrationDependencies(FALSE) to avoid the recursion.

We might do this in addition to the proposed resolution in the issue summary.

This 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.

... is the intention to backport this to 11.0/10.4 too?

I 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:

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.

Left 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.

The test failure is MigrateNoMigrateDrupalTest.php which is soon to be fixed by 🐛 MigrateNoMigrateDrupalTest fails with missing classes in certain situations Fixed .

I added a CR, applied existing suggestions, and added 2 of my own.

I 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.

    1)
    Drupal\Tests\migrate\Functional\MigrateNoMigrateDrupalTest::testExecutionNoMigrateDrupal
    Behat\Mink\Exception\ResponseTextException: The text "Migration was
    successful." was not found anywhere in the text of the current page.
    
    /builds/project/drupal/vendor/behat/mink/src/WebAssert.php:907
    /builds/project/drupal/vendor/behat/mink/src/WebAssert.php:293
    /builds/project/drupal/core/tests/Drupal/Tests/WebAssert.php:979
    /builds/project/drupal/core/modules/migrate/tests/src/Functional/MigrateNoMigrateDrupalTest.php:50
    
    FAILURES!

May be related?

Although 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, 📌 [PP-1] Deprecate Migration::set() Postponed .

Did another lap and believe all feedback has been addressed.

MigrateNoMigrateDrupalTest::testExecutionNoMigrateDrupal is failing.

I rebased on the current 11.x branch. In particular, the feature branch now includes the fix for 🐛 MigrateNoMigrateDrupalTest fails with missing classes in certain situations Fixed . 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.

The 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.

I added the missing return types in the unit test (void for the test methods and array for the data providers). Back to NR.

Believe feedback for this one has been addressed, around test failure and review bot.