Create a way to declare a plugin as deprecated

What about just adding a

deprecated: 1

or something along those lines?

All migration yamls can be backed by a class. We could add such a class that has either in its constructor or somewhere a trigger_error, etc.

Discussed this in migrate meeting. Only want to trigger this on use, not necessarily on discovery. Perhaps add an interface with a method for the message. Then leave it up to the plugin managers or executors to figure out how/when to trigger the error. All of these would require adding a class item to the yml file.

Another question is when we move plugins or migrations to other module - how to keep traces and "reparent"

#3064017: Create a means to mark an asset library as deprecated in a *.libraries.yml file should provide some prior art.

Reference policy issue

It appears this is blocking #3096676: system_maintenance migrations uses incorrect maintenance mode variable in Drupal 7 migrations.

@quietone, @jibran, @xjm and I worked through this at DrupalSouth

Here's some WIP code as a discussion point

+++ b/core/modules/migrate/src/DeprecatedMigrationInterface.php
@@ -0,0 +1,26 @@
+ * Defines an interface for a deprecated migration.
+ */
+interface DeprecatedMigrationInterface {

+++ b/core/modules/migrate/src/DeprecatedMigrationTrait.php
@@ -0,0 +1,38 @@
+ * Defines a trait for easily adding a deprecation.
+ */
+trait DeprecatedMigrationTrait {

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -115,6 +115,14 @@ public function __construct(MigrationInterface $migration, MigrateMessageInterfa
+    if ($this instanceof DeprecatedMigrationInterface) {

Maybe make this interface broader and allow use it for other plugins/managers?
It could be useful instead of #3094722: Add "no_ui = true" to the definition of deprecated action plugins

+++ b/core/modules/migrate/src/DeprecatedMigrationInterface.php
@@ -0,0 +1,26 @@
+interface DeprecatedMigrationInterface {
+
+  /**
+   * Checks if a migration is deprecated.
+   *
+   * @return bool
+   *   Returns TRUE if a migration is deprecated
+   */
...
+  public function getDeprecationMessage();

+++ b/core/modules/migrate/src/DeprecatedMigrationTrait.php
@@ -0,0 +1,38 @@
+   */
...
+  protected $deprecationMessage = '';
...
+  }
...
+  }

In Java, there's a Serializable interface. This is simply a marker interface that has no methods. In the case of this interface, we need to getDeprecationMessage method. But similar to Java, we don't really need this method, because any implementations of this interface are by definition deprecated...

Doing that would obviate the need for the Trait. So less is more :)

OK, changing my opinion after reviewing the rest of this patch. Instead of adding an interface and trait, I highly suggest we put this on the base MigrationInterface (or even PluginInspectionInterface with implementation in Drupal\Component\Plugin\PluginBase). It is all about providing metadata about the plugin. And PluginInspectionInterface has as its docblock:

Plugin interface for providing some metadata inspection

How much does this alternative break core? Let's see just for kicks and giggles. BTW, I think we are allowed to do this with BC from my reading of https://www.drupal.org/core/d8-bc-policy#interfaces

Whoa, that's a lot of failures. Let's see if this results in fewer.

So the PoC passed green. What does that tell us?

+++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
@@ -40,6 +40,20 @@ abstract class PluginBase implements PluginInspectionInterface, DerivativeInspec
+  protected $deprecationMessage = '';

@@ -107,4 +121,18 @@ public function isConfigurable() {
+    return $this->isDeprecated;

Presence of message is a signal to deprecation (plugin annotation property) so isEmpty($this->deprecationMessage) should be enough

I had the same thought as #21.

And we might need the trait, but its for 2 places in all of core. I feel like we're probably OK without it. And we need to update the comments. But first, we need a comment from the plugin subsystem maintainers if we're going to go this route.

Rather then just waiting more weeks here, does anyone else have opinions about #12 vs #19?

I think I like the approach in #19 but can we add a example of deprecating a migration, like system_maintenance, as in #12?

The real need for that is #5
#3067299: Move actions migrations and tests to system module and #3022401: Remove useless config action.settings.recursion_limit
Which could be used as example

• #12 adds migration system-specific deprecation infrastructure.
• #19 generalizes this to work for any plugin.

Migrations are plugins. All plugins may eventually need to be deprecated. So I think #19 makes more sense. I doubt that core committers will accept a new one-off kind of deprecation infrastructure.

The only way I could see #12 being favored is if it also works for non-plugin-based migrations, because then the "consistent for all plugins" argument disappears. Is that even a thing? IOW, does it even make sense to deprecate migration config entities?

Removing the word "core" from the issue title as this is a general issue, not specific to core.

Clarified the title further.

As of #2746541-312: Migrate D6 and D7 node revision translations to D8.309.4, this is blocking #2746541. That issue is critical. Hence elevating this to critical too.

It is not possible to introduce new deprecated APIs in Drupal 8/9. For new deprecated APIs for removal in Drupal 10, they are currently on hold and should be filed against Drupal 9.1 (see https://groups.drupal.org/node/535670). So unless super-critical to deprecate something, we should not blocking any Drupal 9 blockers with deprecations, as that would be a paradox.

Posted at #2746541-318: Migrate D6 and D7 node revision translations to D8 too.

I'll move it over to the plugin subsystem.

I think this is no longer postponed.

reroll for 9.1

NW for adding tests and adding an implementation of this for action plugins.

I am updating the "Remaining tasks" in the issue summary based on @heddn's comment.

I'm keen to work on this but don't where to start on the tests. Can someone layout the work in some detail, what tests to go where?

On the other hand. should we wait for a subsystem maintainer review before spending time on the tests?

@quietone let's start by pinging any one of these folks in slack.

Plugin
- Kris Vanderwater 'EclipseGc' https://www.drupal.org/u/eclipseGc
- Alex Bronstein 'effulgentsia' https://www.drupal.org/u/effulgentsia
- Tim Plunkett 'tim.plunkett' https://www.drupal.org/u/tim.plunkett

Ok, so having read up on this, a couple of quick observations:

1. I would certainly prefer to see a trait in the mix here. We have identical implementations for the base class and TypedData. This indicates that other plugin systems in contrib are likely to run into this too. It'd be nice if they didn't have to copy/paste the code we find here into their plugins/base classes.
2. This doesn't appear to be exclusively yml based. In fact, aren't migration plugins using annotations? I think we can safely change the title to reflect that this is deprecations for the whole of the plugin system.
3. Any plugin that is deprecated should probably have a check in the constructor that trigger_error()s its corresponding message. It seems like the implementation for that could exist in the PluginBase class.
4. Do we want to consider expanding the PluginManagerInterface to provide a filtered discovery for undeprecated plugins? I feel like I'd like an answer to this, but if the answer is "yes" it likely expands this whole issue a lot.

Re #39
1: +1 on a trait. We did have that back in #12
2: Yes, this should be for all types of deprecation. However, the specific plugins in migrate we are trying to deprecate are yaml based.
3: +1, We had that back in #12 as well.
4: Maybe, but that could/should be a follow-up, no?

Just to re-iterate. Doing it via discovery is not what we want. Because we want deprecated migrate plugins to be discovered. But only trigger_error on instance creation and execution.

Re: 4

Yes, I think that would be a follow up. I just wanted to have the discussion and see how it impacts on ongoing use. It's important that we don't affect mainline discovery since instantiation leverages that same process, so you need to apply a filtering process for it. We actually already have a filtering mechanism in place for plugins leveraging contexts. I wonder if that system is generic enough to use here. As a general rule, leave Discovery alone. Messing with it tends to cascade into other problems.

As far as the trigger_error stuff goes, we should probably put that into a protected method on the trait and call that method in the constructor. Let's just make sure getting this functionality is as easy as possible for other Plugins in contrib that may not have used our base classes.

Eclipse

So, something more along the lines of the patch I've provided here.

However, after working with the code, there are a couple things more worth pointing out here.

1. The plugin itself isn't deprecated. That information is part of the metadata stored in the annotation/yml/whatever. That means that our current implementation (even mine here) is wrong. We should be consulting the definition for deprecation and corresponding message.
2. Likewise, TypedData's implementation is even more complicated. I don't think the previous patches nor my own here will hold up for that implementation
3. Thinking further ahead, since this needs to be part of plugin definitions, we have an additional problem which is that plugin definitions come in array & object flavors. We'll need to provide both a plugin definition class implementation and a convention to follow for arrays to satisfy both situations.

Don't take my patch here TOO seriously for all the reasons mentioned above. These just came to me after I'd worked through the code a little. I do, however, want to see if it passes.

Eclipse

NW for docs and tests

+++ b/core/lib/Drupal/Component/Plugin/PluginInspectionTrait.php
@@ -0,0 +1,75 @@
+    if ($this->isDeprecated()) {
+      @trigger_error($this->getDeprecationMessage(), E_USER_DEPRECATED);

+++ b/core/lib/Drupal/Core/TypedData/TypedData.php
@@ -40,6 +42,20 @@
+  protected $isDeprecated = FALSE;
...
+  protected $deprecationMessage = '';

Having empty deprecation message could be confusing
maybe isDeprecated could be removed in favor of !empty($this->deprecationMessage)

Hi @andypost
Made changes as you suggested please review.

+++ b/core/lib/Drupal/Component/Plugin/PluginInspectionTrait.php
@@ -0,0 +1,75 @@
+  public function isDeprecated() {
+    return $this->isDeprecated;
+  }
...
+    if (!empty($this->deprecationMessage)) {

If we're going to replace the internal usage of the isDeprecated property, should you just replace it in the method as well? Seems like it would lead to the possibility of a weird state where something is "deprecated" in the flag but also not triggering and deprecation messages because the message is empty. Then we can just remove the property entirely.

@neclimdul This is precisely my point. The properties don't seem useful to me. Adding this data to the definition does seem useful. We can introduce handling into the plugin manager(s) (or base classes) to identify this and trigger_error() as appropriate.

So I grabbed EclipseGC on slack and after meandering around on a bunch of ideas and a few tangents we came back around to liking the InspectionInterface currently in the patch. It makes a lot of sense for a lot of use cases.
The implementation has lots of things to figure out though. Most crucially we agreed it needs to be more usable out of the box.

1. The PluginInspectionTrait should probably just use definitions for figuring out deprecation instead of properties. This means plugins can just start adding @deprecated = TRUE or deprecate: true and this all starts working and each type doesn't have to create their own implementations to make things work. A key benefit to pushing this down to the definition is, as Eclipse has been trying to convince us, the plugin manager and other parts of the plugin system can act on it _without_ creating instances of the plugin which is clearly ideal. You could maybe ask your discovery for a list of deprecated plugins, or a list of not deprecated plugins, or upgrade tools could poll plugin managers and provide reports based only on definitions. This doesn't force the implementations but allows for them.
2. As noted in #42.3, plugin definitions aren't one thing, there are arrays and there are classes. Supporting that is unclear mostly because the current class interfaces don't provide us enough information yet. Maybe there's a new definition interface you have to implement to make this work for classes? Its an open question.
3. Additionally we need to decide if this is one annotation/definition property or two. Should we just have "deprecationMessage" and compute "isDeprecated" based on that or should we have both and some sort of rules for edge cases?

Note: I understand why migrations don't want to trigger on creation as noted in with #3. migrate often needs to instantiate plugins to do things like run a requirements check and triggering deprecation outside actual use would lead to problems and confusion. I think this is actually still inline because you can replace the triggers logic as its implemented now we just need to be mindful of this around any trigger logic.

It isn't clear what the next steps should be. Are we still in a deciding phase or just need to re-roll the latest patch with some tweaks?

@heddd, I'm not sure either but I decided to make changes for #48 in the hopes of moving this along.

#48.1 Removed the properties and used a definition key of, 'deprecationMessage' to determine if the plugin is deprecated. Is that what was intended? If so, where does 'deprecationMessage' get documented?
#48.3 Yes, compute isDeprecated. Keep it simple, the extra clutter in the plugin isn't needed.
For myself, I prefer this
deprecation_message: 'Deprecated in 9.2.x ...'
to this

deprecated: true
deprecation_message: 'Deprecated in 9.2.x ...'

in a migration yml.
Which brings up a final point is it 'deprecationMessage' or 'deprecation_message' or both?

Let try that again, checking if the plugindefinition is an array.

+++ b/core/lib/Drupal/Component/Plugin/PluginInspectionTrait.php
@@ -39,14 +39,17 @@
+    if (is_array($this->pluginDefinition)) {
+      return $this->pluginDefinition['deprecationMessage'] ?? NULL;
+    }

This raises an interesting point: what about instances of PluginDefinitionInterface? (aka EntityType, Layout, SectionStorage)

We have some duplication here: #2922451: [policy no patch] Make it possible to mark plugins as deprecated

#54: @quietone already linked that in #9 😊

And now something for #53.

Looks good! I spotted a docs error:

+++ b/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php
@@ -29,4 +29,20 @@ public function getPluginId();
+   * Checks if a migration is deprecated.
...
+   *   Returns TRUE if a migration is deprecated

This says migration in the base plugin system.

Changed the test a bit and the comments for #59.

Tried to bring the IS up to date.

Lets see what this looks like when deprecating a migration plugin. I added #3096676: system_maintenance migrations uses incorrect maintenance mode variable in Drupal 7 migrations which deprecates system_maintenance.yml, I used that one because the patch is smaller than the one for the action plugin, #3067299: Move actions migrations and tests to system module.

The changes here are:

• trigger error on plugin creation (MigrationPluginManager::createInstances) and plugin use (MigrateExecutable constructor) as per #40. Not sure we want to trigger error on creation because then some tests will be triggering the error.
• Tests for the above two additions
• Added a filter in MigrationPluginManager that just filters out the deprecated plugins. Then used that in MigrationConfigurationTrait::getMigrations so the deprecated plugin is not executed from /upgrade. No tests on that. And the existing func

I don't know how to prevent the tests from complaining about the deprecated plugin that is created in the tests. Is this the right approach for migrate anyway?

I just rewrote #3096676: system_maintenance migrations uses incorrect maintenance mode variable in Drupal 7 migrations so deprecating migrations can be avoided so should change to using the action plugin issue as an example. In the meantime it would be good to get some feedback on this.

Changing to NR to get feedback on the approach here, particularly for deprecating migration ymls.

For #62.1: which tests are triggering on creation? Is it tests or is it something else like "find all the plugins" (discovery), then go out and create them (creation). If it is this, when we would need some way to filter out deprecated plugins that are not in use before creation. How do we know if a plugin is in use? For migrate in core, we would always want to filter out deprecated, because core shouldn't use them. The tricky part for that statement is running incremental migrations. In that case, I'd argue we still want to filter out deprecated plugins. But someone else might argue the opposite. And if we are always filtering out during discovery, we aren't really "deprecating".

For contrib where migrations are "cloned", we would want to trigger error if we create any cloned/deprecated migration. But we wouldn't really have a "deprecated" migration if it is yaml. As these are place in time copies and no one would copy over a deprecated yaml. That would be crazy.

So, I'm back to, we need to find some step in the process for triggering error on creation. And still don't have any clue what should be the trigger 🤔 or even if we should filter.

Just a reroll.

Try again

Patch looks good!

1. +++ b/core/modules/migrate/tests/modules/migrate_deprecate_test/migrations/deprecated.yml
   @@ -0,0 +1,14 @@
   +deprecation_message: foo
   

   Could we change the message to something more descriptive. When you do something wrong and you get the deprecation message "foo", you have no idea were is comes from.

2. +++ b/core/lib/Drupal/Component/Plugin/PluginInspectionTrait.php
   @@ -0,0 +1,70 @@
   +    return ($this->getDeprecationMessage()) ? TRUE : FALSE;
   

   We can change the code to: return (bool) $this->getDeprecationMessage();.

3. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
   @@ -123,6 +123,7 @@ protected function getMigrations($database_state_key, $drupal_version) {
   +    $all_migrations = $this->getMigrationPluginManager()->filterMigrations($all_migrations);
   

   Do you get testbot failures, when you remove this change?

@daffie, good to know this is going in the right direction.

This patch has fixes for the test failures in #69. Although I don't see why the change to SubProcessTest is needed, not sure why the fix works.

#71
1. A more realistic message is now in the migration yml. I presume there needs to be discussion on the format of that message?
2. Fixed.
3. With the suggested change the SubProcessTest still fails.

Ah, I forgot I was filtering SubProcessTest and didn't realize other methods were failing.

Apply the same fix to testNotFoudnSubProcess as was done in testSubProcess.

Bad patch, Ignore #74.

+++ b/core/tests/Drupal/Tests/Component/Plugin/PluginBaseTest.php
@@ -103,4 +107,44 @@ public function testGetPluginDefinition() {
+    $msg = 'foo';
...
+    $msg = 'foo';

I think these need to be changed. My mistake, these are used with mocks and not the new deprecated test plugin.

The patch looks good to me and is RTBC.
We are creating new functionality in that plugins can now be deprecated, for that we need a CR.

@daffie, thanks.

I started the CR but feel like I don't know enough about what it should contain. It is rather brief. What else is needed (besides someone with better written English skills)?

Could we add testing for when the plugin is in a class and has the property 'deprecationMessage' with the deprecation message.

Yes, that should be done. Thanks for the reminder.

I wasn't sure what class to use but picked Layout from the list in #53. Not knowing anything about Layout this was an interesting journey and could be completely wrong. One good this is that it exposed an error in PluginInspectionTrait, where it was testing for PluginBase instead of PluginDefintionInterface. I found that during debugging and then found the same sort of logic in \Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass which is an encouraging sign.

Change getDeprecationMessage to not use get and adjust the deprecation test.

The CR looks good to me!

Though I wonder about also recommending that a deprecated class plugin add something in its docblock about being deprecated. It seems to me that the $deprecationMessage property is something that could be overlooked by a developer. That would have been one advantage of having a flag in the plugin annotation -- it's easier to spot.

I do not know how to fix this nor am I confident the test is correct. I found LayoutTest so used that, adding a net template with a deprecation message, but the property name is deprecationMessage which seems wrong in a yml file. And then when the plugin is created deprecationMessage is in an array 'additional'. Don't know why or where that is happening.

We discussed this issue on slack a couple of weeks ago. Here are some of my comments.

From the proposed resolution in the IS:

Indicate deprecation by the property 'deprecation_message' for a plugin defined by an array or 'deprecationMessage' for a plugin with an object definition.

Does "defined by an array" mean the same thing as defined by YAML?

If I read the test results correctly, then there are two problems:

1. The deprecation message in the test module's YAML file is not part of the actual deprecation message.
2. There are a bunch of other deprecation messages that get triggered.

I think this explains where the additional comes from: (core/lib/Drupal/Core/Layout/LayoutDefinition.php, Lines 179-187):

  public function set($property, $value) {
    if (property_exists($this, $property)) {
      $this->{$property} = $value;
    }
    else {
      $this->additional[$property] = $value;
    }
    return $this;
  }

Do you have any idea where the other deprecation messages come from? Have you tried removing the deprecation message from the test layout_deprecated to see what happens?

Well, since deprecateMessage is not a defined property we should test for it in additional. This changes getDeprecationMessage to do that. Also changed the property name in the yaml to be consistent with practice.

if ($this->pluginDefinition->deprecationMessage ?? NULL) the ?? NULL is not needed

Hey, I have tried to cover the points mentioned in #90
Please review

That changes causes an Undefined property: Drupal\KernelTests\Core\Plugin\Context\TestPluginDefinition::$deprecationMessage error so I'm ignoring the patch in #90.

@kishor_kolekar, it is always worth running tests, at least the ones that are changed in the patch, before uploading for the testbot to test. The Drupal wiki has instructions for setting up and running tests locally. It does use resources, including money, to run the tests so we try to do what we can locally first.

This patch moves the test from PluginBaseTest to a new file core/tests/Drupal/Tests/Component/Plugin/PluginInspectionTraitTest.php and some tweaks to the trait to handle the 'additional' property used by Layout Plugins. Although, I am not sure if that should be in the trait or in the LayoutPluginManager.

I forgot to run commit-code-check so aborted the tests and there was a one line naming error. Let's try again.

edit: s/abort/aborted/

@quietone:

You added the deprecated layout to an existing test module. That module is enabled by one of the functional tests, and that test ends up generating deprecation messages.

I think the simplest fix is to move the deprecated layout to a new test module. That fits with the general rule of not modifying existing tests unless you need to.

I am thinking the same thing but it was too late that day to act upon that. This makes a new test for testing a deprecate layout plugin.

Polishing new methods and docs a bit, also few clean-ups for review bellow

1. +++ b/core/lib/Drupal/Component/Plugin/PluginInspectionTrait.php
   @@ -0,0 +1,77 @@
   +    return (bool) $this->getDeprecationMessage();
   

   I think it should return TRUE only when message is string

2. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
   @@ -471,6 +471,23 @@ public function testProcessRowEmptyDestination() {
   +    $event_dispatcher = $this->createMock('Symfony\Component\EventDispatcher\EventDispatcherInterface');
   

   dispatcher should be used from contracts

Another attempt and fix

What action, if any, should be taken if the deprecation message is not a string?

I see no way except null when no deprecation message defined, that's why I used to try apply is_null()

A bit more clean-up

- revert to is_null() check because if plugin defines deprecationMessage public property as not a string but then ?string will throw on non string (otherwise trigger_error() will fail fatal)
- I see no reason to extend API of MigrationPluginManager as the only place where filtering used (comment could be improved)

It looks mostly ready, just not clear why $plugin->deprecationMessage public property needed (maybe for typed data)

+++ b/core/tests/Drupal/Tests/Component/Plugin/PluginInspectionTraitTest.php
@@ -0,0 +1,64 @@
+    $msg = 'foo';

Just a small nitpick: Typically, we only use full words for variable names (so $message rather than $msg).

It looks mostly ready, just not clear why $plugin->deprecationMessage public property needed (maybe for typed data)

Is this still in the patch?

Fixed #103 about naming, the property looks useless

+++ b/core/lib/Drupal/Component/Plugin/PluginInspectionTrait.php
@@ -0,0 +1,78 @@
+  public function getDeprecationMessage(): ?string {
...
+    if ($this->pluginDefinition instanceof PluginDefinitionInterface) {
+      if ($this->pluginDefinition->deprecationMessage ?? NULL) {
+        return $this->pluginDefinition->deprecationMessage;

+++ b/core/tests/Drupal/Tests/Component/Plugin/PluginInspectionTraitTest.php
@@ -0,0 +1,64 @@
+    $plugin_definition = $this->getMockBuilder(PluginDefinitionInterface::class)
+      ->getMock();
+    $plugin_definition->deprecationMessage = $msg;
+
+    $definition_with_additional = $this->getMockBuilder(PluginDefinitionInterface::class)

Yes, it is used only in getDeprecationMessage() and in test

1. +++ b/core/modules/migrate/tests/src/Unit/process/SubProcessTest.php
   @@ -46,11 +46,14 @@ public function testSubProcess($process_configuration, $source_values = []) {
   +    $migration->expects($this->at(2))
   
   @@ -140,12 +143,15 @@ public function testNotFoundSubProcess($process_configuration, $source_values =
   +    $migration->expects($this->at(2))
   

   ->at() is deprecated, so we should avoid adding new calls if possible - there's a fair bit of existing usage here so probably not worth changing in this issue unless someone feels inclined - see #3217717: Replace usages of the at() matcher, which is deprecated

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -102,6 +102,9 @@ class MigrateExecutable implements MigrateExecutableInterface {
   +    if ($this->migration->isDeprecated()) {
   +      @trigger_error($this->migration->getDeprecationMessage(), E_USER_DEPRECATED);
   +    }
   

   I'm wondering why we trigger in both the executable and the migration plugin manager.

   Is there a supported way to create a Migration without using the plugin manager?

106.1. Agree that, in this case, to add at() and remove in the #3217717: Replace usages of the at() matcher, which is deprecated
106.2. Yes, it is happening twice. Adding this to be discussed at the next migrate meeting.

Well, #3217717: Replace usages of the at() matcher, which is deprecated was committed, so re-rolling.

Patch looks good:

1. +++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
   @@ -54,13 +42,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +    $this->checkDeprecation();
   

   Can we replace this code with:

   if ($this->isDeprecated()) {
     @trigger_error($this->getDeprecationMessage(), E_USER_DEPRECATED);
   }
   

   The helper method checkDeprecation() can then be removed. It is only used here.

2. +++ b/core/lib/Drupal/Core/TypedData/TypedData.php
   @@ -16,6 +17,7 @@
   +  use PluginInspectionTrait;
   

   Why is the PluginInspectionTrait included here? I am probably missing the point and there is a very good reason why. Could you explain?

3. +++ b/core/modules/migrate/tests/modules/migrate_deprecate_test/migrations/deprecated.yml
   @@ -0,0 +1,14 @@
   +deprecation_message: core/modules/migrate/tests/modules/migrate_deprecate_test/migrations/deprecated.yml is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use core/modules/migrate/tests/modules/migrate_deprecate_test/migrations/new.yml instead. See https://www.drupal.org/node/3039240
   
   +++ b/core/modules/migrate/tests/src/Kernel/MigrationPluginManagerTest.php
   @@ -70,4 +70,15 @@ public function providerCreateInstanceByTag() {
   +    $this->expectDeprecation('core/modules/migrate/tests/modules/migrate_deprecate_test/migrations/deprecated.yml is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use core/modules/migrate/tests/modules/migrate_deprecate_test/migrations/new.yml instead. See https://www.drupal.org/node/3039240');
   

   We are now developing for D9.3 not D9.2.

4. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -102,6 +102,9 @@ class MigrateExecutable implements MigrateExecutableInterface {
   +      @trigger_error($this->migration->getDeprecationMessage(), E_USER_DEPRECATED);
   
   +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -113,6 +113,9 @@ public function createInstances($migration_id, array $configuration = []) {
   +        @trigger_error($migration->getDeprecationMessage(), E_USER_DEPRECATED);
   

   Could you tell me which deprecation message test belongs to each trigger_error()? I want to be sure that both have testing.

5. +++ b/core/modules/system/tests/modules/layout_deprecation_test/layout_deprecation_test.layouts.yml
   @@ -0,0 +1,4 @@
   +  deprecation_message: foo
   

   Can we change the deprecation message to something more realistic.

6. +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginInspectionTraitTest.php
   @@ -0,0 +1,64 @@
   +    $message = 'foo';
   

   Can we change this message to something more specific for this test. I just want to make sure that the message that we are testing is from this test class and not some other 'foo' message.

I turned the patch from #108 into a MR.

#109
1) However unlikely, a plugin could opt to implement PluginInspectionInterface and not extend from PluginBase, so I think it's appropriate to keep the method in the trait.

2) ...and this proves my point from above. TypedData implements PluginInspectionInterface but does not extend from PluginBase.

3) Fixed

4)
MigrateExecutableTest::testMigrationDeprecation() tests MigrateExecutable
MigrationPluginManagerTest::testMigrationDeprecation() tests MigrationPluginManager

5) Fixed

6) Fixed

I also restored the $pluginId and $pluginDefinition properties to PluginBase. They are not necessarily used by other code (like TypedData). To preserve the expectations of PluginInspectionTrait, I added abstract methods for each of the corresponding methods.

Untagging for subsystem maintainer review.

I think this needs one more test scenario: An actual annotated layout plugin in layout_deprecation_test that declares itself deprecated via the annotation.

Fixing credit

Also needs a case for a deprecated TypedData plugin, cause I think that's broken right now.

We also have the real-world use case of the NodeType condition plugin and we can convert the mechanisms around that (e.g. dynamically showing it only if it has existing configuration) to verify this.

Good call! Switched NodeType to using this mechanism. I guess there's more to see about whatever hiding mechanism you're referring to...

When I add a call to $this->checkDeprecation() to TypedData::__construct(), I get this error when trying to save a config entity (for example):

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "uuid" plugin does not exist. Valid plugin IDs for Drupal\Core\TypedData\TypedDataManager are: (filter_format, email, timestamp,.... SNIP)


core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:53
core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php:25
core/lib/Drupal/Core/TypedData/TypedData.php:84
core/lib/Drupal/Component/Plugin/PluginInspectionTrait.php:35
core/lib/Drupal/Component/Plugin/PluginInspectionTrait.php:57
core/lib/Drupal/Core/TypedData/TypedData.php:70
core/lib/Drupal/Core/TypedData/TypedData.php:49
core/lib/Drupal/Core/TypedData/TypedDataManager.php:91
core/lib/Drupal/Core/TypedData/TypedDataManager.php:103
core/lib/Drupal/Core/Config/Schema/ArrayElement.php:135
core/lib/Drupal/Core/Config/Schema/ArrayElement.php:38
core/lib/Drupal/Core/Config/Schema/ArrayElement.php:85
core/lib/Drupal/Core/Config/Schema/ArrayElement.php:59
core/lib/Drupal/Core/Config/StorableConfigBase.php:179
core/lib/Drupal/Core/Config/Config.php:212
core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:273
core/lib/Drupal/Core/Entity/EntityStorageBase.php:452
core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:252
core/lib/Drupal/Core/Entity/EntityBase.php:339
core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:592
core/modules/node/tests/src/Kernel/NodeConditionTest.php:24
vendor/phpunit/phpunit/src/Framework/TestResult.php:722

I found the other part of code @Berdir was referring to. Fixed that up.

Follow up made, #3252702: Add filter for un-deprecated plugins to PluginManagerInterface.

I am not sure what to do about the @todo mention Berdir. I'd prefer a follow up issue but is removing the @todo in scope here?

I've made the test for the layout plugin and it seems to working fine. Typed data is completely different and I've yet to see the light.

I am not able to push changes so will make a patch in a day or so.

Removing the 'needs tests' tag.

This does need a rebase but I still haven't figured out how to do that. The instructions do not work for me.

Tests are passing, setting to NR

Looks good, just one nitpick:

  public function testLayoutDeprecation() {

  public function testLayoutDeprecated() {

Weirdly named test methods, especially when one is for YAML and one for annotation plugins.

Would make more sense to call them testLayoutDeprecationYaml and testLayoutDeprecationAnnotation. (And then as a bonus, you can run both tests together with '--filter=testLayoutDeprecation')

I might be missing something, but it feels like the checkDeprecation method should be used more. This would mean it checks the message and print a deprecation error right away it seems. Why would we need the checks and trigger_error in the plugin themselves?

I might be missing something here, but this confuses me.

Also since the PR was updated to 9.4, this means the deprecation messages might need updating, since we are now targeting 9.4. This might also make removal in 10 a little harder, but i've heards of more deprecations that deprecate in 9.4 then removed in 10. Might need a release manager review though.

For the tests it doesn't matter, there is no BC for that, it's just for test purposes, but we should then probably actually use fake/placeholder numbers instead. And NodeType is already deprecated for 9.3, we're just moving that around, so I think that part is fine.

Ah that is a good point, that makes things less complicated then :)

It's a blocker for #3204511: Deprecate tracker migration

Updating this to D10.1. There were too many conflicts with a rebase so I applied the diff to 10.1.

1. +++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
   @@ -54,6 +56,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +    $this->checkDeprecation();
   

   So this works generically, for all plugin types 👍

2. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -90,6 +90,9 @@ public function createInstance($data_type, array $configuration = []) {
   +    if ($typed_data->isDeprecated()) {
   +      @trigger_error($typed_data->getDeprecationMessage(), E_USER_DEPRECATED);
   +    }
   

   … so then why is this necessary? 🤔 I know TypedData has extra complexity. But how do we convey clearly in the change record which plugin types need extra attention?

3. +++ b/core/modules/layout_discovery/tests/src/Kernel/LayoutDeprecationTest.php
   @@ -0,0 +1,42 @@
   +  public function testLayoutDeprecated() {
   +    $this->expectDeprecation('layout_deprecated is deprecated');
   +    $this->container->get('plugin.manager.core.layout')
   +      ->createInstance('layout_deprecated', []);
   +  }
   

   👍 This proves it works generically.

I've done a proper rebase and a new MR for 10.1, leaving the 9.4 MR for the benefit of people wanting to patch their sites.

Please don't do squash merges just because conflicts are hard to resolve -- it means we lose the history of the work done on this issue, which makes it harder to understand the changes and to revert any of them in further work on this issue. It also makes it harder for a future rebase, if this issue needs to be rebased for 10.2 and so on. Rebasing a branch with a string of small commits is always easier than rebasing a single big commit.

The correct way to do a rebase to a different core branch is with the '--onto' option, like this:

First, get the issue branch up to date with its existing core branch.

1. Determine the core branch that the feature branch is against. e.g. 9.4.x.
2. Switch to 9.4.x. Git pull to get it up to date.
3. Switch to 1234-my-issue.
4. git rebase 9.4.x. Resolve any conflicts.
5. git push -f

Now, we rebase onto the new core branch, e.g. 10.1.x:

1. git checkout --track origin/10.1.x or `git checkout 10.1.x`
2. git pull
3. Switch to issue branch: git checkout 1234-my-issue
4. Make a new issue branch. This is so that people can continue to use the old issue branch if they are using an older version of Drupal: git checkout -b 1234-10.1-my-issue
5. Now we take the new issue branch, snip it from 9.4.x, and stick it onto the tip of 10.1.x: git rebase --onto 10.1.x 9.4.x. Resolve any conflicts
6. Push the branch and make a new MR.

There were only two conflicts:

- a change to the NodeType condition which has since been removed from core
- a conflict in BlockForm due to the same

Setting to NW because this doesn't look right:

      // Don't display the deprecated node type condition unless it has existing
      // settings.
      if (isset($definition['deprecation_message']) && !isset($visibility[$condition_id])) {
        continue;
      }

Consumers of plugins shouldn't need to know specifics about which plugins are deprecated. All deprecated plugins should be handled correctly -- and that should probably happen at the plugin manager level rather than in the form class?

> Consumers of plugins shouldn't need to know specifics about which plugins are deprecated. All deprecated plugins should be handled correctly -- and that should probably happen at the plugin manager level rather than in the form class?

A deprecated plugin can still be used, and if and when it it is shown and how that message is presented is up to the specific implementation, that can't be generalized. As this example shows, we want to show a deprecated plugin only if it's already in use, so it depends on existing configuration, which is not something the plugin manager has knowledge of. At best the first part could be a method or something to make it a bit more readable.

As deprecated plugin can still be used, yes.
Why couldn't we still use PHP Annotations ?

> As this example shows, we want to show a deprecated plugin only if it's already in use, so it depends on existing configuration, which is not something the plugin manager has knowledge of.

Agreed, but making the code specific to the NodeType plugin is wrong here. What if a contrib module was providing the FooBar condition plugin, and then deprecated it?

That's just the restored comment from the old node type specific logic that used to be in that place in D9:

      // Don't display the deprecated node type condition unless it has existing
      // settings.
      // @todo Make this more generic in
      //   https://www.drupal.org/project/drupal/issues/2922451. Also remove
      //   the node_type specific logic below.
      if ($condition_id == 'node_type' && !isset($visibility[$condition_id])) {
        continue;
      }

that was dropped including the todo in D10. That's the leftover from merging/rebasing that.

the code is not actually specific about that, so we just need to fix the comment. That said, BlockForm is full of hardcoded customizations for specific core plugins that look different on block visibility conditions than elsewhere.

Consumers should know, cos something needs to apply "no_ui filtering" like #3094722: Add "no_ui = true" to the definition of deprecated action plugins

I think the changes to BlockForm are out of scope for this issue. That change is deciding what to do when a condition plugin is deprecated in the case of Blocks whereas the scope here is simply to create a way to declare a plugin as deprecated. But I could be wrong - I have been doing a lot of that lately.

edit: fix formatting of 'what'

It can go either way. For Drupal 9, since this dealt with the specific one-off deprecation of the NodeType condition plugin, updating BlockForm had to be included. In Drupal 10, that's gone, so we can leave it out. That does however mean that it won't be able to handle a deprecated condition plugin from a contrib module for example. But then again, the same can be said about all other UI's that list all possible plugins somewhere: The add block list for example if you deprecate a block plugin, or the actions module when deprecating an action plugin and so on. So to get this in, we probably need to identify a list of relevant places that all need to be updated and create follow-up issues for that. They might all need fake-deprecation in test modules to be able to test it, so that is going to be a bit of work.

needs work for version independenct message in test, otherwise looks ready

Probably it backportable to 9.4.x but current MR for 10.1.x and 9.5 could be needed for porting, ++ #142 and other "no ui" patches could go as follow-up

I have updated the CR.

What are the places that need followup?

I made changes for the latest unresolved issues. There is an MR for 9.5 (which does still have the changes for NodeType condition plugin) and 10 and both are passing tests. Back to NR

> What are the places that need followup?

Menu plugins will need not just a deprecation message, but a way of indicating the replacement plugin.

Use case: node module wants to rename some of the menu/task/action plugins it defines for nodes (see #2976861: add an entity Links handler, to provide menu links/tasks/actions for entity types for reasons). But contrib and custom code refers to the deprecated task plugins, for example as parent items.

So node module needs to be able to tell the menu system, 'Plugin X is deprecated, anything that refers to plugin X should actually get the replacement, plugin Y'.

Oops, edit conflict, didn't mean to change the status!

Skimmed.

But seems to be a failure in the MR 2821 for D10.

rebased, failure looks unrelated

This seems like a big change.

The 2 open threads seem to be resolved as @quietone changed to "some deprecation message"

The two remaining tasks

Identify a list of relevant places that all need to be updated and create follow-up issues for that.
Update deprecation policy

Have those been completed?

For the 2 remaining tasks if they can be identified.

We have some old layouts (in my_module.layouts.yml) that we want to deprecate and use a more general use case layout instead. It would be nice to make the IS more explicit that this will cover those yaml plugins as well.

The CR has an example of deprecating a YAML plugin.

But the CR leaves a lot unsaid and is confusing in other ways:

> Methods are added to check for the deprecation when a plugin is constructed, to determine if a plugin is deprecated and to get the deprecation messaged.

Whose responsibility is it to check for a plugin being deprecated? Will core's plugin managers take care of checking their plugins, or is it up to the code that gets a plugin instance?

Created new MR for meaningful 7 commits against 11.x branch by cherry-picking and fixed PHP 8.3 compatibility in test

ATM mostly migration plugins awaiting this deprecation
Also attributes conversion may need it #3265945: Deprecate plugins using annotations and plugin types not supporting attributes

RE #151

at least 2 blocked
- #3204511: Deprecate tracker migration
- #3209220: Deprecate and remove action settings migration

updates has policy issue #2922451: [policy no patch] Make it possible to mark plugins as deprecated

PS: probably makes sense to file follow-up to introduce #Deprecated attribute like https://wiki.php.net/rfc/deprecated_attribute but for classes

Attempt to use the current code for migration https://git.drupalcode.org/project/drupal/-/merge_requests/6075

Left some small comments.

Hid the old branches for clarity.

rebased and addressed feedback

Feedback appears to be addressed! Thanks!

This seems to only add support for annotations and it relies on undocumented, undefined annotation properties to do so.

We need to make sure this works on attributes as well, which in its current state, I believe it does not, because for example the just committed Mail attribute class in #3420977: Convert Mail plugin discovery to attributes has a fixed, hardcoded set of properties defined in its constructor and it is not possible to just pass in additional stuff the way this expects it to.

I think we need to do either or both of:

a) Standardize on using ...$var to pass all extra constructor arguments through to the parent. See for example ConfigEntityBase in #3396166: Convert entity type discovery to PHP attributes, so that the attribute base class can introduce and define the deprecated_message property and it works an all child classes too.
b) Support an explicit additional key, which is considered here in the trait in the parent class, so that extra stuff can be put there. We discussed that a bit in the original issue but postponed its introduction on having use cases for it, more or less defining that it would be done for specific plugin types that are likely to be extended.

I agree with #166. While PluginDefinition declares AllowDynamicProperties we should not be relying on that for deprecations and should be introducing specific properties, and I think AttributeBase and AnnotationBase also need properties and setters adding to match.

To me option A from #166 seems simpler, as it means we have to explicitly declare all our properties and we will not be tempted just to dump extra keys into the "additional" property unless absolutely necessary (e.g. in the case of entity types, at least to start with).

I also tried prototyping a separate attribute, e.g.

#[Mail(
  id: 'php_mail',
  label: new TranslatableMarkup('Default PHP Mailer'),
  description: new TranslatableMarkup("Sends the message as plain text, using PHP's native mail() function."),
)]
#[Deprecated('php_mail is deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. Use the symfony_mailer plugin instead.')]

but this doesn't really seem to gain us much - static analysis could find all deprecated things more easily if they all shared one attribute, but it complicates discovery as we have to check the second attribute and doesn't map well to YAML plugin definitions.

Is this going to be extensible by specific plugin types?

For example, in the menu system, we need to be able to not just say plugin 'foo' is deprecated, but to use plugin 'bar' *instead*.

@joachim in a way that needs to be understood by code and handled automatically, or only in a message to be read by humans?

> In a way that needs to be understood by code and handled automatically, or only in a message to be read by humans?

By code.

Suppose node module defines a menu link 'foo'. It wants to replace that with 'bar'.

Contrib and custom code declare their own menu link items and say 'parent: foo' because they want to place their pages within Node module's admin UI.

The menu system needs to be able to go 'Oh, I'm going to use 'bar' as a parent for that item instead because 'foo' is deprecated'.

The use case is #2976861: add an entity Links handler, to provide menu links/tasks/actions for entity types, where we need to deprecate hardcoded menu links/tasks/actions and replace then with automatically-created ones.

In services this can be done with aliases; you create the new service and alias the old service name to it. Aliases can be deprecated too. Do we need this concept in plugins too?

A specific plugin system can always define additional keys on top of this, like a replaced_by or something. For most other plugins that's likely not needed or more complex, as plugins usually have configuration and the configuration of the replacement might not apply 1:1 and so on. So I'm not sure if that we need to build this into the base functionality.

The more I think about it the more I wonder what is a plugin manager, if not much more than a service container/locator that only contains a subset of services that conform to a specific interface?

@longwave: Plugins aren't (single-instance) services. Every node you load is a plugin object, so are fields and blocks and so on.

> A specific plugin system can always define additional keys on top of this, like a replaced_by or something. For most other plugins that's likely not needed or more complex

Agreed - this is not going to be needed for most plugin types. I just wanted to make sure it'll be possible to add that on top of what's being done here. This issue is the last blocker to the entity links handler issue :)

Maybe better to address Attribute and "attribution-discovery" deprecations in follow-up as ATM core needs deprecation for annotation discovery

Additionally to #167 there's PHP RFC for the #[Deprecated] attribute which has stuck
- https://github.com/php/php-src/pull/11293
- https://wiki.php.net/rfc/deprecated_attribute

But PhpStorm has implementation which has growing adoption
- https://packagist.org/packages/jetbrains/phpstorm-attributes/stats
- https://github.com/JetBrains/phpstorm-attributes