$migration_dependencies has inconsistent structure

Personally, I prefer the expand-on-access strategy (#2 from the current Proposed resolution section of the issue summary). This is already the approach used in #2796755: [PP-1] Deprecate Migration::set() starting with the patch in #130.

Perhaps I am being over-cautious, but I am worried that a contrib or custom module might do something like this:

$migration = $plugin_manager->createInstance(...);
// Now $migration->migration_dependencies is expanded.
$migration->set('migration_dependencies', ...);
// Now it can be anything.
$dependencies = $migration->getMigrationDependencies();
// Will the code that follows break if we change $dependencies?

So I think the safest thing to do is

1. Deprecate getMigrationDependencies() and do not change its return value.
2. Add a new method, getExpandedDependencies(), that applies expandPluginIds() to the return value but does not change the class variable.
3. When we remove getMigrationDependencies(), go to a strict expand-on-access strategy.

For (2), other possible names are getDerivedDependencies() or getAllDependenciesIncludingDerivatives(). (shrug)

For (3), I mean we can update createInstances() so that it does not apply expandPluginIds().

As @alexpott mentioned in #2796755: [PP-1] Deprecate Migration::set(), getExpandedDependencies() is idempotent. That is, it is safe to apply it more than once (when setting the variable, when changing it, and when accessing it). Safe, but a little wasteful.

How should we handle expandPluginIds()?

In #2796755: [PP-1] Deprecate Migration::set(), we make that method public, so that it can be used from both MigrationPluginManager and Migration. In #146 on that issue, @jibran pointed out that this is a BC break. Perhaps we should update the policy to consider the plugin manager internal: #3262252: Add concrete plugin managers to the list of things considered internal.

If we want to keep the method protected, then one option is to add a trait with the method addDerivatives(array $migration_ids, MigrationPluginManagerInterface $manager). Then expandPluginIds() can be implemented as addDerivatives(..., $this) and getExpandedDependencies() can call addDerivatives(..., $this->migrationPluginManager).

A variant of that idea is for the trait to define expandPluginIds(array $migration_ids, ?MigrationPluginManagerInterface $manager). If $manager is empty, then replace it with $this. Then we do not need the wrapper function and we do not have to decide on a name for the new method. ;)

Am I over engineering?

Currently, createInstances() uses expandPluginIds() twice. The first call generates the list of migrations to instantiate. The second call expands the dependencies of each instantiated migration plugin. In #2, I suggested eventually removing the second call, but we will still need the first one. So we need to keep some form of expandPluginIds().

Ouch, that is a lot of test failures!

I thought I set the status to NR, but I guess it should now be NW until I get the tests to pass.

I think the latest commits will fix a lot of the failing tests.

Still NW for a deprecation test and an IS update.

Also, I think that adding an optional parameter to expandPluginIds() is too "clever". I want to use addDerivatives() as in #3.

Yesterday's commits fixed all the failing tests: better than I expected.

Today's commits add a deprecation test and added addDerivatives(): see #6 and the extended commit message. I am removing the issue tag for tests.

I am updating the issue summary, so I am removing that issue tag. Unless the testbot disagrees, I think this issue is ready for review.

If the tests pass, then I will add a change record first thing tomorrow.

I added a draft change record, so I am removing the issue tag.

I made some of the changes requested in the MR comments.

Re the approach: I guess the question is whether to expand dependencies when setting or when getting.

Given the work on the parent issue, I want to make it easy to implement the new add*Dependencies() methods. Once we deprecate and remove set() and getMigrationDependencies(), we will have the constructor and the two add* methods that change $migration_dependencies and one method that gets the value.

I think it is simpler to have the "expand" code in the one getter, instead of the three setters.

I am not sure, but I think it is also more efficient. Often, we will set once and get once. There are places we might set, add, add, and get. Are there any places where we will get several times after setting?

I think I have addressed all the comments on the MR, so back to NR.

I am adding credit to @heddn for some of those comments.

I have not yet incorporated the changes discussed on the MR into the issue summary.

I updated the change record, so I am removing that issue tag.

The change record has details about the deprecation messages and exceptions, but the issue summary does not. I am updating the issue summary a little, less than I anticipated in #14.

I believe that new deprecations have to target 10.1.x, so I rebased the feature branch on 10.1.x and updated the MR.

I also updated the deprecation messages: change 9.4.0 to 10.1.0 and refer to the CR for this issue, not the parent.

@alexpott, thanks for looking at this issue.

There are a couple of reasons for introducing the new trait, but none of them are really strong. I did not want to make the method public for BC reasons, but #21 points out that it is OK in this case. It seems to me that the method has nothing to do with migrations, so it makes sense to move it to the Drupal\Component\Plugin namespace. Then there is the vague hope that, once we have the trait, we might find other uses for addDerivatives() and/or we might find other methods that can be added to the trait.

I will make the suggested changes as soon as I can find some time.

I replied to the comments on the MR and updated it as follows:

1. Rebase, resolving add/add conflicts caused by #3198608: trackLastImported YAML key to enable tracking the last import date of a row is undocumented and confusing..
2. Remove the new trait. Instead, make expandPluginIds() public (#21).
3. Call MigrationPluginManager::getDefinitions() outside the loop.

The second change simplifies the array_map() in Migration::getExpandedDependencies(). It also simplifies MigrationTest, since only one method from the plugin manager has to be mocked, not two.

Back to NR.

I am adding one commit based on the latest comment on the MR.

I am also updating the proposed resolution for the changes I made yesterday. I will update the change record next.

In #21 and on the MR, @alexpott asked whether we considered making expandPluginIds() public. Reviewing the earlier comments on this issue, I am reminded that we did this on the parent issue, and #2796755-146: [PP-1] Deprecate Migration::set() pointed out that it is a BC break. But #21 gives me permission to make that break. :)

I am setting the status to NW to address @quietone's comments on the MR.

I replied to the MR comments, so back to NR.

@alexpott:

You make a good point about changing the value of the protected property $migration_dependencies. There are some mitigating factors: the situation you describe in #31 will give unexpected results only if the code uses the deprecated method getMigrationDependencies() AND uses the method set() (to be deprecated in the parent issue) without expanding dependencies. Also, we can avoid modifying the property whether we deprecate getMigrationDependencies() and add getExpandedDependencies() or just deprecate getMigrationDependencies() without setting the new parameter to TRUE.

I am worried about the BC implications of adding a parameter to a public method of the Migration class. Are we covered by the Drupal core backend backwards compatibility and internal API policy?

Particular plugins, whether class based or yaml based, should not be considered part of the public API. References to plugin IDs and settings in default configuration can be relied upon however.

I do not think the Migration class itself counts as a "particular plugin".

The last thing that bothers me about MR 3019 is what the final state will be. That is, in Drupal 10.1 we will deprecate all current calls to getMigrationDependencies(), replacing them with getMigrationDependencies(TRUE). What will we do in Drupal 11? Will we keep the new $expanded parameter and throw an exception if it is not TRUE? Will we change the default value of the parameter? Either way, the final state seems awkward. What if, in some future issue, we decide that we want to add a new parameter to the method?

We discussed this issue on Slack during #3322193: [meeting] Migrate Meeting 2022-12-01 1400Z. That issue will soon have a transcript.

I am still nervous about the BC/deprecation strategy, but I defer to @alexpott on this point. See my comment in #32. The plan is to remove the boolean $expand parameter in a future version, so I am happy with the planned final state.

I rebased MR !1797 on the latest 10.1.x and cherry-picked @alexpott's commit from !3019. I then added a few tweaks (in separate commits).

We need to update the change record (CR). I am adding the tag for that.

I am updating the issue summary.

Given that we plan to remove the $expand parameter, I wonder whether we should re-word the deprecation message. I need to sleep on that. Maybe it is fine as it is, and we can do the explaining in the CR.

@quietone:

Thanks for the careful review! I updated the change record to describe the current strategy. I did not suggest any planned future suggestions, such as removing the parameter that we are now adding.

@rkoller and I discussed this issue at #3327488: Drupal Usability Meeting 2022-12-23. That issue will have a link to a recording of the meeting.

Based on that discussion, I updated the change record (CR). I replaced "Specifically" with "In detail" (in two places).

I will take the last line of Comment #34 as permission to set this issue back to RTBC now that I have updated the CR.

Thanks, @alexpott!

I have un-postponed #2796755: [PP-1] Deprecate Migration::set(). I think that issue is close, now that this one is resolved.