[PP-1] Deprecate Migration::set()

@benjifisher, sorry it has taken awhile to get to this.

I have worked through most of #172 and you will see the tests should be more straightforward and with better names. I was hoping to a do a self review before uploading the patch but it is now too late for that.

#172
4. Added TestMigration::getMigrationDependenciesRaw() and updated tests. There is now a testGetMigrationDependencies and testSetMigrationDependencies. That should make things clearer.
5. This is now correct.
6,7. Making this change causes phpcs to fail.
8. Checked that @covers in all new tests. I hope the are correct now.
9. Should be restored to the original test, which does have the @throws.
10. Perhaps it has to do with execution time, https://hakre.wordpress.com/2011/03/29/php-is_null-vs-null
11. Checked that @covers in all touched tests. I hope the are correct now.
12. The helper is made (although I disagree in this case). However, I did not use it in testMigrationDependenciesWithValidConfig() to minimize the changes to an existing test.
13. Done
14. Done
15, 16, 17. Done. I have not looked at why the array indices are different for the data set 'add existing' in the two test methods.
19. Done

Todo
#168
#172 1, 2, 3.

+++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
@@ -118,10 +118,9 @@ public function createInstances($migration_id, array $configuration = []) {
-    // @todo Remove loop when the ability to call ::getMigrationDependencies()
-    //   without expanding plugins is removed.
+    // @todo Remove loop. https://www.drupal.org/node/3332807
     foreach ($instances as $migration) {
-      $migration->set('migration_dependencies', $migration->getMigrationDependencies(TRUE));
+      $migration->setMigrationDependencies($migration->getMigrationDependencies(TRUE));
     }

I really do not think we should be adding setMigrationDependencies() - there's no need to support this once we're on Drupal 11. Maybe we just need to get there before we can deprecate Migration::set()

Now, to remove setMigrationDependencies per #168 and #175. Without that method #172, points 1 and 2 no longer apply. That leaves, #172.3 and yes I think the @legacy is sufficient.

I am looking at some of the remaining points in #168.

I'm wondering about the use-case of programatically adding an optional dependency. Also I'm wondering about the use-case of adding multiple dependencies at the same time.

Comment #81 has a link to the search page for contrib modules, and Comment #83 reviews several of those modules. I had another look at both.

• The media_migration module sets multiple required dependencies at once, but it can be simplified based on 📌 $migration_dependencies has inconsistent structure Fixed and then add the dependencies one at a time. In another place, it adds two required dependencies.
• The location_migration module adds two required dependencies. (Something like a field and a field instance.)
• The paragraphs module adds one required dependency and conditionally adds another; it does the same thing in two places. In its test code, it resets the dependencies, which will take a little work to re-implement.
• The paragraphs_migration module really adds multiple required dependencies at once. The goal of this module is to have its code merged into the paragraphs module.

Based on that, I think there is a use case for adding several required dependencies at once. Of course, it is easy enough to write a loop, but it seems sloppy (not a serious performance problem) to call array_unique() multiple times.

I do not see any place where optional dependencies are added.

I'm also struck by the concept of the requirements property being a list of migrations that have to have completed and a dependencies array with a required key - how come these are not always the same? I wonder if instead of setting requirements on the migration we could use the list of required migrations and do something recursive like: ...

Can we handle that in a follow-up issue? We already added a follow-up, 📌 Remove unused class property \Drupal\migrate\Plugin\Migration::$dependencies Fixed , based on an earlier comment.

Trying to follow with what's going on

📌 Remove unused class property \Drupal\migrate\Plugin\Migration::$dependencies Fixed has been committed

Guess the only outstanding question is if that last part of #176 should be a follow up or not?

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

This needed a reroll and then I was getting failures from PHPStan. Turns out I made a mistake in the reroll. That turned out to be helpful because I then saw a comment needed to be removed from \Drupal\Tests\migrate\Unit\TestMigrationMock. This patch is a reroll with that comment removed.

I do not understand how it is safe to remove this code block:

    +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -118,12 +118,6 @@ public function createInstances($migration_id, array $configuration = []) {
    ...
    -    // @todo Remove loop when the ability to call ::getMigrationDependencies()
    -    //   without expanding plugins is removed.
    -    foreach ($instances as $migration) {
    -      $migration->set('migration_dependencies', $migration->getMigrationDependencies(TRUE));
    -    }

We added the @todo comment in 📌 $migration_dependencies has inconsistent structure Fixed . In that issue, we decided on a strategy of "validate on set, expand on get", but for backwards compatibility (BC) we do not implement that strategy directly. We provided $migration->getMigrationDependencies(FALSE) as a (deprecated) way to get the dependencies without expanding. If we remove this code block in createInstances() and some contrib or custom code gets the dependencies without expanding, then that code may stop working.

The suggestion that we not provide setMigrationDependencies() was first made in Comment #174:

I really do not think we should be adding setMigrationDependencies() - there’s no need to support this once we’re on Drupal 11. Maybe we just need to get there before we can deprecate Migration::set()

If we postpone this issue until Drupal 11, then I agree that we can remove the code block in createInstances(), but I would really like to finish off this issue, not postpone it. Can we compromise? That is, can we add setMigrationDependencies() but immediately deprecate it, then remove it along with the code block in createInstances()? If we do that, then we have to restore the test coverage that was removed in #175.

I am also tagging this issue for an update to the change record (CR). It still says that these changes will be made in Drupal 9.3, and it uses setDependencies() instead of setMigrationDependencies(). We should come to agreement on that method before updating the CR.

I created the follow up for $requirements, 📌 Remove \Drupal\migrate\Plugin\Migration::$requirements Active

That is, can we add setMigrationDependencies() but immediately deprecate it,

Except that then a deprecated method, setMigrationDependencies, is called when creating migrations and triggering an error.

I am uploading a patch but not running tests because they fail for the reason above.

I have not worked on the change record.

Once again, I am postponing this issue on a newly created child issue: 📌 Expand $migration_dependencies when it is set Active .

I think we can get around the difficulty discussed in Comments #180, #181 if we replace the strategy "validate on set, expand on get" from 📌 $migration_dependencies has inconsistent structure Fixed with "validate and expand on set". Since that issue has been fixed on the 10.1.x branch, and not yet been part of a release, it is not too late to make this change in approach.

@quietone and I discussed this new option on 📌 [meeting] Migrate Meeting 2023-03-30 2100Z Fixed and agreed to give it a try.