Deprecate plugins using annotations and plugin types not supporting attributes

Question: Do we need a change record if there is already one for the original ticket?

Personally I think the two existing change records (one for plugins, one for plugin types/managers) are enough, and we can update them as we convert more plugin types - perhaps we need a table to show which types were converted in which core versions.

Personally I think the two existing change records (one for plugins, one for plugin types/managers) are enough, and we can update them as we convert more plugin types - perhaps we need a table to show which types were converted in which core versions.

Great idea! I expanded the existing table in https://www.drupal.org/node/3395575 with a new column.

Will this also deprecate the annotations Discovery itself or will we do that in a separate issue? We'll only be able to do that once all core types are done while this should probably be done asap

The use for the unused class needs to be removed so that tests start to run.

For the message, I think it should be Using instead of Use. And since annotations didn't require namespaces, is it worth cutting that off in the message and prefixing with a @ maybe? So the test example would be "@CustomPluginAnnotation" That might be a bit more intuitive when searching for it.

> - when all core plugins are done, trigger deprecation for plugin managers that use annotations instead of attributes

I've created #3396165: [meta] Convert all core plugin types to attribute discovery and I think we should handle that part there.

We can get this in as soon as all block and action plugins are converted, and I think it's mostly just that weird xss thing (plus layout builder, depending on how we approach that), while the that part needs to wait until core is fully converted.

And getting this in asap *will* be useful to make sure we catch all plugins then for each plugin type.

Rebased this now that the main issue is in. Was quite messy, don't bother looking at the older commits, I've kept them but they might not make much sense.

At this point, we have two known blockers:

* the block_xss_title test block plugin, which is pretty bogus, as explained in the parent issue. I would suggest to just dropping that and use any random block instaed, there are no test for the block *plugin* title, just the configurable title. I suspect that was ported over in some form from D7 tests.
* The layout_builder InlineBlock with its dependency issue: #3255804: Hidden dependency on block_content in layout_builder.

Additionally, this MR added unit test coverage for the deprecation. However, we can IMHO now use the \Drupal\KernelTests\Core\Plugin\DefaultPluginManagerTest for that, I already added the legacy group and expected deprecation assertions to that. Optionally, we can split it into different test classes/methods to make it clearer that only some variations of that are legacy/cause deprecations. Then we can drop those unit tests again. But I didn't want to remove the existing work here without discussion.

Needs review to run DrupalCI. Dealing with a large amount of deprecations on GitlabCI is _not_ fun (yet).

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

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

The parent is closed so reparenting to the closely related meta issue.

I haven't had time to read this over. But as someone who will be receiving support requests about this, has it been evaluated how this can be discovered with PHPStan if implementing a deprecation?

I've been considering how we can have phpstan-drupal detect plugins using deprecated annotations for plugin managers that support attributes.

Thought one. Add @deprecated to the annotation class. When phpstan-drupal encounters a class that is a superof \Drupal\Core\Plugin\PluginBase, parse its document comment. phpstan-drupal can then iterate through its known namespaces to find the annotation (if it doesn't have a namespace.) It's like an inefficient SimpleAnnotationReader. For the most part, the pattern is \Drupal\{module}\Annotation. We can provide some configuration for unique situations, such as blocks (\Drupal\Core\Block\Annotation\Block) or contrib, which don't follow this pattern. Drupal core can then keep its runtime trigger proposed in this MR. The weird part is that it could be confused for deprecating the plugin type itself. But we don't have a pattern for that in Drupal core, so maybe it's not a big issue.

Thought two. PHPStan has a way to perform analysis at the end of the scan; it's how it checks for unused traits. Use this approach to collect all plugins with annotations and plugin managers which call the parent constructor with an attribute, like BlockManager does with Block::class

    parent::__construct('Plugin/Block', $namespaces, $module_handler, 'Drupal\Core\Block\BlockPluginInterface', Block::class, 'Drupal\Core\Block\Annotation\Block');

If it does, flag those plugins as using deprecated annotation.

Thought two feels extremely hard. Thought one also has its challenges in assuming we can find the original annotation class.

I don't know anything about how phpstan internals work, but I wonder why you think that's harder?

This is the line you also want to parse anyway to discover plugin managers that are not yet converted, The first argument is the folder, so you know for the look for the current annotation. The 6th and 7th are annotation and attribute class. if the 6th argument is an attribute class, the plugin manager is converted and the 7th is the old annotation class, you can search for that and suggest updates. IIf argument 6 is an annotation class, you can instead trigger a deprecation to add an attribute class.

The unusual part of both approaches is that you need to scan for them in *all* of Drupal core, other contrib modules and the parsed module/folder, not just the folder being scanned. For example when scanning search_api_solr, you also want to find the plugin managers in search_api module, so you can find the plugins that search_api_solr provides for that. Not sure if that's something you have needed to to?

The good thing is that plugin managers are tagged services, so maybe you can start there? Specifically, pretty much all plugin plugin managers you can work with look like this:

```
plugin.manager.image.effect:
class: Drupal\image\ImageEffectManager
parent: default_plugin_manager
```

So, find all "parent: default_plugin_manager", load their class, then that constructor parent call, which almost the vast majority of those plugin managers will have.

So, find all "parent: default_plugin_manager", load their class, then that constructor parent call, which almost the vast majority of those plugin managers will have.

It's kind of like that. But the logic is more so "Hey PHPStan, if you're analyzing a class and it implements PluginManagerInterface I care."

The problem is that "thought two" requires post-processing and isn't something that can be done while analyzing a plugin class, specifically.

Here's an example of checking to see if a cache backend has been defined: https://github.com/mglaman/phpstan-drupal/blob/main/src/Rules/Drupal/Plu.... Similar logic will be needed to grok the __construct call to determine if it uses an attribute. Not horrible. But also not fun.

In the end, I don't think the ability to detect this via PHPStan should be a blocker. But it should be considered. Upgrade Status can possible detect this, because its stateful and might be able to load all plugins a module provides to see if their using a deprecated annotation format.

I just wanted to open the discussion.

> It's kind of like that. But the logic is more so "Hey PHPStan, if you're analyzing a class and it implements PluginManagerInterface I care."

Right, but you couldn't rely on that anyway because PHPStan is never going to look like that at *all* the plugin manager classes in core and other modules when you scan one module/folder of modules? So I'd imagine you'd need to listen on classes implementing PluginInterface, see if you have an annotation and no attribute and then fetch the (cached?) plugin manager info to find the attribute class you need.

I see how Thought one would be easier then but I still think that's tricky to find the Annotation class reliably. At best you have to make some guesses. I guess it would be easier to just hardcode the ones from core, and then hope that most contrib modules respect the recommendation that the first part in the Plugin namespace should be the module name, like Drupal\views\filter with @ViewsFilter. So you look for a views module, and then in its Annotation namespace you find a clas named ViewsFilter. On match, the safest bet is IMHO to just look for the same class in the Attribute namespace. IMHO that will have a much bigger chance of success compared to hoping that contrib will provide deprecation messages that you can parse.

And all of those steps are IMHO optional to provide better messages. You could always say "I found a plugin Class Foo, it has no Attribute, so sooner or later, it will need to be update, I just can't tell you to which class exactly and it might not actually exist yet."

The upgraded version is then, "switch @Foo to [#Foo]" and extra points for "requires version 1.2.0 of contrib module bar".

Trying to push this forward:

For test_xss_title, as proposed above, I just removed that. There are two tests methods in BlockXssTest.php using this plugin, neither actually relies on it:

* testXssInCategory() is completely bogus. The test is 8 years old and was added as part of #2512456: Implement the new block layout design to emphasize the primary interaction of placing a block, and it makes sure that <script>alert('XSS category');</script> doesn't exist on the page unescaped. But this test is literally the only place that the string "XSS category" exists and ever has existed. Unlike many other tests in this file, there is no assertEscaped() method to make sure that the string does exist as an escaped version.
* testXssInTitle() never actually uses the code-provided admin label as it provides a user label with the alert string. It also doesn't use assertEscaped(), adding that showed that the label isn't displayed on the frontpage as that is off by default. I switched that to the powered by block and added an assertEscaped() on both the block list and the frontpage to make sure we're actually testing what we think we are testing.

As mentioned in the original issue, what this tries to test is not how the system works. A translatable string is *not* escaped, we don't need to protect against XSS from code, such a block can just put whatever it wants in build() anyway. What we need to protect against is user-provided input, which is covered quite extensively in the test, both for the user provided and default labels through menus, views and so on.

Possibly this should be extracted into a separate issue as well so it can be reviewed in a more isolated case.

As proposed in #13, I also removed the unit test deprecation coverage that currently wasn't working, IMHO the test coverage on the default plugin manager test is sufficient for this.

For the layout_builder block, I included my example from #3255804: Hidden dependency on block_content in layout_builder, mostly just to see if there are any test fails left.

Changed the link.

The JS test looks very much random to me, I can't reproduce the kernel test fail locally, the expected deprecations work for me?

Rebased.

I'd love to get some reviews on this, then we can move the block xss test related changes out into a separate issue, that probably makes sense to simplify this. Still needs a layout_builder fix too.

> It also doesn't use assertEscaped(), adding that showed that the label isn't displayed on the frontpage as that is off by default

That was actually my mistake. Block labels *are* shown by default, I switched to the powered by block and that one overrides the global default. Maybe not the best block to use for this, but wasn't sure what else I should use. None of the block_test plugins seem like a good fit and require extra setup, permissions or something else.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rebased after the last views plugin conversion landed, this might go back to green now, lets see.

An actionable task her would be to move the missing block conversions to a new issue, so we can get them reviewed and committed to simplify the remaining changes here. The layout block plugin has it's own issue that we need to revive and bring forward somehow.

What is the "Needs Developer Tooling" is for. It does not have a definition and I am not sure it fits within the tag guidelines. Can the existing DX be used?

Sorry, I should have left a comment here explaining. It is related to https://www.drupal.org/project/ideas/issues/3400254#comment-15565595
as an example of how it would work.

I only tagged two issues for reference. One that I created the intended follow up for phpstan-drupal and the other was this issue since it was posted as an example in the related issue.

I'd love your comments on the referenced issue. I don't want to derail this issue any further, but the tag is a first pass at a way to notify Gábor Hojtsy, mglaman, and bbrala of issues that may require attention in drupal rector and phpstan drupal.

I won't be tagging more issues until there is more discussion in the relevant issue and there is much more detail on why this may be necessary on the other issue.

Oh, I remember that issue. Thanks for working to develop and document the process. I do like a good process! But since that is not an Approved Plan I think using those tags in the Core issue queue is premature, and has caused confusion for one core committer. It would be better to remove the tags from the Core queue and have approval and a definition before using them. Thanks.

I think we should move any of the forgotten/missed conversions from this issue to a spin-off.

the trait hacks are hopefully temporary anyway and can be removed completely in #3502913: Add a fallback classloader that can handle missing traits for attribute discovery.

I created two child issues to get the easier parts in so this can focus on the deprecations and test changes:

#3509244: Remove TestXSSTitleBlock, update block XSS tests
#3509240: Convert remaining plugins that aren't using attributes

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

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Postponed on #3502913: Add a fallback classloader that can handle missing traits for attribute discovery

Un-assigning per Assigning ownership of a Drupal core issue.

#3502931: Add an Exit nested layout button is in, so unblocked.

There's code here but it needs a rebase and there are various open threads.

Updating the title to reflect that this now does more.

Rebased, MR is now pretty straight forward, just deprecation messages and test changes. Will need to check the MR comments, leaving at needs work.

I'm wondering if for:

Not supporting attribute discovery in ' . __CLASS__ . ' is deprecated in drupal:11.2.0 and is removed from drupal:13.0.0. Provide an Attribute class and an Annotation class for BC.

We should deprecate this for removal in 12.0.0 - e.g. force contrib plugin managers to support attribute discovery from Drupal 12.

If we do that, then contrib and custom modules can reliably use attributes for every plugin once they require Drupal 12, and don't have to continue declaring both annotations and attributes even when annotation discovery has been removed from core into Drupal 13, while Drupal 12 is still supported.

That also gives the maximum amount of time for contrib + custom plugins to be updated to attributes before we entirely drop annotation support (in Drupal 13 hopefully), otherwise they need to wait an unspecified amount of time before the relevant plugin manager gets updated.

I like that, can update it accordingly.

The conversion is a non-trivial amount of work, especially on modules with a lot of plugins, but the minimal step is just the attributes, don't even have to convert your own plugins. But your own plugins also have the advantage that you don't need to worry about BC.

I agree with #47, for the reasons stated there and in #49.

I updated the deprecation messages and linked to the correct change record.

I also made a few updates to the original change record to mention that attributes and annotations can be combined to be backwards compatible and also mentioned the error handling stuff a bit, feel free to adjust: https://www.drupal.org/node/3395575/revisions/view/13465802/13973808. I think that makes more sense than a new change record even though it's quite old. It's not a change you really need to be aware of unless you run into into problems (and are not yet on Drupal 11.2)

Properly updated the test assertions now.

Asserting multiple deprecation messages is painful as that's somehow done as a single assertion. Having multiple deprecations asserted together often doesn't work, so I had to add another test method to be able to assert the deprecation message from DefaultPluginManager. I think it's useful to have explicit test coverage as it's generic/dynamic.

This looks really good to me and I can't see anything to complain about.

Moving to RTBC, but that means I can't commit it straight away, but could do so in a few days if there's an additional review/+1 and no-one else has.

I think we really want to get this into 11.2 given #3518671: [policy, no patch] Defer disruptive 11.3 deprecations for removal until 13.0 and the fact it's a cascading deprecation that will need a lot of individual contrib releases to full work its way through. 11.2 gives us three years for all of that to happen.

Do the annotation-only discovery classes need to be deprecated? For any plugin managers out there that override getDiscovery() and use these classes specifically. I think last year there was an issue in Rules, because that module had a plugin manager like that. Though, the plugin manager would also need to not inherit from DefaultPluginManager or call its constructor, because the deprecation for the missing $plugin_definition_attribute_name property would be triggered for any that do.

Drupal\Component\Annotation\Plugin\Discovery\AnnotatedClassDiscovery
Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery
Drupal\migrate\Plugin\Discovery\AnnotatedClassDiscoveryAutomatedProviders

See the referenced-by #3521472: Deprecate AnnotatedClassDiscovery, @catch opened that recently before I updated the title here, so I thought we could do that there.

I think we should deprecate them before we remove them, even if it's not strictly necessary to notify plugin implementors, back to needs work for that.

If the deprecation of those classes is being done here, should #3521472: Deprecate AnnotatedClassDiscovery be used for their removal in 12.x?

Oh wait, look who opened that issue a few days ago, let's use that to deprecate all three classes, since this one is urgent and the class deprecations can happen any time before 12.x. Apologies for the status pong, anyone would think I really want to get rid of annotations or something.

(this was a reply to #56, I had a crosspost then with #58, still posting it for the bit on phpstan)

I can do it here, but I meant to deprecate in the other issue, not remove. So that we can land this, especially to get the D12 deprecation into 11.2

We still conditionally call those classes in DefaultPluginManager, and I just verified that phpstan complains about that, so we need to add additional comments o baseline changes for that.

Couple comments on the MR: one nit that's not a blocker. One thing that is probably an out-of-scope bug without test coverage.

Questions answered. +1 for RTBC.

Don't want to un-RTBC, but while reviewing the MR, I grepped the code for other references to this nid, and found:

modules/migrate/src/Plugin/Discovery/AnnotatedDiscoveryAutomatedProvidersTrait.php

/**
 * Provides method for annotation discovery with multiple providers.
 *
 * @internal
 *   This trait is a temporary solution until annotation discovery is removed.
 *   @see https://www.drupal.org/project/drupal/issues/3265945
 */
trait AnnotatedDiscoveryAutomatedProvidersTrait {

core/modules/migrate/src/Plugin/Discovery/AttributeDiscoveryWithAnnotationsAutomatedProviders.php

    // @todo Handle deprecating definitions discovery via annotations in
    // https://www.drupal.org/project/drupal/issues/3265945.

The first seems like we would ignore that @see until we completely remove annotations, not just trigger deprecations. But I'm unsure on the second. That seems like maybe something else we need to deal with here?

I'll keep looking at the MR, but wanted to post here since these files don't yet show up in the diff and it's not so obvious how to open MR threads about them...

Between #62 and the MR thread nits, bumping back for at least NR.

Initial pass at saving credits:

berdir and duadua for the code
catch, andypost, quietone, godotislate, myself and larowlan for MR reviews

The 1-off phpcs fix from prabha1997 which didn't resolve the problem and was the only effort here at all doesn't seem to qualify. Nor the other commenters so far.

Re #62: the @todo in the trait could probably be updated to point at #3521472: Deprecate AnnotatedClassDiscovery, or we could just note in that issue to make sure that trait gets looked.

parseClass() in migrate's AttributeDiscoveryWithAnnotationsAutomatedProviders does need a similar change to what was done in core's AttributeDiscoveryWithAnnotations parseClass(). Nice spot.

(The migrate source plugin stuff is really annoying.)

Great, thanks for confirming! NW for AttributeDiscoveryWithAnnotationsAutomatedProviders::parseClass() then.

we could just note in that issue to make sure that trait gets looked.

Updated IS in #3521472: Deprecate AnnotatedClassDiscovery to reference 3 classes mentioned in #54 and trait in #62.

Sorry, not sure how the status got changed to NR.

I updated the issue summary to answer some questions from @dww about min version and D12/13.

I'll try to update the MR tonight (CEST), if someone has time before that, feel free to pick this up before that.

Found a bit of time to work on this.

Resolved the threads, updated one reference to this issue to the other one where we deprecate those classes and updated the migrate class to trigger the same deprecation, nice catch.

Something is off with MigrateSource.

It looks like #3009349: Revert migrate_drupal source annotations to attributes conversion has reverted #3421014: Convert MigrateSource plugin discovery to attributes, all MigrateSource plugins use annotation again?

To be able to proceed with this, I would suggest we open a separate issue to fix this and then add the deprecation for migrate plugins there?

Reverted the migrate change, created a follow-up and pointed the @todo that: #3522409: Triggering deprecations for migrate plugins using annotations and plugin types not supporting attributes

With the @todo comment in the trait updated to point at #3521472: Deprecate AnnotatedClassDiscovery and the follow up for Migrate source as noted in #73, I think this is good to go back to RTBC.

I've opened #3522740: Provide a way for plugins to specify the version that annotations are deprecate in for @quietone's remaining feedback on the MR, I don't think within the context of the MR we can make the message any more specific, but maybe we can come up with a way for plugin type providers to specify a more specific version and use that. But don't think we have any similar examples in core at all to base this pattern on.

Otherwise this looks great, things always look easier after the fifty previous highly complex issues, so committed/pushed to 11.x, thanks!

After committing this... Do we need a new change record pointing out that attribute classes will be required from 12.0.0? That bit feels like new information.

I created https://www.drupal.org/node/3522776 and also updated to other two change records to link to this issue as well.