Remove deprecated code from migrate module

This should be limited to the deprecations in the Migrate module only.

Deprecated source, field, process and destination plugins are part of Migrate Drupal and are being removed in #3572280: Remove Migrate Drupal. The entries in core/.deprecation-ignore.txt are also for Migrate Drupal, not the Migrate API.

Opened #3577140: EntityContentBase make all parameters required for the one leftover. This should be ready for review.

I am adding #3261004: Remove deprecated code from the migration system as a related issue. As we are discussing on the MR, the patch for that issue introduced an error that we now have to clean up.

On the MR and on Slack, I argued that we should re-deprecate letting $account_switcher be NULL and throw an exception (or an error) when $entity_type_bundle_info is NULL. After thinking about it some more, I have changed my mind. I think we can now simply make both parameters ($account_switcher and $entity_type_bundle_info) required.

For the record, this is what I worried about and why I think it is not actually a problem.

Suppose

1. A site has a custom destination plugin that extends EntityContentBase.
2. The constructor calls parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $entity_field_manager, $field_type_manager) (and has not been updated in several years).
3. None of the migrations using this destination plugin use the configuration validate: true.

When the $account_switcher parameter was added in Drupal 9.3.0, the code in (2) started triggering deprecation errors, but maybe the site owner did not notice them.

In Drupal 10.0.0, the deprecation notices stopped because they were removed in #3261004. Because that issue did not make the parameter required, and because of (3), nothing bad happened.

In Drupal 11.2.0, we added new deprecation notices: not for $account_switcher but for $entity_type_bundle_info. I think that is good enough.

Since the code has been generating deprecation notices since Drupal 11.2.0, I think it is OK to make both parameters required now that we are preparing for Drupal 12.

I am setting this issue to NW so that we can remove the deprecated code in EntityContentBase. If we agree to that plan here, then I think we can close #3577140.

Personally doesn’t matter to me but another reason we did the separate ticket for FormBuilder was for the git history

I don’t think I can make it though

@smustgrave: I am not sure what you mean in Comments #11 and #12.

@andypost: Thanks for the updates.

I checked the change record https://www.drupal.org/node/3533565 and noticed that this issue does not remove the import() method in the Comment module. It looks as though that was already done in #3574661: Remove deprecated code from comments module, so I am adding that as a related issue.

I will continue my review later.

From Comment #2:

This should be limited to the deprecations in the Migrate module only.

That restriction never made it into the issue summary, but the current MR makes five exceptions:

1. Three destination plugins have deprecated import() methods, and these are being removed.
2. Two destination plugins extend EntityContentBase and have their own constructors. These two classes are updated, making all constructor parameters required.

I do not care about (1). I am just pointing out that it conflicts with Comment #2. I think it will not even cause merge conflicts, since Git is pretty good at recognizing when "the same" change is made in separate branches.

I think it makes sense to do (2) as part of this issue. In fact, PHPStan may insist that we do it as part of the same issue.

Also from Comment #2:

The entries in core/.deprecation-ignore.txt are also for Migrate Drupal, not the Migrate API.

That file is not touched by the current MR, so I am removing it from the issue summary.

I finished my review, and I left some comments on the MR. Back to NW.

@andypost:

Thanks for the updates, and for adding testMigrationDependenciesWithRemovedExpandArgument(). Everything looks great!

One comment on the MR.

To keep the deprecation removals moving went ahead and removed the check. Since it was such a small change moved to RTBC.

The test for it will also need to be removed.

Oh yea removed

All green

Can we go back in time and change the error message from

'Calling ' . __METHOD__ . ' with the $expand parameter is deprecated in drupal:11.0.0 and is removed drupal:12.0.0.

to

'Calling ' . __METHOD__ . ' with the $expand parameter is deprecated in drupal:11.0.0 and has no effect in drupal:12.0.0.

Or go back further in time and never add the parameter in the first place? (Mea culpa.)

I edited the change record mentioned in the deprecation message: https://www.drupal.org/node/3442785.

Haha you bring up a good point though. There’s a few things I’d like to change about deprecations and maybe one should be if we are saying it will throw an exception we should be 100% sure about that and mention what kind of exception. There were a few spots where it just exception (not here) but didn’t say which

Committed/pushed to main, thanks!

@benjifisher we could do an 11.x change to the message if we want. We should maybe add that language as a template to the removal docs for future deprecations since it comes up often enough and that's a lot clearer.

@catch:

I added #3582125: Improve the deprecation message in getMigrationDependencies(), now NR.

We should maybe add that language as a template to the removal docs for future deprecations since it comes up often enough and that's a lot clearer.

Can you give me a pointer to the removal docs?

Should I also update How to deprecate > Method parameters?

@benjifisher what you found is exactly what I was referring to as 'removal docs' even though I used the wrong word, something there would be great.

@catch: done

On second thought: the @deprecated PHPdoc tag format and the @trigger_error() format are supposed to use the template

@deprecated in %deprecation-version% and is removed from %removal-version%. %extra-info%.

Am I going to mess up the API docs if I do not follow that format? If so, then I guess the deprecation message in #3582125: Improve the deprecation message in getMigrationDependencies() should be more like this:

'Calling ' . __METHOD__ . ' with the $expand parameter is deprecated in drupal:11.0.0 and is removed from drupal:12.0.0. The parameter has no effect in drupal:12.0.0.

Edit: Actually, there is no @deprecated tag in #3582125, but maybe I should update the change I just made in the "How to deprecate" doc.