Use PHP attributes instead of doctrine annotations

So it smells like NIH

@andypost: What does "NIH" stand for?

Not Invented Here? https://en.wikipedia.org/wiki/Not_invented_here

fwiw I think we should definitely prefix the HTML attribute class(es) like HtmlAttribute.

For PHP attributes, I can see the arguments in both directions and don't really have a strong opinion at the moment.

Working on #142 for Action.

I started reviewing this again and I'm finding the use of PhpAttribute everywhere a bit jarring to read. I also agree with @andypost in that Symfony is just using Attribute and to me that is a good reason to do the same.

📌 Move Attribute classes under Drupal\Component Needs review has been reworked to use HtmlAttribute as suggested in #148 - I even wonder if over there we should *only* use HtmlAttribute in the namespace, but not in the class names - namespaces exist in order to disambiguate things, we shouldn't need to repeat this in the class names as well.

There are some nitpick comments that need work, but overall this looks almost ready to go to me - once we make a decision on naming things.

Tagging "Needs followup" to deal with the new deprecation skip, I think it is out of scope to solve here but might cause us to ignore Symfony changes further down the line so it is important to solve after this gets in.

It would be nice to get this in soon so we can spin off conversion issues for all annotations and ideally get them all done for Drupal 11.

Thanks @longwave. Fixed comments 🥂

- Created 📌 Handle newly added deprecation skip of "%Method "[^"]+" might add "[^"]+" as a native return type declaration in the future. Do the same in (child class|implementation) "Drupal\\[^"]+" now to avoid errors or add an explicit @return annotation to suppress" Postponed to deal with the new deprecation skip.
- "Cross-fixed" (together with @VladimirAus) the latest suggestions made by @longwave

This looks nice, some thoughts:

* 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work

We can do it as a follow-up, but don't we need to do this per plugin type anyway, so it's not blocked on having all plugin types converted? For example after this, we'd do a deprecation message on block plugins, but not entity types. Should be easy to do as we know exactly which ones are using PhpAttributeDiscoveryWithAnnotations? We can't do a deprecation on plugin types that haven't been converted yet in contrib anyway.

There's a second thing to do a deprecation on, and that's a deprecating the old discovery classes and plugin types that don't support php attributes yet. That will need to wait until core is fully converted. On the other hand, the sooner we start to tell contrib about that, the better and I guess it will take core some time to convert over all plugin types. We try to avoid it, but we could add the deprecation soon and ignore the not-yet-converted core plugin types..

* Determine whether to support plugin annotations convert whatever you throw in to the plugin annotation into the plugin definition approach - it's a dumping ground. See \Drupal\Component\Annotation\Plugin::parse()

Do we really have a choice on this? Entity types especially are known to do this, as entity and other modules module adds extra features, content_translation in core does that too (content_translation_entity_type_alter). What if we handle that through an "extra" property, so all the non-standard things need to be put inside that and then it's just an array? Then it's also up to each plugin type whether or not it wants to support that. I guess alter hooks and so on can still do it on the parsed definition but it wouldn't be possible to put it in the annotation upfront. tmgmt_content_field_info_alter() is an example where I've used that myself.

I started reviewing this again and I'm finding the use of PhpAttribute everywhere a bit jarring to read. I also agree with @andypost in that Symfony is just using Attribute and to me that is a good reason to do the same.

I think if we use HtmlAttribute for HTML attributes, it's OK to skip the prefix here and just use Attribute.

Went round on this a few times, even if we did use the prefix, it should only be in the namespace and not the class names. The thing that flipped it for me is where modules are defining attributes - there the PhpAttribute does look jarring.

@Berdir re: the dumping ground, isn't the alternative is to have separate annotations? I think you are talking about e.g. content_translation_metadata which, if defined in code, could become:

#[EntityType(
  id: ...,
)]
#[ContentTranslationMetadata(ContentTranslationMetadataWrapper::class)]
class ...

This attribute could then could be retrieved only where it is actually used (and a default applied if no attribute is set) - but that loses the ability to alter it easily, so we might need a new mechanism for that?

Separate annotations sounds promising.

Re #138, interesting idea, but I don't quite understand how this would be accessed then. We don't want to parse those attributes at runtime when content_translation needs it, or do we?

Looking at the examples where we use that in content_translation, none of them are something that should do something with all entity types that have a certain key set, but that could very well be a use case. We could say if you do something like that then you need to set up your own discovery/caching, but that also means extra cache lookups if you do implement a cache for it. And yeah, BC is going to be fun, also with my "extra" idea.

Either way, I think that's not something we have to figure out now to get this in.

Seems to have a valid failure in the MR

I've opened an issue to decide on where to put annotation classes, so all annotation issues can work to a common standard: 📌 [policy, no patch] Decide on a namespace for PHP Attribute classes Fixed .

Copying a comment from 📌 [policy, no patch] Decide on a namespace for PHP Attribute classes Fixed

I think I made a mistake in here - I don't think that should be create Drupal\Component\Attribute ... I think I made an Attribute component because I was copying the Annotation component. But looking at this more closely I think the Drupal\Component\Attribute in that issue should be Drupal\Component\Plugin\Attribute because that's what it is for. No I look at it, I think having a Drupal\Component\Attribute is odd - like what are the attributes for?!!? It's the same with Drupal\Component\Annotation - it looks generic but actually it is part of the plugin system.

I've moved all the code from Drupal\Component\PhpAttribute to Drupal\Component\Plugin\Attribute and all the module attributes to MODULE_NAME\Attirbute.

I've also added test coverage based on the old test coverage for annotations. I'd been working on this a long time ago but forgot to push. Unfortunately some functional changes occurred to attributes before others we've added test coverage. This has resulted in things that could have been assumed about annotations and attributes are no longer true. For example the plugin attribute was made abstract where as the plugin annotation is concrete. The tests made use of this. So I've made it concrete again for now. I think that before we make further changes to the Drupal\Component\Plugin\Attribute functionality we need to ensure that the test coverage in Drupal\Tests\Component\Plugin\Attribute mirrors that of Drupal\Tests\Component\Annotation as much as possible and then we can make improvements with the knowledge that we're diverging from a 1-to-1 conversion.

What's left here except extra tests and CR?

📌 [policy, no patch] Decide on a namespace for PHP Attribute classes Fixed is resolved and already implemented here - just noting for easier scannability.

Added a little bit of feedback to the MR.

What an amazing win in terms of developer experience.

• Rebased the branch
• Addressed a comment by @longwave
• Simplifed a tiny bit of code in AttributeBridgeDecorator

I was looking at the todo

::parseClass<code>:
<blockquote>
      // @todo Handle deprecating definitions discover via annotations.
</blockquote>
Given that Drupal 8/9 did some magic with the loading of annotations, it feels like it will not be enough to put <code>@deprecated

onto existing Annotation classes.

A small suggestion:

• Add @deprecated to the existing annotation classes, as this doesn't hurt
• Add a property onto the old annotation classes to point to the new attributes
• Let AttributeDiscoveryWithAnnotations::parseClass trigger a deprecation on runtime

After reviewing the comments mentioned above, it appears that the general consensus is to directly use the new PluralTranslation approach 📌 Use PHP attributes instead of doctrine annotations Fixed . However, there is a concern regarding the current placement of this object within the Drupal\Core\Annotation namespace, which does not seem like a progressive step.

Moving forward, there are several possible options to consider:

• Create a new object with the same properties within the Drupal\Core\StringTranslation\Attribute\PluralTranslation namespace: This option involves developing a fresh object that mirrors the existing properties but resides in the appropriate Drupal\Core\StringTranslation\Attribute\PluralTranslation namespace.
• Accept the fact that the object is in the wrong namespace: This option may involve some compromises or implications related to code organization and maintainability.
• Defer the concern to a later point and open a follow-up ticket: Since there are no usages of the object outside of entity annotations in the core

The MR could use a rebase onto 11.x to match the new core branching scheme.

I'm having trouble seeing what's left to do here and how much is for follow-ups.

Rebased and opened new MR from the old one only deprecation for constraint makes sense to fix https://git.drupalcode.org/project/drupal/-/merge_requests/1576#note_167475

Ready for review, removed last commit as it was for 📌 GitlabCI should fetch less from git Needs review

I did not want to generate a comment for every instance of $class being used as a variable, but those seem like good candidates for a @var or @phpstan-var annotation with the value class-string to help signify to developers that this is a special type of string.

Seems open threads in MR.

Did not review.

Seems to have CC failure.

Looking at the remaining tags though it was tagged for Issue summary update in #134. There are few remaining tasks wonder if they have been done/could be crossed out if so.

Assuming the change record is on hold until the final solution though.

Looking at this now.

• Two change records provided: one for plugins and one for plugin types (As suggested by longwave)
• Update the issue summary

Discussed the POTX issue at Drupalcon Lille with @catch, @Gábor Hojtsy and @alexpott.

After working through the possible issues we think that even though POTX runs on PHP 5.6 and the tokeniser does not understand the syntax, it continues parsing when it finds syntax it doesn't understand and therefore the new TranslatableMarkup() syntax should still be discovered.

A basic test case can be found at https://3v4l.org/SeWYv7 - when run on PHP 8.1 vs PHP 5.6 the output is different but the translatable string is still tokenised in the same way. It would be a good idea to add a test case for this to POTX itself.

This is some interesting research about the php parsing process!

This is some interesting research about the php parsing process!

It works completely by accident (or maybe not, they might have done this on purpose) - the first line is treated as a comment, the next lines are garbage, but then it gets to new Class and it recognises that even if the surroundings are meaningless. When we tried it out we had no expectation it would actually work, just wanted to see if it completely choked up the rest of the file or not.

alexpott → closed merge request !1576

Left a comment on the DefaultPluginManager constructor, there's also some nits from @catch, setting to needs work for that.

> the first line is treated as a comment, the next lines are garbage, but then it gets to new Class and it recognises that even if the surroundings are meaningless.

FWIW, that will only work if the attribute definition is using multi-line syntax, if everything is written on a single line all of it is just a comment. But I guess that's something we can live with. Anything complex enough to have translatable labels will likely be on multiple lines.

FWIW, that will only work if the attribute definition is using multi-line syntax, if everything is written on a single line all of it is just a comment. But I guess that's something we can live with. Anything complex enough to have translatable labels will likely be on multiple lines.

Yes I think it will tide us over until potx is running on PHP 8.1, then it should actually work properly again. Until we got to that point, we were thinking about shipping a file with core that called t('Some string that is in an annotation somewhere') built on commit or something to workaround it, so working by accident is a big step over that.

+1 for refactoring the constructor to the future argument order now, instead of having to juggle them again later.

Addressed MR feedback.

NW for latest comments.

We discussed my comment in Slack, and there actually can be a default value for the attribute class, so that can be mostly ignored, I think we just want to add an explicit @todo to add that default then.

I created 📌 [meta] Convert all core plugin types to attribute discovery Active as a follow-up and started looking into entity type annotations in 📌 Convert entity type discovery to PHP attributes Needs review

The variadics idea from #43 seems to work but PHPStorm doesn't fully understand it, you can't click on the attribute to jump to the definition. It also complains about the parameter order, suggesting to keep them in order, autoresolving that puts $revision_metadata_keys first as the only thing that it understands.

I see that PluralTranslation was discussed to be replaced with new PluralTranslationMarkup. That doesn't work, because that has a required count argument that we don't have. But I think we don't need to do anything special. Looking at \Drupal\Core\Entity\EntityType::getCountLabel() it actually expects $this->label_count to be a regular array with singular, plural and context keys, so if we convert it into an array it just works I think.

What's a bit weird is that we have these properties defined in two places now, both on \Drupal\Core\Entity\Attribute\EntityType (not documented yet) and on \Drupal\Core\Entity\EntityType, then we convert the first back to an array to create the second. That's not really different to now, but now it doesn't duplicate all the properties and just refers to the other class.

So far I didn't run into any issues with undefined parameters, field_ui_route_name for example is actually defined for example. And I think the solution for that already exists in \Drupal\Core\Entity\EntityType, with the additional property, anything that's not a defined property is actually put into that with get()/set(), so I imagine just putting extra stuff directly in additional in the annotation should work?

With that, I was able to define the 3 attribute classes and convert Node to that. That conversion was pretty painful, with array definitions being different from named attributes and also the double to single quote change as otherwise the \ needs to be escaped. That said, I did just realize that we can actually do NodeStorage::class now in there, but we possibly don't want to do that in the initial conversion?

I didn't run any tests, but I was able to manually create a node through the UI, so seems to be working pretty ok?

See https://git.drupalcode.org/project/drupal/-/merge_requests/5099/diffs?co...

Added an @todo for the default attribute class once we have removed annotations.

Refactored the additional namespaces argument to be used in annotation discovery only.

I think this is very close. There are some small bits on the BC layer and test @todos that I found.

It's not really scoped like I would have. IMHO, 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work should have been done here as it's a one-line addition (if we ignore tests) and instead a bit less real conversion as Block and Action make this a very large MR to review. But that's just my opinion, and the follow-up for deprecations is pretty much ready and and can go in soon after.

We also have the issue to convert the remaining types, and I did a proof of concept as a child issue to verify that the probably most complex EntityType annotation can be converted too (Node entity type) and that we can handle those child annotation/attribute classes the entity type system as well in a sane way.

> Determine whether to support plugin annotations convert whatever you throw in to the plugin annotation into the plugin definition approach - it's a dumping ground. See \Drupal\Component\Annotation\Plugin::parse()

This is still open. I think the variadics trick that we use for child inheritance could also work for this, but it will still fail in the current implementation and I'm not sure if we even *want* to support this. I think we can figure this out when we get to use cases for it. One example would be entity types relying on entity.module which add extra features and annotation keys for this. But specifically entity types should work, the extra stuff just needs to be put explicitly in additional, because that's how it's then managed anyway (\Drupal\Core\Entity\EntityType::$additional). And adding stuff in alter hooks will still work, as attribute classes are only used for discovery.

> The research shows that this already works, as we use the same TranslatableMarkup

This is actually *not* fully solved for the plural case, as explained above, we can not use PluralTranslationMarkup, it's just a plain array atm, so we likely do need to add a class for it just for discovery. But that can be done when needed in the entity issue.

Rebased, addressed some @todos and the MR feedback. I think the two unit tests are valid enough to test basic behaviour, and the XSS test is still worthwhile to check that TranslatableMarkup used in attributes is still correctly escaped in the admin UI.

Agree that we can deal with the "dumping ground" problem in a followup when we actually need that functionality, and same for plural translations; would really like to get this in to 10.2.x if possible so contrib can start using it and we can get the rest of the conversions in 10.3.x.

> and the XSS test is still worthwhile to check that TranslatableMarkup used in attributes is still correctly escaped in the admin UI.

Except that's never actually tested because that's not how it works. TranslatableMarkup is a markup object. That is *not* escaped. We're just not actually testing this. \Drupal\Tests\block\Functional\BlockXssTest::testXssInTitle() only tests a user-provided label on the block list and *not* the add block page. If you extend that test with the following then it *does* fail:

$this->clickLink('Place block');
$this->assertSession()->responseNotContains("<script>alert('XSS subject');</script>");

And that's OK, because that's code (in this case. But that's exactly why it's really really bad when contrib/custom module put config strings or other things through t() or log user input directly to the logger, because that's an instant XSS security issue).

We don't actually need TestXSSTitleBlock, that test can use any other arbitrary block plugin.

Removed the conversion of the XSS test block as fixing anything to do with that here is getting out of scope and makes the MR harder to review; we can defer that to a followup where we deprecate the Block annotation.

Added explicit use of AttributeDiscovery where the annotation name is not set in DefaultPluginManager. Unsure if we need to add more test coverage here for that case, or whether we can defer that to a followup as well.

Added additional explicit test coverage of DefaultPluginManager by extending plugin_test module with an attribute, asserting that annotations and/or attributes are correctly discovered. I think we can then build on this as we start adding deprecations and converting existing plugin types over.

Great, thanks for that. I trust unit test coverage less and less, too often it doesn't actually test what we think it should :)

I verified that reverting "b89c86425a - Allow attribute-only discovery." causes the new test to fail with:

TypeError : Drupal\Core\Plugin\Discovery\AttributeDiscoveryWithAnnotations::__construct(): Argument #4 ($plugin_definition_annotation_name) must be of type string, null given, called in /var/www/html/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php on line 292

I think this is ready. There's a lot left to do with some special leftovers and edge cases like plural translation, all core plugin type conversions as well as tooling for rector/phpstan but this is a good starting point that we need to get in asap so we have enough time to get the remaining things done in time as well.

Went through this again line by line hoping to commit it, but found one possible issue and some non-blocking nits. Leaving at RTBC for now.

Replied to all MR feedback: a lot of code is copied from attributes and could be improved, but to me that is out of scope here. The layout builder inline block changes also can be deferred to a followup to make this MR smaller. I also changed some new code to use constructor property promotion.

Oops I broke the tests.

Oh, I think we have to do this conversion here. The attribute discovery loads the class anyway because of the namespace. The problem I guess is that if any other block plugins exist in contrib that have similar optional dependencies, they will run into the same problem as soon as this lands.

Added 📌 Write a console command to help converting annotations to attributes for plugins Active

Can we just make layout_builder declare the dependency on block_content?

OTOH discovery can check that provider exists in the module.list so it will prevent issues with other plugin kinds.

Anyway it smells like follow-up to improve discovery tests for disabled/altered plugins

> Oh, I think we have to do this conversion here. The attribute discovery loads the class anyway because of the namespace. The problem I guess is that if any other block plugins exist in contrib that have similar optional dependencies, they will run into the same problem as soon as this lands.

Yes, that's a problem.

Some ideas I was thinking about:

* Discover annotations before attributes when BC is enabled. I'm not sure about the performance implications, obviously just for core it would be slower as we need to both read and parse most plugin files then, but real sites will have a lot of not yet converted plugins at least initially.

Problem: The current order would allow modules to have both an attribute and an annotation to be compatible with older core versions. In this case we'd still throw a deprecation message then, but modules could just ignore that as it will work fine in 11.0. And phpstan could respect this.

* Could we catch errors and ignore such classes, maybe with a log message?

* Possibly as a long-term solution, introduce something like provider-subfolders which we only look at if the corresponding module is enabled?

I switched the order in AttributeDiscoveryWithAnnotations so annotations are parsed first; this sidesteps the issue with the layout builder block plugin for the time being.

ReflectionClass throws a fatal error if autoloading fails. I am not sure if it is possible to catch or work around this. When layout builder is installed but block content is not:

> new ReflectionClass('Drupal\layout_builder\Plugin\Block\InlineBlock');
PHP Fatal error:  Trait "Drupal\block_content\Access\RefinableDependentAccessTrait" not found in /var/www/html/drupal/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php on line 32

The layout builder block problem is an edge case that I think we should solve over in 📌 Hidden dependency on block_content in layout_builder Needs work .

As far as I can see this will only affect derived plugins; the issue is that the base plugin is discovered, and then if block_content is not installed, the deriver returns zero derivatives, so the plugin is never instantiated. If it was not derived, then the plugin could get instantiated at any point and the fatal error would be triggered. If we consider that to be a bug in Layout Builder then we can solve it by adding a dependency or by moving the interface/trait into core.

If this does only affect derived plugins then I suspect that contrib/custom code will have few to none that have the same issue and I'm less worried about it being a problem in the future.

@Berdir reminded me that plugins can also specify a provider and if that provider is not enabled, it will be filtered from the plugin list; previously this would happen without loading the class but attribute discovery will load the class and so if it relies on code (traits, interfaces) from a module that is not installed, a similar crash will occur.

Looks like the class not found fatal is throwable, so I think we can wrap it in a try/catch and be OK: https://3v4l.org/Kdppv

Tried that already with @Berdir but it's an uncatchable fatal error when a trait is not found: https://3v4l.org/jFId8

trait really is a super-special case. I think hardening the attribute discovery and catching whatever throwable we can and provide it with a bit more context maybe could be useful as a follow-up.

I think this is the best we can do at the moment. Trying annotations first is not so great for performance, but then it will only fail when contrib/custom code switches and they'll hopefully catch the issue while doing so and not immediately after updating core.

The provider-for-another-module thing is used somewhat often, we use it a lot in our monitoring project for example, but using a trait provided by another module really is rare, I couldn't find another example like this.

Annotations have problems like that too, what's happening frequently to me is that I copy paste a plugin, forget to update the namespace, and then get a weird error about the class already having been defined because it's for some reason loaded more than once.

Lets try this RTBC thing again :)

I opened 📌 Parse attributes before annotations Active to see if we can switch back to parsing attributes first in a future minor release of 10.x

Lots to sort out in follow-ups but this is a really good first step.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

Amazing.

To avoid crossposting, I'm going through the change records a bit, doing some improvements and publishing them.

Adding some tags.

Every time I see new TranslatableMarkup() it makes me happy. Remembering the D8 journey from safe markup lists to objects, overcoming doctrine annotation changes and deprecations... and now seeing all of this done with PHP language features - Stringable object and attributes. Lovely.

Added a note (directly) to the 10.2.0 release notes draft, a bit more verbose than the one here.

andypost → closed merge request !4787

The entity types that core ships with (Node, Block, Media, etc), is there a single issue to convert their annotations to attributes, or separate issues for each entity type?

And if there's still more work to be done until we get to that point, then that's also fine. I just couldn't find a clear path for what's next, as the MR here doesn't seem to change existing entity types and it wasn't clear from looking at the list of child issues.

@AaronMcHale no direct link from here but see 📌 Convert entity type discovery to PHP attributes Needs review which is a child issue of 📌 [meta] Convert all core plugin types to attribute discovery Active .

Perfect! Thanks @catch.

Just to get this right: the annotation classes and plugin managers will be kept unchanged for now?
Is this only about changing plugin implementations from annotations to attributes? Or is there some core magic in the background?

Annotations and their discovery mechanism will eventually be deprecated, but we have to convert all (or at least most) of core's annotations and plugin managers first. It is currently not clear whether this will happen in time for Drupal 11 or whether we will support annotations until Drupal 12.

If you have a plugin manager in contrib you are encouraged to start converting to attributes now, if you can; plugin managers can support both annotations and attributes side by side for the time being while the conversion takes place.

I've been thinking about this in regards to d11 readiness. And i feel trying to sqreeze this removal into 11 is a very risky endeavor. We will be done late, and this will make contrib d11 readiness a lot harder. A big change like this would be a lot easier to arrange if we gave it a little more time.

There needs to be a way to have contrib support both types of discovery in a since we want to be able to have a module support ^major || ^marjor+1. Will that be possible, it felt like that earlier, but starting to doubt that a little.

I'm wondering if we could move the annotation/attribute + annotation discovery to a contributed module for Drupal 11. This would mean core wouldn't be on the hook for supporting Doctrine annotations until 2028/9 (I'm assuming it'll be EOL more like 2025ish) while also allowing contrib to catch up at a more reasonable place.

I'm most concerned about contrib project plugin managers, and contrib/custom implementations of those plugin types, because that's a cascade of conversions with hard blockers - so if we can support those converting later, that'd be the highest risk thing. However, if we can make annotations with deprecations work again via contrib for core plugin types, that would smooth things out a lot too.

Opened 🌱 [policy, no patch] Allow both annotations and attributes in Drupal 11 Active to discuss the future on this otherwise fixed issue.

Are there any code examples for converting plugin managers / annotation classes?

@drubb see https://www.drupal.org/node/3395582 → and the Action and Block plugins in core have been converted as examples to follow - if this is not clear enough please let us know.

Automatically closed - issue fixed for 2 weeks with no activity.