Triggering deprecations for plugins using annotations when core plugin type has been converted to 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.