[policy no patch] Make it possible to mark plugins as deprecated

@EclipseGc shared some thoughts on this in #1932810-36: Add entity bundles condition plugin for entities with bundles

If we were to add a little logic to the default plugin manager, we could probably include a "deprecated = TRUE" in annotations and hide the plugin from being returned by getDefinitions() but still allow it to be instantiated. That would let us deprecate old plugin from the UI so that users aren't using it going forward and people can, over time unify on this plugin. We can then address anywhere core uses plugins for the 9.x migration when the time comes, or even add in extra logic to the ConditionManager that when "node_type" is requested, we instantiate "entity_bundle:node" instead. Something along these lines should be pretty doable.

The ability to hide deprecated plugins in listings but still allow them to be used would be good for both Views and Field UIs.

Could we trigger_error() from methods that aren't used in discovery?

Could we trigger_error() from methods that aren't used in discovery?

I was wondering for a second: Why are there just 4 test failures if the discovery is involved. It turns out, actually its stuff like the actual form which initializes the plugin. The actual code looks like this:

    foreach ($this->manager->getDefinitions() as $id => $definition) {
      if (is_subclass_of($definition['class'], '\Drupal\Core\Plugin\PluginFormInterface')) {
        $actions[$id] = $definition['label'];
      }
    }

Given that we could totally deprecate inside the constructor.

If we were to add a little logic to the default plugin manager, we could probably include a "deprecated = TRUE" in annotations and hide the plugin from being returned by getDefinitions() but still allow it to be instantiated. That would let us deprecate old plugin from the UI so that users aren't using it going forward and people can, over time unify on this plugin. We can then address anywhere core uses plugins for the 9.x migration when the time comes, or even add in extra logic to the ConditionManager that when "node_type" is requested, we instantiate "entity_bundle:node" instead. Something along these lines should be pretty doable.

The idea to hide deprecated plugins is a good idea! What about "simply" not add "deprecated = TRUE", but instead just "hidden = TRUE" and continue to use the constructor for the deprecation?

I agree with keeping hidden = TRUE and deprecations separate.
Deprecations in the constructor also seems like the best option.

Agreed that's a good plan. Would also let us mark an entire module deprecated (to be moved to contrib or similar) but we might still allow it to be visible on sites it's already installed on including its plugins.

@catch
Its special for action plugins, should we still treat all plugins different? It feels like every PHP class should better be deprecated via the constructor, as loading might happen at some point.

FWIW, I think it'd be better to check the plugin definition during createInstance() in the DefaultPluginFactory for whether the plugin denotes that it's deprecated. Then if it is, we can do this trigger error stuff you all are talking about AND we get uniform support for plugin deprecation across most of core with one file change.

Eclipse

For what it's worth, I think the issue title should include deprecate in web UI. Or something like that. We've been deprecating quite successfully plugins in migrate for some time and not had issues. Either that an updated issue summary. Because I do not understand the issue then.

@heddn
That is a good point. The fact that our traditional approach of deprecating any PHP class doesn't work is due to the action module doing something special. Given that I don't really like the approach in #11 / anything which is special to plugins.

I don't see how action.module is special here, conditions for e.g. block visibility have exactly the same problem, deprecating the code is one thing, but how do we tell the user which one he should use? See #1932810: Add entity bundles condition plugin for entities with bundles. My idea there was a combination of showing it with a (deprecated) suffix if it's in-use and not showing it at all when it's not currently enabled. But that is likely something that indeed each UI needs to do on its own.

About constructor vs factory, the problem with "one file change" is that there isn't one factory class.. There's DefaultFactory, ContainerFactory, ReflectionFactory (not sure if someone uses that, actually). What we could possibly do is put it in \Drupal\Component\Plugin\PluginBase::__construct() and check the plugin definition there, then we'd cover almost all plugins becaue they usually call the parent constructor.

Putting it into PluginBase::__construct() is probably going to be about the same degree of successful as DefaultPluginManager::createInstance().

DefaultPluginManager::createInstance() doesn't ACTUALLY exist, it delegates to PluginManagerBase::createInstance(). What's important to point out is that all the classes you just named off are FactoryInterface implementing classes, NOT PluginManagerInterface implementing classes. The difference means that the PluginManager will actually get first shot at the plugin and that we don't have to change runtime code for every plugin we want to deprecate. I understand that, for some reason, dawehner doesn't like this suggestion. I do not understand why that is. I would like to since I have no way to address his concerns w/o understanding them.

What I DO know is that DefaultPluginManager is used by the vast vast majority of plugin types, both core and contrib, and that keying off the definition should allow us to limit whether a plugin is delivered by getDefinitions() or not (we'll have to write some code, but that seems doable). That said, I don't see why we want to alter (and or introduce) a constructor to every plugin we want to deprecate. Please let me know why we want to do this. Thanks!

Eclipse

#2921810: Allow TimestampFormatter to show as a fully cacheable time difference with JS is also trying to deprecate a plugin. Adding as related.

#2921810-21: Allow TimestampFormatter to show as a fully cacheable time difference with JS shows that putting the deprecation error trigger in constructor doesn't help.

@EclipseGc we want to do this because we want to deprecate plugins. And tell custom / contrib modules that are using them that they need to change something. I think adding the @trigger_error() seems sensible and adding it to the plugin file somewhere makes sense and the discussions above show why the constructor is a good choice.

@claudiu.cristea re #17 yes it did help. It told you that core was still using the deprecated plugin. In order to deprecate something you need to remove all usages.

I also think keeping the UI hiding as a separate thing makes sense too. Many plugins, for example field type and a plethora of views ones, use no_ui as a way of excluding a plugin from the UI. We could add a trait that gives discovery the ability to check the __constructor or class docblock for the @deprecated annotation and then exclude from the UI if the annotation has a no_ui key. Or we could just mandate that if you deprecate a plugin and there is a UI that you add the no_ui to the definition and then there's no magic. If there isn't a no_ui key for that plugin type then it will need to be added.

+1 to deprecating plugins by adding trigger_error() to the constructor.

So the constructor deprecation attempt in #292181: Improved API documentation caused failures because several core views are still using the formatter, that's by design in this case, we just don't want to trigger deprecations during discovery.

Adding the deprecation to the constructor, even if it means adding a constructor seems like the best option to me, it should be the last change to the plugin anyway in most cases.

So given that @catch and I agree and plugin maintainer @tim.plunkett also agrees with constructor deprecation. I think we are good to go here but a +1 from the other plugin maintainer @EclipseGc would be also good.

Here is the suggested update to https://www.drupal.org/core/deprecation

Plugins

Add @trigger_error('...', E_USER_DEPRECATED) to the plugin constructor. If the plugin does not have a constructor, add one. Add no_ui = true to the plugin definition. If the plugin does not support no_ui but is selectable in a UI then an issue will be needed to add it. Also, add an @deprecated phpdoc annotation to the plugin's class docblock. For example:

<?php

namespace Drupal\Core\Field\Plugin\Field\FieldFormatter;

/**
 * Plugin implementation of the 'timestamp' formatter as time ago.
 *
 * @deprecated in Drupal 8.5.0, will be removed before Drupal 9.0.0. Use the
 *   \Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampFormatter formatter
 *   instead and configure it with "Display as 'time ago'" option.
 *
 * @FieldFormatter(
 *   id = "timestamp_ago",
 *   label = @Translation("Time ago (deprecated)"),
 *   field_types = {
 *     "timestamp",
 *     "created",
 *     "changed",
 *   },
 *   no_ui = true
 * )
 */
class TimestampAgoFormatter extends FormatterBase implements ContainerFactoryPluginInterface {

  /**
   * Constructs a TimestampAgoFormatter object.
   * ... rest of the docs ...
   */
  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, DateFormatterInterface $date_formatter, Request $request) {
    @trigger_error('\Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampAgoFormatter is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampFormatter instead. See https://www.drupal.org/node/2926275.', E_USER_DEPRECATED);
    // ... constructor code...
  }

The no_ui stuff is a bit tricky.

As mentioned in the block example, that flag must only be considered when creating something new with a plugin and/or there is no existing configuration because the current block UI has no concept of "adding" conditions, it simply lists them all and they might or might not have configuration already.

We should probably document the expected behavior for that clearly and I'm not sure if the existing no_ui implementions really cover that. I did a quick test by adding no_ui = TRUE to ImageItem and it still works for the existing fields and I can't add new ones, so basically it works as expected.

But I'm not 100% sure if that will be the same for other plugin types and if we should really re-use that flag for deprecations or instead introduce an explicit new property for this. But I guess we can also document that this needs to be decided per plugin type?

@Berdir good point. I agree that a case-by-case basis is going to be best for the no_ui stuff. Tricky.

Given @catch, @tim.plunkett and I agree that adding the @trigger_error() to the constructor is the best way forward I'm going to rtbc this. I think here we are agreeing a policy. There is no code change per-say that is necessary.

I agree with @Berdir that we should be careful with the no_ui part of the suggestion and use it only where it works and add good tests if we use.

Yes +1, let's add the text from #21 to the deprecation policy then mark this fixed, we can revise as more comes up but this lets us actually do the deprecations elsewhere and doesn't require anything new to support.

Ok, it took a little bit to wrap my head around the approach here so let me start by saying I'm generally on-board and won't push back, but I don't like it. The rest of this post is my "why".

I don't like this because:

1. The solution is designed exclusively for 1 edge case, manual instantiation of plugins. Frankly, no one should be doing this, and anyone who is is using the plugin system incorrectly.
2. Adding code to a constructor isn't introspectable. We couldn't (for instance) introduce any sort of filtering mechanism that hides deprecated plugins from core's UI's.
3. It also cannot be easily nuanced for situations where a plugin deriver might be involved. If, for instance, we have a plugin that depends on other plugins via a deriver and we deprecate one of the dependencies (think blocks wrapping fields and we deprecate a field) passing that deprecation knowledge along upstream is essentially impossible. YES the trigger warning will still eventually happen when the wrapping plugin invokes its dependency, but there's a whole layer of abstraction the developer will have to dig through to figure out how and why that warning is being triggered.

I realize that the idea of doing a no_ui in the plugin definitions is being considered a "separate thing" and I fully support that thing happening, however an @Deprecated('new thing', 'version', 'etc') annotation is introspectable, could contain the same sort of data we've established that we want here, and is ultimately a lot more robust. No, it doesn't cover the edge case of people manually instantiating plugins... but again I can't say that's considered a supported use of the plugin system and it should really only happen in tests if at all.

Eclipse

The solution is designed exclusively for 1 edge case, manual instantiation of plugins.

Unless the plugin consists entirely of static methods, that's not true. All plugins are eventually instantiated under normal circumstances, either directly or via the static create() method. Am I missing something there?

the developer shouldn't ever do new SomePluginClass(); they should be calling the plugin type's manager and invoking createInstance($plugin_id, $configuration); So there should always be a factory involved in the process.

Eclipse

the developer shouldn't ever do new SomePluginClass(); they should be calling the plugin type's manager and invoking createInstance($plugin_id, $configuration)

Yes, but the factory will eventually do new Plugin(). Or the create() static factory method will. The constructor is guaranteed to run at some point. The plugin cannot be created any other way. Even doing it with reflection will call the constructor. The only way the constructor could be bypassed, as far as I know, is with PHPUnit's getMockBuilder()->disableOriginalConstructor() -- but that, as far as this issue is concerned, is an edge case.

Let's back up. What I'm saying is that the solution outlined in this post is designed to work EVEN WHEN a developer manually calls the plugin's constructor. Manually calling the constructor via something like:

$plugin = new PluginClass();

This behavior by a developer is NOT SUPPORTED by the plugin system. The plugin system instead expects:

$plugin = $manager->createInstance($plugin_id, $configuration);

Yes, of course that will ultimately call the plugin's constructor. I'm not debating that. I'm saying the plugin system doesn't support developers manually calling the plugin's constructor, so designing for that misuse of the classes seems counterproductive to me given that the plugin system is essentially robust tooling for informing Drupal of aspects of these classes WITHOUT instantiating them. That's kind of the whole point of the plugin system, so deprecating plugins in this way, when it will do nothing to prevent the use of deprecated plugins (by say, hiding them in the UI) seems inefficient at the very least.

That being said, I'll expand on one other topic here. The use of "no_ui" in the plugin definition is, as I've previously said, something I completely support. That being said, I would posit that "no_ui" does NOT mean "deprecated". The inverse of this however may be true. Odds are "deprecated" always means "no_ui". All scotch is whiskey. All whiskey is NOT scotch.

Eclipse

One case where having it in the constructor over having it in the plugin definition/factory is possibly an advantage is when you have subclasses, if someone subclassed a plugin that is deprecated, then their plugin definition would not automatically be deprecated as well. But calling it will still trigger the deprecation message, which is kind of correct, because at the very least, that subclass will need to be updated to no longer depend on that deprecated class.

That said, I agree that also having it explicitly in the plugin definition could be useful, in a more explicit way than a no_ui flag. As explained in #22, the handling of deprecated plugins in the UI is not trivial. A plugin that is still being used and configured still needs to show up. You're just not allowed to add new ones and it will go away once all configuration is removed. But as I wrote, actually implementing that is most likely specific for each UI. But one thing that an explicit deprecated flag would allow us to do for example in the block condition/visibility UI is adding a (deprecated) flag behind them, maybe even strike them through or something like that.

Yup, or hide them completely from being placed by the UIs but still let them be instantiated. That would let them continue to work, but prevent site builders from placing new ones. It could also give us a way to highlight pre-existing instances to which they need to give attention.

Eclipse

What I'm saying is that the solution outlined in this post is designed to work EVEN WHEN a developer manually calls the plugin's constructor.

Just to say, this is a byproduct of the solution, it's not the reason the solution was chosen. The reason it was chosen was to trigger the deprecation notice only when the plugin is actually used and not when it's listed in UIs (which as has been discussed may be necessary to allow people to change the plugin used from the UI, but on a case-by-case basis).

however an @Deprecated('new thing', 'version', 'etc') annotation is introspectable

The proposed solution here includes adding @deprecated on the class, which is already introspectable. It doesn't preclude us adding an additional annotation key later if we decide to do that but the possibility is there to introspect @deprecated already.

The main thing for me here is that the solution here is closest to how we deprecated 'regular' classes/methods/functions - @deprecated and @trigger_error() in the thing being deprecated. There are cases where we'll have to add additional code to manage the deprecation process #2866779: Add a way to trigger_error() for deprecated hooks is the main example, but for me better to do that only when it's something we can't handle with @deprecated and @trigger_error() directly.

+1 to #35 if we can stop deprecated plugins from being used in the UI that's great but the real purpose of deprecation is to allow developers / testing infra to know when deprecated code is being used.

Yes, I understand that and am on board with it. That being said, with the current approach, how do you intend on dealing with things like derivatives?

Also, just a clarification, I was never suggesting that deprecation warnings should get thrown during a listing operation. The intent was always to do that during the factory's instantiation procedures. I think that was clear, but in case it was not, I thought I'd make it more so.

Eclipse

Looks like there is more discussion to be had here?

Although the docs have already been updated...so maybe this should be marked fixed with a followup for the remaining concerns?

In #1932810: Add entity bundles condition plugin for entities with bundles, we now have this snippet:

+++ b/core/modules/block/src/BlockForm.php
@@ -249,6 +249,15 @@ protected function buildVisibilityInterface(array $form, FormStateInterface $for
+
+      // 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.
+      if ($condition_id == 'node_type' && !isset($visibility[$condition_id])) {
+        continue;
+      }
+

The goal is that BlockForm could apply the same logic to any condition plugin that declares itself as deprecated.

Right now, with the definition we have, that's not really possibly or only in a very complex way as I'd nee to parse the dockblock myself of the plugin class myself. Which might actually not work if for example ctools would alter the class of that plugin to something else (That is actually exactly what is happening right now)

IMHO, there are two options:

a) Our plugin discovery would automatically somehow detect the @deprecated and inline it into the plugin definition
b) We duplicate the @deprecated annotation into @Plugin and add that as an official thing to \Drupal\Component\Annotation\AnnotationBase

> add a "deprecated" key into the plugin annotation and call trigger_error during Drupal\Component\Plugin\PluginManagerBase::createInstance()

That only covers annotated class plugins.

Should we discuss YAML plugins here too, or fix the scope of this issue and file a new one for YAML plugins?

This became blocker for #3097204: Remove deprecated action plugins from 9.0.x

Btw "no_ui" also may work #3094722: Add "no_ui = true" to the definition of deprecated action plugins

Seeing this is a policy, no patch issue... we should try to set policy that applies to *all* types of plugins. Or re-title this to only code based or UI based or something else based plugins. This is a blocker though for #3039240: Create a way to declare a plugin as deprecated, where we need to find a solution to yml based plugins.

It turns out there's a policy for deprecating plugins here: https://www.drupal.org/core/deprecation#how-plugin

However, I don't see how it can be used for YAML plugins:

> Add @trigger_error('...', E_USER_DEPRECATED) to the plugin constructor.

YAML plugins *can* define a specific class, but generally they tend to all use a common class. Seems a bit faffy to have to create a new class just to deprecate a plugin.

> Add no_ui = true to the plugin definition. If the plugin does not support no_ui but is selectable in a UI then an issue will be needed to add it.

That's doable at least.

> Also, add an @deprecated and @see phpdoc annotations to the plugin's class docblock.

We can do the same sort of thing to the YAML plugin definition, with a comment or something. But it needs defining.

This is blocking #3010983: Deprecate Drupal 6 and Drupal 7 migrations and move to contrib

at least it major blocker for many subsystems

It could use new attribute for plugin classes https://wiki.php.net/rfc/deprecated_attribute

The new native #[\Deprecated] attribute in PHP 8.4 can automatically throw deprecation messages, so external libraries can give unexpected deprecations

I've read through this again and I agree with @larowlan, that this can be closed and discussion can continue in any followups. For example, there is already #3039240: Create a way to declare a plugin as deprecated. And in that issue the documentation can be updated when there is a working implementation.

I am going to set RTBC. I trust if I missed something that should be in a follow, someone will comment.

Agree there is nothing left to discuss here, we already have docs on how to deprecate PHP plugins. I agree with #47 that we don't have a good way to deprecate YAML plugins, but that's not a problem for a policy issue, that needs someone to open another issue with a proposed implementation.

Closing as fixed and granting credits.