Use PHP attributes instead of doctrine annotations

Would love to see how awful this looks with a large entity-type annotation

Don't get me wrong, I think moving away from Doctrine is great.

But I'm not sure the wall of info we have on content entities is going to translate well to built-in attributes.

Nuts idea, but we're using frontmatter for help topics.

Could we do something similar for those big annotations?

FWIW I think the multiple attributes approach looks better

Nested attributes were added in PHP 8.1, so to handle translations we might need to wait until that is our minimum.

There is at least one case, context sensitive conditions which use three levels of annotation:

 * @Condition(
 *   id = "user_role",
 *   label = @Translation("User Role"),
 *   context_definitions = {
 *     "user" = @ContextDefinition("entity:user", label = @Translation("User"))
 *   }
 * )

Attributes also support named arguments so there is a third way that IMO is cleaner than arrays or individual attributes - added to the IS.

Would love to see the "named arguments" version of this idea implemented!

The named arguments version has the benefit of enabling IDE suggestion and completion. Additionally it can use PHPs typesystem to its full advantage.

I think at this stage in Drupal's life we have learned too much to introduce a new array based API. Arrays are difficult to access and require runtime type-checking (duplicating what PHP now provides us out of the box). For example we now have #3016948: Type check theme variables to do this for the theme system which still uses arrays.

Having separate annotations per property would cause issues in completion (e.g. what if you forget BlockId? Where would the error actually happen).

Is moving away from annotations a performance boost?

+1 to what @Kingdutch said, the more we can enhance the DX the better.

Drush went with the named attributes approach. Compare two command declarations at - https://github.com/drush-ops/drush/blob/11.x/examples/Commands/XkcdComma.... Its not exactly pretty to read but the IDE completion is fantastic and avoids typo bugs.

Given that doctrine is abandoning packages, this might actually be critical (or, some alternative approach is).

Updating status to 'needs work'.

- * @Block(
- *   id = "system_powered_by_block",
- *   admin_label = @Translation("Powered by Drupal")
- * )
- */
+ #[Block(
+  id: "system_powered_by_block",
+  admin_label: new TranslatableMarkup("Powered by Drupal")
)]

Syntax-wise this looks pretty good to me, haven't done a more in-depth review of the MR yet.

The issue we're going to run into, as we did with annotations in the first place, is loading the PHP files via reflection in order to parse the definitions. It might be worth doing a full conversion and actually measuring the difference vs our current annotation parsing since PHP 8 has moved on a bit since 7.1 or whenever we started using annotations. We also might want to think about other approaches to cache rebuilding like offloading some rebuilds to a prewarm.php or fibers if that's doable?

There's also a wider issue that I'm not sure how to deal with - we can provide discovery in Drupal 9, but we won't be able to update core usages in Drupal 9, because it only requires PHP 7.3, and attributes are a syntax error on lower PHP versions.

Contrib will be able to update, but only by adding a PHP 8 or PHP 8.1 requirement themselves.

if i am not mistaken this would have a namespace clash with #2664570: Move Attribute classes under Drupal\Component

Very interested to see some of the perf results.

Looking at the Drupal\Core\Block\Attribute\Block class, I'm interested to see how this conversion plays with the split in \Drupal\Core\Layout\Annotation\Layout vs \Drupal\Core\Layout\LayoutDefinition (aka, object-based definitions). This might lend itself very well to dealing with attribute objects instead of arrays!

To clarify the intention here, it seems we would be deprecating annotations in D10 for removal in D11, not deprecating them in D11 as the current IS says. Is that correct ?

              // @todo For annotations we used a special reader to avoid
              //   reflection. At the moment there is no 3rd party library for
              //   parsing attributes without reflection. Ideas for performance
              //   improvements here:
              //   * Build our own library to do this.
              //   * Search the file for "#[" to see is there is an attribute.

PHP's reflection loads the reflected class into memory and never frees it from memory. So there'd be an upper bound to how many plugins we can discover in a single request, unless fibers can be used to free that memory, but I don't know if they can be (see #3257725-4: Add a cache prewarm API).

If fibers don't allow that memory to be freed, https://github.com/Roave/BetterReflection released 5.0 two weeks ago, with attributes support, but there's two concerns with using that library:

1. It's slower. We'd be paying for the memory savings with extra CPU time.
2. We'd become dependent on that package and its release management. For example, it took them a year from when PHP 8.0 was released to when they started supporting it. Not sure that kind of delay would be acceptable for us when it comes to compatibility with future PHP versions.

That library builds upon https://github.com/nikic/PHP-Parser. Alternatively, we could write our own attribute parser that builds upon PHP-Parser. PHP-Parser has a great track record so far with quickly keeping up with PHP changes, though who knows if that will continue given Nikita's recent change of focus.

If fibers allow the memory of loaded classes to be freed when the fiber terminates, that would be a pretty compelling reason to set PHP 8.1 as the minimum for Drupal 10.

If that's not the case though, then I don't think that nested attributes alone is enough of a compelling reason. I think we can still make this issue work without nested attributes.

As I wrote in #3223435: Track PHP 8.1 support in hosting and distributions, I think we can change from identifying translatability on the values to identifying it on the properties. That would remove the use-case for @Translation and @PluralTranslation needing child attribute classes.

Similarly, @ContextDefinition is only used within the "context_definitions" property of plugins, and every item in that property is a ContextDefinition, so there too we can have an attribute (or even just a typehint) on the "context_definitions" property itself that says it's an array of ContextDefinition objects.

Using the example from #7, Drupal\Core\Condition\Annotation\Condition already looks like this:

  /**
   * @var \Drupal\Core\Annotation\Translation
   */
  public $label;

  /**
   * @var \Drupal\Core\Annotation\ContextDefinition[]
   */
  public $context_definitions = [];

We can either leave that as-is or turn those into real type hints:

  public \Drupal\Core\Annotation\Translation $label;
  public \Drupal\Core\Annotation\ContextDefinition[] $context_definitions = [];

And then rewrite #7 as:

#[Condition(
  id: "user_role",
  label: "User Role",
  context_definitions: {
    "user": ["entity:user", {"label": "User"}]
   }
)]

And let the discovery system read the type hints for the properties of the Condition class and pass the attribute values to those corresponding constructors.

There's a 2 cases that classes already preloaded since PHP 7.4 (preloader) and 8.0 (JIT - can't find related but boost ~10%)

related issues
- #3108687: Compatibility with/take advantage of code preloading in PHP 7.4
- #2704571: Add an APCu classloader with a single entry
- https://www.drupal.org/project/preloader

Re #3252386-27: Use PHP attributes instead of doctrine annotations, in addition to #3252386-30: Use PHP attributes instead of doctrine annotations, if we adopted the PHP 8.0 approach, then require PHP >= 8.1 in Drupal 11, then it seems likely we'd want to switch later to the 8.1 approach later anyway. However, that would mean two API changes for contrib, as well as bc layers and deprecations on top.

The IS proposes several different ways this could be done. Needs updating to say which one has been chosen for the MR.

If loading plugin classes into memory during discovery is indeed no longer a bottleneck, then what's the advantage of attributes over a static class method?

With the named arguments / constructor property promotion approach, how should we handle attributes that extend other attributes? E.g., ConfigEntityType which extends EntityType but adds a few extra named arguments / properties? It would be a shame for ConfigEntityType's constructor declaration to have to repeat all of EntityType's named arguments. Should we do this with multiple attributes? E.g:

#[EntityType(
  id: "node_type",
  ...
)]
#[ConfigEntityType(
  config_prefix: "type",
)]
class NodeType ... {
}

Or does that leak too much ugliness to all of the plugins?

Actually, #42 can be solved with variadics. This works:

class A {
  function __construct(public $a, public $b, public $c) {
  }
}

class B extends A {
  function __construct(public $d, ...$others) {
    parent::__construct(...$others);
  }
}

$b = new B(a: 1, b: 2, c:3, d:4);
print_r($b);

Which means we can have a single attribute for ConfigEntityType and have it extend EntityType cleanly :)

The great thing about being able to create instances of objects in attributes is that we're not using attributes for this. We're using exactly the same objects as you would in regular PHP code. So for translatable strings that's \Drupal\Core\StringTranslation\TranslatableMarkup(). That means the way these strings are translated is exactly the same as regular PHP code. We've got rid a whole abstraction layer (i.e. \Drupal\Core\Annotation\Translation) and that's fantastic in my opinion.

I think we can achieve almost the same thing via PHP 8.0, but we'd need a bridge.

The PHP 8.1 approach from #40 is

#[Block(
  id: "test_context_aware",
  admin_label: new TranslatableMarkup("Test context-aware block"),
  context_definitions: [
    'user' => new EntityContextDefinition(
      data_type: 'entity:user',
      label: new TranslatableMarkup("User Context"),
      required: FALSE,
      constraints: [
        "NotNull" => [],
      ]
    ),
  ]
)]

which instantiates the following attribute class that's in the MR:

class Block extends Plugin {

  public function __construct(
    public readonly string $id,
    public readonly ?TranslatableMarkup $admin_label = NULL,
    public readonly ?TranslatableMarkup $category = NULL,
    public readonly array $context_definitions = [],
    public readonly ?string $deriver = NULL,
    public readonly array $forms = []
  ) {}

}

If we wanted to do this in PHP 8.0, we could do it as:

#[BlockBridge(
  id: "test_context_aware",
  admin_label: "Test context-aware block",
  context_definitions: [
    'user' => [
      'data_type' => 'entity:user',
      'label' => "User Context",
      'required' => FALSE,
      'constraints' => [
        "NotNull" => [],
      ]
    ),
  ]
)]

The implementation of the BlockBridge class could just be:

class BlockBridge extends Block {
  use AttributeBridgeTrait;
}

And that trait could be:

trait AttributeBridgeTrait {
  function __construct(...$args) {
    // Reflect on $this to determine the type declarations of each property in $args
    // and upcast each one accordingly by passing the value to that type's constructor.
    // For example:
    // - $admin_label is declared as TranslatableMarkup. Therefore do:
    //      $args['admin_label'] = new TranslatableMarkup($args['admin_label']);
    // - $context_definitions is declared as EntityContextDefinition[]. Therefore do:
    //      foreach ($args['context_definitions'] as $key => &$value) {
    //         $value = new EntityContextDefinition(...$value);
    //      }
    ...
    
    // Now pass the upcast $args to the underlying attribute class.
    __parent::construct(...$args);
  }
}

I don't know if this is better or worse DX.

With:

#[Block(
  id: "test_context_aware",
  admin_label: new TranslatableMarkup("Test context-aware block"),

you can immediately see that one of those strings is not translatable and the other is, whereas with:

#[BlockBridge(
  id: "test_context_aware",
  admin_label: "Test context-aware block",

you don't have that immediate feedback. You can only know by referencing the code that says:

class Block extends Plugin {

  public function __construct(
    public readonly string $id,
    public readonly ?TranslatableMarkup $admin_label = NULL,

But that's the same situation that we have with config files and their schema, so if it's okay for config files, why is it less ok for plugin attributes?

However, if we think the former way is significantly better than the latter way, then I bumped #3173787-31: [policy] Require PHP 8.1 for Drupal 10.0 if a dependency does, otherwise require PHP 8.0. (up from 7.3 in Drupal 9) for us to reconsider that issue.

Also decoupling the value from the translation marker is going to cause us heaps of pain when it comes to changing the POTX module to work with attributes.

All of the info that's needed, even in the PHP 8.0 approach above, is at least theoretically available to static analysis. Is your concern that there are technical limitations to POTX that would prevent it from figuring out what it needs to from that static analysis, or is your concern that it would be a lot of work to write that code?

> Yes the entity type attribute class is going to be horrific

How are we going to handle the fact that some entity type attributes don't belong in Core-core but in modules (e.g. field_ui_base_route)?

(Not objecting to this in general, definitely +1 to PHP attributes, just curious about details!)

@catch in comment #35:

if we adopted the PHP 8.0 approach, then require PHP >= 8.1 in Drupal 11, then it seems likely we'd want to switch later to the 8.1 approach later anyway. However, that would mean two API changes for contrib, as well as bc layers and deprecations on top.

I am not really a fan of this. Lets do it in one go. Doing it in D10 will require the minimum PHP version to be 8.1. This will make it for difficult for siteowners to switch for D10. And with D9 will EOL within 2 years from now, is something that we, to me, should not do. My suggestion would be to switch to PHP attributes in D11. Also now are we inventing our own approach with the chance of creating a new Drupalism. To me, it would be better to wait 2 years and see what the PHP community comes up with as the best solution. Maybe there will be new stuff in PHP 8.2. Just my 2 cents.

Trying to think if there's any way to introduce this in Drupal 10 (to allow us to potentially remove annotations in Drupal 11), if Drupal 10 had a PHP 8.0 requirement, without PHP 8.0 workarounds. I think there is, but it's not very pretty:

1. Add attribute discovery in 10.0.x as a new API, don't deprecate annotations yet, don't do any core conversions (but have an MR with the conversions to prove it all works).

2. In 10.3.x (i.e.when PHP 8.0 is at end of security support, and 11.x is 'coming soon'), deprecate annotations in core, with the deprecation message suppressed. Make sure we have a rector rule by this point to auto-update all modules.

At this point if a module wants to start using attributes (at least, if the attribute has any translatable strings or nesting), they'll need to introduce a PHP 8.1 requirement - but contrib can do that, and it's not the same as fatal errors from core after a minor update. Contrib won't need to worry about breaking compatibility with older Drupal 10 versions because the discovery was added in 10.0.x

3. As soon as 11.x opens, commit the conversion + drop annotation discovery (and the doctrine/annotations dependency).

This maintains the continous upgrade path as long as you update to PHP 8.1. Sites stuck on PHP 8.0 might find they're not able to update to newer versions of certain contrib modules, but they'll need to update PHP version to get to Drupal 11 anyway, so could do it then if not before.

We could also (at the 10.3.x or later point) decide to be less aggressive about dropping annotation support, if for example Doctrine has published their EOL date for doctrine/annotations and it's after Drupal 11's expected EOL date. However introducing the discovery in 10.0.x allows us to do things later on that we otherwise couldn't.

I just remembered that PHP 8.0 has union types. Therefore, #44 could be simplified. No need for a BlockBridge class. Instead we could just have the one Block class, but change its constructor from:

class Block extends Plugin {

  public function __construct(
    public string $id,
    public ?TranslatableMarkup $admin_label = NULL,
    public ?TranslatableMarkup $category = NULL,
    public array $context_definitions = [],
    public ?string $deriver = NULL,
    public array $forms = []
  ) {}

}

to:

class Block extends Plugin {

  public TranslatableMarkup $admin_label = NULL;
  public TranslatableMarkup $category = NULL;
  public array $context_definitions = [];

  public function __construct(
    public string $id,
    ?TranslatableMarkup|string $admin_label = NULL,
    ?TranslatableMarkup|string $category = NULL,
    array $context_definitions = [],
    public ?string $deriver = NULL,
    public array $forms = []
  ) {
    if (isset($admin_label) && !($admin_label instanceof TranslatableMarkup)) {
      $admin_label = new TranslatableMarkup($admin_label);
    }
    $this->admin_label = $admin_label;

    if (isset($category) && !($category instanceof TranslatableMarkup)) {
      $category = new TranslatableMarkup($category);
    }
    $this->category = $category;

    foreach ($context_definitions as $key => $value) {
      if (!($value instanceof EntityContextDefinition)) {
        $context_definitions[$key] = new EntityContextDefinition(...$value);
      }
    }
    $this->context_definitions = $context_definitions;
  }

}

This would allow people to convert their plugins from annotations to attributes starting in Drupal 10.0, just not being explicit about TranslatableMarkup and EntityContextDefinition within those attributes. Then at any point in time that the module wants to set a PHP 8.1 minimum, it could make those explicit.

Then along the lines of #48, for Drupal 10.3, we could deprecate the union types, and in 11.0 we could get back to the stricter and simpler constructor of the first code block.

need to also account for POTX and translation string extraction from the code

If we don't want to make our POTX generator smart enough to scan for attribute class constructors that allow union types that include TranslatableMarkup, we could instead push a little work/repetition to the plugins like the following:

#[Block(
  id: "test_context_aware",
  admin_label: "Test context-aware block",
  context_definitions: [
    'user' => [
      'data_type' => 'entity:user',
      'label' => "User Context",
      'required' => FALSE,
      'constraints' => [
        "NotNull" => [],
      ]
    ),
  ]
)]
#[TranslatableMarkup("Test context-aware block")]
#[TranslatableMarkup("User Context")]

Those last two being solely for POTX until the module adopts PHP 8.1 and inlines them.

To me it seems like it wouldn't be that bad to add a PHP-8.0-compatible iteration in D10, and then improve the DX in D11, especially so long as we have the help of rector rules.

I took a closer look at how likely it is that Doctrine will drop support for annotations. The question we're dealing with, is whether Doctrine is likely to drop support for Doctrine Annotations before the EOL of Drupal 10 or Drupal 11.

Unlike SimpleAnnotations - which they deprecated and announced as abandoned (and we had to fork), Doctrine Annotations is used by Doctrine ORM, this means they'd need to do a new major release of Doctrine ORM, after deprecating annotation support, to drop Doctrine Annotations.

Doctrine ORM has just added support for Attributes - very recently https://github.com/doctrine/orm/issues/9240. It's either not a full 1-1 replacement for annotations yet, or it's only been viable for about a month. Due to this, they haven't yet deprecated Annotations at all, but that's definitely the direction being taken.

To drop annotations, they'd need to deprecate it in ORM and any other components that rely on it in 2.x, release 3.x with a PHP 8.1 requirement, and then EOL 2.x There's a Doctrine ORM 3 milestone, but Doctrine doesn't have a release schedule or published roadmap, so no indication when that might be released, or whether the issues attached to it are accurate https://github.com/doctrine/orm/milestone/1

Given all that, there is absolutely no discernable timeline whatsoever, but there's definitely a fair bit of actual work required for them to drop support, it won't just happen one day.

---

@xjm (and everyone else) what would you think about putting both the PHP 8.0-compatible and PHP 8.1 syntax into 10.0.x? We'd need the 8.1 syntax support in 10.3/4 anyway to allow conversions in time for Drupal 11. If we support both in 10.0.x, then core and any other modules wanting to support 8.0 can use that, but there's the option for modules to add PHP 8.1 compatibility and do the work once too. Would be useful for custom modules especially and maybe new ones.

what would you think about putting both the PHP 8.0-compatible and PHP 8.1 syntax into 10.0.x?

That's what #49 does, and if we decide to make PHP 8.0 the minimum for Drupal 10 core, I don't see any reason why we wouldn't do it that way so as to support both. Enforcing non-nesting onto contrib would mean changing the constructor parameters from being union types to allowing strings only, but why would we do that if we're pretty sure we'll want to move in the direction of them being TranslatableMarkup objects later?

@effulgentsia the only reason not to do so is because it will mean some number of contrib modules will begin to require PHP 8.1 the day 10.0.x is released. You only need a handful of reasonably high usage modules to then mean that the de-facto PHP requirement is 8.1 to run potentially a majority of Drupal sites, and then core's PHP 8.0 support becomes mostly nominal and a bit pointless. However, it's a lot better than changing the API on everybody twice or not supporting Attributes in Drupal 10 at all, it might not be better than just requiring PHP 8.1 and calling it done.

#1892504: XSS in block titles is a bit vague on what the original XSS bug was, but I suspect it was XSS within a UI field. We don't generally need to protect against XSS in literal strings within PHP code.

However, if we want to support plugin definition properties being optionally translatable rather than required translatable, here's an alternate way that we can express #50 that I think addresses all the concerns in #56:

#[Block(
  id: "test_context_aware",
  admin_label: [TranslatableMarkup::class, ["Test context-aware block"]],
  context_definitions: [
    'user' => [EntityContextDefinition::class, [
      'data_type' => 'entity:user',
      'label' => [TranslatableMarkup::class, ["User Context"]],
      'required' => FALSE,
      'constraints' => [
        "NotNull" => [],
      ]
    ]],
  ]
)]

I admit it's kludgy. But if #3173787: [policy] Require PHP 8.1 for Drupal 10.0 if a dependency does, otherwise require PHP 8.0. (up from 7.3 in Drupal 9) decides that supporting PHP 8.0 has a high enough value, is this kludge acceptable?

I get the impression that we have two options: We do it all in D10 or we do it all in D11. We do not want to do half in D10 and the rest in D11, because it would be back DX. My question is how back is it to wait and do it all in D11. Is that really unacceptable for some reason or is it not that bad? Just asking the question.

I think that #57 is more like doing 90% of it in D10 and the remaining 10% in D11, and where most of that remaining 10% can be automated with a Rector rule (I think changing the attributes could be automated, but changing the attribute class constructors will be manual).

As far as how bad is it to wait till D11 to start, #53 suggests it might not be too bad.

Off the top of my head here are major projects that have started moving code to Attributes: Symfony Console, Symfony Routing, PHPUnit, Doctrine, Laravel. Lets please be among the leaders here, instead of a late adopter. Drupal already has the reputation of a tired old man. Its important that we combat that by adopting PHP features that actually benefit us like this one. Its important for developers and for evaluators.

That's the spirit I like!

#60 resonates with me, I think we should definitely aim to support discovering php attributes as soon as possible.
Even if we add the php 8.1 way from the start and just have core not use it until 8.1 becomes the minimum.
So to say: you can start using attribute annotations in custom code starting with 10.0.0 (or why not 9.4.x?) but core will not do so until 8.1 is the minimum version of php. I agree that there could be contrib modules that start using it ie the argument from #55. But that is the case regardless of the issue at hand, a contrib module may want to use fibers or other other 8.1 language features. I think a lot of contrib modules rather do the opposite and remove deprecated code very late in order not to drop support for already EOL versions of Drupal core. So I think as long as we clearly communicate that while attributes are a thing now contrib should only update when core requires php 8.1 we should be fine.

I could get on board with the suggestion from #57 if it was the only way, but I think it would be quite a DX issue, especially becase this variant would just be for a transition since Drupal 11 will most definitely reqire php 8.1 I think, and we would have to keep it around until Drupal 12.

So if it was up to me, I would add support for the php 8.1 version as soon as possible and start converting core only later, I don't know if we could make the discovery somehow smart enough (ie you can not mix doctrine and attributes in the same module) to make it discover things just once.
But I don't think it is worth it to add a variant that would work with php 8.0 when maybe soon afterwards 8.1 becomes a requirement anyway.

Trying to summarise the options:

1. Provide PHP 8.1 and 8.0 syntax in 10.0.x
Pros: allows us to drop annotations in Drupal 11, allows modules to use the nice DX version now, allows us to stay on PHP 8.0 (may not be a pro for everybody)
Cons: two things to support, harder to document, hard rector upgrade from 8.0 to 8.1 compatible version.

2. Provide 8.1 attribute support in Drupal 10, with a PHP 8.1 requirement
Pros: one nice clean API change and deprecation, we can drop annotations in Drupal 11
Cons: we have to require PHP 8.1 asap (may not be a con for everybody)

3. Provide 8.1 attribute support in Drupal 10, with a PHP 8.0 requirement
Pros: one nice clean API change and deprecation
Cons: can't use it in core until Drupal 11, can't drop annotations until Drupal 12.

I think the following two are more or less ruled out already, but adding for completeness:

4. Provide PHP 8.0 attribute syntax in Drupal 10, PHP 8.1 attribute syntax in Drupal 11.
Pros: allows us to drop annotations in Drupal 11.
Cons: API change twice, DX issues, hard to rector the second change, we still need to support both at the same time at some point.

5. Wait until Drupal 11, provide PHP 8.1 attribute support then
Pros: no double API change and tricky rector rules
Cons: delays the change for two years, can't remove annotations until Drupal 12.

Personally I like #2 the best of these options. #1 is much more complex for core, #3 increases the likelihood of hitting Doctrine\Annotations EOL, six of one, half a dozen of the other, but could live with both probably.

There is also the 'require PHP 8.1 later in the 10.x cycle and do the conversion then', but it's been extremely difficult to actually increase minimum PHP version in Drupal 8, so that feels like we'd risk pushing things to Drupal 11 (and again, why not just do it up front so expectations are clear).

@bircher a 9.4.x backport of the discovery isn't out of the question, but we can't backport anything to 9.4.x until it lands in 10.x. The advantage of doing that that though, would be that modules could support 9.4 and 11 at the same time if they want.

Exploring that last unnumbered option a bit more:

Release 10.0.x (and maybe 9.4.x) with support for PHP 8.1-only attribute syntax. Do not deprecate annotations yet. Minimum PHP requirement of PHP 8.0, recommended 8.1. Pre-announce that Drupal 10.3.x will require PHP 8.1.

10.3.x is likely to be released in December 2023, or maybe mid-2024 depending on when 10.0.x comes out, this is after PHP 8.0 goes out of official security support.

Then, in 10.3.x or 10.4.x, we can convert core to attributes, and deprecate annotations for removal in 11.0.x.

Advantages:
Only one syntax. Available to contrib and custom code asap. Can drop annotations in Drupal 11.

Disadvantages: slightly more detailed PHP minimum version requirement to communicate.

Yes! #64 very eloquently puts into words what I had in mind but couldn't formulate.

The benefit of this approach is that if #3173787: [policy] Require PHP 8.1 for Drupal 10.0 if a dependency does, otherwise require PHP 8.0. (up from 7.3 in Drupal 9) comes along and makes this discussion mute it will smoothly transform into option 2 from #63. And if somehow we can not gather other reasons as per #3223435: Track PHP 8.1 support in hosting and distributions then it smoothly transforms into option 3 from #63.

I think this is a much better approach than the extra effort for core to support a more complex system with 8.0 attributes which is also not ideal for module maintainers.

So in a summary I would implement the 8.1 variant and decide when to deprecate doctrine annotations later. (ie plan for what #64 suggests)

So as I see
- everyone agrees that we should use PHP attributes instead of doctrine annotations
- it will be allowed to use PHP attributes with Drupal 10.0
- but Drupal 10.0 core will not use PHP attributes

I just don't understand why is this a good solution... maybe it's just me...

RE #66: I will be happy to explain how I understand it.

It is all about compromises and trade-offs.

The attributes language feature was added to php to do the kind of things we (and doctrine ORM) are using doctrine annotations for. It is therefore reasonable that doctrine annotations will be deprecated in the future. When that will be nobody knows but catch explored and reasoned about it in #53 to know when that might be. So yes I think it is save to assume everyone agrees to use attributes instead.

However, the allowed syntax for attributes was expanded in php 8.1 compared to 8.0. And the kind of nesting we need and do with annotations is possible relatively easily with php 8.1 but not with php 8.0. For php 8.0 we would have to resort to nesting things with arrays as demonstrated in #57. So the syntax allowed in php 8.1 is a much better fit for what we need. Using the syntax which works for php 8.0 is much more work and requires contrib and custom modules to update twice or skip the 8.0 version and go straight to the 8.1 version (what I would do) which then doesn't justify the trouble to do it in the first place.

Now we have the solutions outlined in #63.

If we would want to use attributes in Drupal 10.0 we would have to make the minimum required php version 8.1. Me personally would not be against that, but there is a good amount of people who would not be happy about requiring such a new php version because of what it means for where Drupal can be hosted and I respect that. This is a whole other discussion with pros and cons and compromises and trade-offs.

The suggestion that attributes should be supported but not required (and not allowed by the minimum php version of Drupal) means that core and contributed modules can not use it until the minimum php version is raised, but custom modules can go ahead and use the new syntax today so that they don't have to be re-written when Drupal 11 or 12 comes (apart from of course the other deprecations which will get added). You can think of it in the same way as all other php language features, for example in your custom code you could use match with Drupal 8 and 9, but you couldn't do that in a contrib module or core because it wouldn't work with php 7.

Also lets not forget that you can already see this practice in action today! You are most likely familiar with Drush. In Drush 11 you can use attributes to declare a command, but Drush 11 itself doesn't use attributes for its commands because it supports running on php 7 together with Drupal 9. A future version of Drush will switch its core commands to attributes and deprecate the annotation syntax, and later in a higher version remove support for the annotation syntax altogether.

So in essence we allow people to use the new syntax early, but don't require php 8.1 just because of it. So people with php 8.0 hosting can be happy and developers on php 8.1 can be happy.

Re #67

But isn't core is the "source of truth" for many newbie developers, like myself? I think it's one of the popular "lessons" to look things up in core for examples on how to implement things properly (and copy-paste chunks of code to adapt later). And thus the majority of custom modules would be using the 8.0 version of syntax anyways, until there are plenty of examples in core/popular contrib.

RE #68: Yes that is an excellent reason for why we should not be introducing the php 8.0 annotation version!

Whit the proposal I am supporting (ie #64) you would look at Drupal core 10.0 and you would see the same old doctrine annotations we had in Drupal 8. And you would not be wrong to use them in your custom or contrib code. Then if you look at Drupal 10.3 (or at least Drupal 11) You would see the shiny "new" php 8.1 attributes. And you would still have some time to migrate (maybe with automated tools aka rector) to the new syntax before your code doesn't work any more.

I think it is important to allow the new syntax as soon as possible and start using it as soon as possible. Though when that is possible could be different for the two cases. For example we could backport the attribute syntax discovery to Drupal 9, but we couldn't switch Drupal 9 to use the attribute syntax.

I guess there are three potential problems we would introduce with the different approaches, and we have to decide which is worst:

1. Disruption, hassle, and security risks of forcing sites to upgrade to 8.1 in two years (like we did with PHP 5.5 and 5.6, with all the work that took and all the drama from the developer community about putting site owners' needs first for a smooth transition).
2. Risk of sites not adopting D10 in a timely fashion during D9's LTS (or at all) because our platform requirement for PHP is too ambitious.
3. Suboptimal DX in D10 and a messy/fragile upgrade path to D11.

For me, #3 is still the least bad because it only affects developers and core/contrib maintainers. #1 and #2 are big disruptions for the entire ecosystem.

I updated / rewrote much of the issue summary to clarify #57 and list out the options as I understand them.

typo fix

Issue summary updates look good, and #57 looks a lot more viable than #50.

I tweaked the Options 2 and 3 to add a couple of details, and clarify a bit more the differences and similarities.

Fixed one of the code blocks in the IS.

For me it would be option #1. I am worried, about all the D9 sites that will need to migrate to D10 before November 2023, when D9 goes EOL. With that said I think that the other main benefit of PHP 8.1, which to me is fibers, is also very important to have in D10 instead of having to wait for D11. I would be great when there would be a core initiative with ReactPHP to make Drupal more like node.js.

My second option would be #4.

Let's say that for example there are 100,000 or more sites that want to migrate/rebuild from Drupal 7 prior to its November EOL: however many of these don't have access to PHP 8.1 will be forced into launching their new site on Drupal 9 and then having to do an additional upgrade to Drupal 10 less than a year later (when Drupal 9 goes EOL in november 2023), rather than being able to launch on Drupal 10 and avoid the 2nd upgrade.

I would say it is highly speculative that a D7 site which wants to migrate (to D10) between June 15, 2022 and November 28, 2022 that their main issue will be a missing PHP 8.1 hosting. If really that's the case, then I would recommend to migrate to D9, as solving PHP 8.1 hosting is much easier than solving site issues with the newly released Drupal 10.

One more consideration to dispel one of the selling points of the php 8.0 attributes syntax.
How I would use it in my different Drupal roles:

1. As an author of custom code for specific projects: I would never use the php 8.0 syntax, in fact I am already today enjoying php 8.1 language features such as readonly properties and enums. (But of course I know not everyone has this privilege)
2. As a lazy contrib maintainer: I would never use the php 8.0 syntax, doctrine will be supported in D10 still in any scenario so I will switch away from them only when Drupal 9 is EOL (or if attributes get backported to 9.4, when Drupal 11 comes out) at that point I would release a new major version with the php 8.1 requirement, which will not be problematic because it will be for D11. This will coincide with the date php 8.1 itself will slowly go out of official support. (But of course I am aware of this issue and the discussion here, Others may not be so lucky see #68)
3. As a sporadic core contributor: I am not often involved with issues where I would create a new class with new annotations, so most often I would just change existing ones and in case I would forget to use the different syntax then the CI would remind me.

So the php 8.0 syntax would only affect me if my colleagues or me copy paste things from core, and in those cases it would be just as with doctrine annotations and they would eventually be cleaned up because of phpstan deprecation messages or upgrade status checks before upgrading.

@bircher so I think #77 is very reasonable, the main advantage of the PHP 8.0 syntax is that core can use it, so that we can drop annotations in Drupal 11, instead of Drupal 12. If we have PHP 8.0 minimum and PHP 8.1 syntax, core can't convert to attributes until Drupal 11, which means we can't fully deprecate annotations in Drupal 10.

> The suggestion that attributes should be supported but not required (and not allowed by the minimum php version of Drupal) means that core and contributed modules can not use it until the minimum php version is raised, but custom modules can go ahead and use the new syntax today so that they don't have to be re-written when Drupal 11 or 12 comes (apart from of course the other deprecations which will get added).

Could we clarify exactly what this mix of core and contrib would mean?

For instance, can Doctrine annotations and PHP attributes co-exist in the same plugin type, like this:

- core defines the Block plugin type
- core's Block plugins use Doctrine annotations
- can a contrib Block plugin use PHP attributes?

If that is not possible, and it is instead just the case that contrib-defined plugin types can use PHP attributes, then I think the halfway step is a lot less interesting, as a lot of contrib code is implementing core-defined structures.

Just a few cents from my side.

I think Drupal should go with requiring 8.1, convert to a a native attributes in D10, deprecate Doctrine in D10 and remove in D11.

Yes, this feels like a bug step, but it's not actually. Most of the existing D9 websites will be upgraded towards the end of support for D9, which is mid/end 2023. Brand new projects also won't be started right away on D10 in mid 2022, just because it's too new and contrib must follow. Realistically, true D10 adoption will start by end 2022 (and by then PHP8.1 will be over a year old). Also, I doubt PHP version will be the blocker for updating, usually it's budget.

Drupal core sets a standard for contrib, community and (as a project) impacts the entire PHP ecosystem, so moving to 8.1 will be a great move showing the Drupal is up to date with the tech and not falling behind.

Can we take this opportunity to improve DX, while also keeping attributes compatible with PHP 8.0? So far I haven't seen any other project proposing the extremely complex attributes that we are suggesting - we are attempting a 1:1 migration from our existing docblock metadata. For example, PHPUnit isn't proposing a single Test attribute where they put all the flags of the test inside one constructor; instead they have multiple attributes that are all optional.

So, could we do something like this?

#[Block('test_context_aware')]
#[Block\AdminLabel('Test context-aware block')]
#[Block\ContextDefinitions([
  'user' => [
    'data_type' => 'entity:user',
    'label' => "User Context",
    'required' => FALSE,
    'constraints' => [
      "NotNull" => [],
    ]
  ),
])]

This avoids constructors-of-doom (imagine what a ContentEntityType annotation will look like) and also removes the need for developers to even think about the translatability of their admin labels; we just assume that all admin labels must be translated and make POTX do this by default.

This also allows for sharing of some attributes across different types, which might be useful.

Further, this also allows for some more flexibility where contrib might want to add more metadata, as they just add a new annotation; with the existing proposal I am not sure how contrib (or even an optional core module) would be able to add something if they needed to - would we need separate annotations per module? Or would we have a variadic constructor where any remaining arguments are stuffed into an array?

I even think it would be possible to do something like this where POTX can itself use attributes to determine which attributes are translatable:

#[Translatable]
class Block\AdminLabel {

  // or maybe here?
  #[Translatable]
  public TranslatableMarkup $label;

  public function __construct($label) {
    $this->label = new TranslatableMarkup($label);
  }
}

ie. we put attributes on the attributes which POTX can then use to determine what attributes are translatable.

OK so how about considering that "required" metadata is on the main annotation, then "optional" metadata is on additional annotations?

#[Block(
  id: 'test_context_aware',
  admin_label: 'Test context-aware block',
)]
#[Block\ContextDefinitions([
  'user' => [
    'data_type' => 'entity:user',
    'label' => "User Context",
    'required' => FALSE,
    'constraints' => [
      "NotNull" => [],
    ]
  ),
])]

I just learned that even parameters can have attributes so if we can do something like this to inform POTX that an attribute is translatable, we remove cognitive load from developers - unless you are developing e.g. a plugin type yourself you don't even have to think about the admin label of your block needs translating:

class Block {
  public TranslatableMarkup $admin_label;

  public function __construct(
    public string $id,
    #[Translatable]
    string $admin_label
  ) {
    $this->admin_label = new TranslatableMarkup($admin_label);
  }
}

Re PluralTranslation I am beginning to think Symfony made the better decision here combining the two into a single string, e.g. "There is one apple|There are %count% apples".

I added an option 5, which would remove the double conversion concern, but it requires bumping to PHP 8.1 in Drupal 10.2 (rather than 10.3) in order to give contrib modules that want to support all of Drupal's supported versions time in which to do the conversion prior to Drupal 11.

We'll also be training people to pass variables into the first variable of TranslatableMarkup on quite a large scale. This is normally a big security no-no (in fact there's a coder rule specifically for this).

Not sure about this when it's a literal in the plugin declarations, we do the same with YAML.

#88 was an x-post that reverted #87, so adding option 5 to the issue summary again.

Minor grammar tweak.

typo fix

> if we've happy to lose the ability to not mark a block admin label as translatable.

Is there a real world use case for this ability? Are there any cases when we accept both string or TranslatableMarkup, that wouldn't be better off if all strings were automatically captured by POTX and created as TranslatableMarkup?

> We're constructing a real \Drupal\Core\Plugin\Context\ContextDefinition object - not an annotation or attribute.

Can we allow multiple attributes, and construct them like this?

#[ContextDefinition(
  id: 'user',
  data_type: 'entity:user',
  label: "User Context",
  required: FALSE,
  constraints: [
    "NotNull" => [],
  ]
)]

ie. the definition itself becomes the attribute object? Then it just needs a little bit of glue in the discovery to extract the IDs and build an array of ContextDefinitions?

#92 would mean that:

 * @SectionStorage(
 *   id = "overrides",
 *   weight = -20,
 *   handles_permission_check = TRUE,
 *   context_definitions = {
 *     "entity" = @ContextDefinition("entity", constraints = {
 *       "EntityHasField" = \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::FIELD_NAME,
 *     }),
 *     "view_mode" = @ContextDefinition("string", default_value = "default"),
 *   }
 * )

would become:

#[SectionStorage(
  id: 'overrides',
  weight: -20,
  handles_permission_check: TRUE
)]
#[ContextDefinition(
  id: 'entity',
  data_type: 'entity',
  constraints: [
    "EntityHasField" => OverridesSectionStorage::FIELD_NAME,
  ]
)]
#[ContextDefinition(
  id: 'view_mode',
  data_type: 'string',
  default_value: 'default'
)]

That looks pretty ok to me. How would we convert @PluralTranslation?

> How would we convert @PluralTranslation?

We could adopt Symfony syntax, or we could just use arrays? e.g.

#[ContentEntityType(
   ...
   label_count: '@count custom block|@count custom blocks',
)]

or

#[ContentEntityType(
   ...
   label_count: ['@count custom block', '@count custom blocks']
)]

edit: or as it seems fairly rare, and is currently broken, just deal with it explicitly? We already have label_singular and label_plural separate - do we need it anywhere other than ContentEntityType?

#[ContentEntityType(
   ...
  label_singular: 'custom block',
  label_plural: 'custom blocks',
  label_count_singular: '@count custom block',
  label_count_plural: '@count custom blocks',
)]

#92 looks nice - good to see less indentation.

I added an option 6 to the issue summary for the suggestion in #92.

Adding one more disadvantage for option 6.

Maybe another idea worth exploring:

php 8.1 syntax in D10 with php 8.0 minimum requirement.
Add attributes as we go but leave the annotation in place.
The discovery code which gets attributes and annotation data, instead of merging will throw an exception if the two are not equivalent asserting that nobody can create disparity between the two. (Obviously if the code is running on php 8.0 then the attribute discovery would just not run at all)

This would mean duplicated data in Drupal 10, especially on entity types etc this would be quite verbose.
But on the other hand we could deprecate doctrine in D10 and people who look at the code would see examples of how to convert everywhere they look.

Re #98: Argh, I forgot about translation contexts. We are getting into ugly Drupalisms here but if we are OK with manually handling PluralTranslation maybe we can get away with:

#[ContentEntityType(
   ...
  label_singular: ['View', 'View entity type'],
  label_plural: ['Views', 'View entity type'],
  label_count_singular: ['@count view', 'View entity type'],
  label_count_plural: ['@count views', 'View entity type'],
)]

We could also allow TranslatableMarkup for each of these so PHP 8.1 users can use that directly, and eventually migrate away from this syntax when we have minimum 8.1?

> php 8.1 syntax in D10 with php 8.0 minimum requirement.

Unfortunately PHP 8.1 nested attributes cause a fatal error in PHP 8.0: https://3v4l.org/buKfZ

Fatal error: Constant expression contains invalid operations in /in/buKfZ on line 7

> This is likely to mean that we'll need something is block discovery to do this - like each specific case will need to know what attributes to discover and how to munge them into a definition.

IMO this can be done generically in AttributeClassDiscovery. Add an extra parameter to the constructor alongside the main class attribute (e.g. Drupal\Core\Block\Attribute\Block) that declares an array of something like ['context_definition' => ContextDefinition::class] - this inform the discovery of further attributes to collect and assign to a property of the main attribute.

Other than context definitions are there any other cases where we have complex nesting in the metadata and would need to do something like this? In one way we're trying to solve this for all possible cases of metadata, but realistically if nested attributes plus PluralTranslation and translation contexts are rarely used, can we sidestep the generic case and handle these as special cases so we can support PHP 8.0?

Re constructors-of-doom I still think multiple attributes is the way to go for optional arguments especially when we currently have an array.

 *   handlers = {
 *     "storage" = "Drupal\Core\Entity\Sql\SqlContentEntityStorage",
 *     "access" = "Drupal\block_content\BlockContentAccessControlHandler",
 *     "list_builder" = "Drupal\block_content\BlockContentListBuilder",
 *     "view_builder" = "Drupal\block_content\BlockContentViewBuilder",
 *     "views_data" = "Drupal\block_content\BlockContentViewsData",
 *     "form" = {
 *       "add" = "Drupal\block_content\BlockContentForm",
 *       "edit" = "Drupal\block_content\BlockContentForm",
 *       "delete" = "Drupal\block_content\Form\BlockContentDeleteForm",
 *       "default" = "Drupal\block_content\BlockContentForm"
 *     },
 *     "translation" = "Drupal\block_content\BlockContentTranslationHandler"
 *   },

Do we just map this to a single handlers key in a constructor? How do we then document this? Is it better DX to declare this as something like

#[EntityHandler('storage', SqlContentEntityStorage::class)]
#[EntityHandler('access', BlockContentAccessControlHandler::class)]
#[EntityHandler('list_builder', BlockContentListBuilder::class)]
#[EntityHandler('form.add', BlockContentForm::class)]
#[EntityHandler('form.edit', BlockContentForm::class)]

etc?

We agreed today that PHP 8.1 will be required by Drupal 10 (for reasons outside this issue), so we can go ahead with a PHP 8.1-dependent solution here if we want. Thanks everyone for the thoughtful discussion!

#3173787: [policy] Require PHP 8.1 for Drupal 10.0 if a dependency does, otherwise require PHP 8.0. (up from 7.3 in Drupal 9) documents this decision and #3264819: Require PHP 8.1 for Drupal 10.0.0-alpha2 is implementing it.

We should update the issue summary to remove the PHP 8.0 workarounds. I think we still have the question of whether we do a 1-1 port of the current structures, or start moving to multiple attributes to reduce nesting.

Maybe split this into this issue where naming of attributes happening and re-title #3086628: Adopt \Doctrine\Common\Annotations\AnnotationReader and deprecate SimpleAnnotationReader forked from Doctrine to add reader and deprecate old one?

I bet this issue will need some meta to discuss each kind of plugins

As I got there's only option 1 makes sense nowadays

Filed child issues
- #3265942: Extract TranslatableMarkup() from PHP attributes
- #3265945: Deprecate plugins using annotations and plugin types not supporting attributes Assigned to: berdir
- #3265946: Provide automatic conversion from annotations to native attributes

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()

I think this issue supposed to be a meta or plan?

Is it doable for 9.5?

@andypost we can't require PHP 8.1 in Drupal 9.5. It would be theoretically possible to add attributes support that would only work on PHP 8.1 to 9.5.x, but it would not allow us to convert any of core, so would then only exist to encourage contrib to start using it, but this means we wouldn't have it tested against all the core implementations first and it wouldn't allow us to deprecate annotations any earlier.

So the easiest approach here is to add this as a new API in 10.1.x and try to deprecate all use of annotations for removal in Drupal 11.

While it is possible that Doctrine will deprecate annotations in the meantime, they rely on it extensively for Doctrine ORM and only just added full attributes support for that, so overall I think it will be challenging for them to completely drop support for it on a short timeline. But that's a good reason to try to finish this in Drupal 10 so that we don't have that dependency in Drupal 11.

Has anyone taken a look at the viability of using something like https://github.com/koriym/Koriym.Attributes as an in between?

A reader can read both doctrine/annotations and PHP8 attributes with a doctrine annotation interface.

This project has a minimum PHP version of 7.2

I understand Drupal has its own customisations of reader, so could this be used as inspiration/forked?

Interesting statistics about top-1000 packages and attributes usage https://twitter.com/brendt_gd/status/1527615035782180867

Updated the IS to remove the PHP 8.0 options as we now require PHP 8.1, and bumped to 10.1.x as per #110.

Hey, I'm working on a contrib module "geolocation" and specifically a version 4 only intended for Drupal 10+.

Should reworking any current-style attributes to php-core-style attributes be already on my task-list?

Not yet, it is unlikely this will be implemented until at least Drupal 10.1.x and existing docblock annotations will be supported at least for all of Drupal 10.

Merged in 10.0.x and now it's failing on PHPstan issues.

 ------ --------------------------------------------------------------- 
  Line   core/lib/Drupal/Component/Attribute/Plugin.php                 
 ------ --------------------------------------------------------------- 
  16     Abstract class Drupal\Component\Attribute\Plugin cannot be an  
         Attribute class.                                               
 ------ --------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------- 
  Line   core/lib/Drupal/Core/Plugin/Discovery/AttributeDiscoveryWithAnnotatio  
         ns.php                                                                 
 ------ ----------------------------------------------------------------------- 
  57     Access to an undefined property                                        
         Drupal\Core\Plugin\Discovery\AttributeDiscoveryWithAnnotations::$read  
         er.                                                                    
 ------ ----------------------------------------------------------------------- 

Filed another issue to discus new attribute from PHP 8.2 #3296293: Apply SensitiveParameter attribute

Per #20, this MR has a namespace clash with #2664570: Move Attribute classes under Drupal\Component. Wouldn't it make more sense to name these PhpAttribute instead?

Like \Drupal\Component\PhpAttribute\PhpAttributeBase etc.

And, this sounds like a feature request (and therefore non-Critical) at this stage.

Drupal\Component\PhpAttribute
- Php81Attribute
- Php82Attribute
Also here could be
- Symfony https://github.com/symfony/polyfill/blob/v1.26.0/composer.json#L34
- PhpStorm https://github.com/phpstan/php-8-stubs/blob/main/composer.json#L6
It could use collector with kinda applies() method or even autoload/preload script

Then it could be exposed to core via context system

Filed issue to add first attributes for core's codebase #3299853: Apply #[\AllowDynamicProperties] attribute to base classes to make PHP 8.2 log size sane

PhpAttribute sounds like a good idea so it's easy to see it's not HTML attributes.

@andypost I don't think we'd need a different syntax for plugin discovery between PHP 8.1 and PHP 8.2 would we?

No need, the only different is 8.0 vs 8.1 - attributes became multiline

Added #2011038: Use Context system for actions as child issue as it would be great to make conversion and API change same time

Sometime we could add attribute for "changes state" for #2030291: Allow actions to postpone saving the modified entity

Merged in 10.1.x to the MR but Gitlab is having issues showing it. Apparently testbot might pick it up, so setting to NR to see if that triggers drupalci.

I will work a bit on this to update to PHP 8.1 and for #123.

The changes required for the validation constraints, to avoid deprecation errors, tell me that there's no test coverage for them...

Bumping priority and changing to a task for this on the basis that Doctrine is going to drop annotations support in doctrine/orm 3.0, it's not clear (to me at least) what the release date of doctrine/orm 3 and the EOL of Doctrine 2 will be, but the sooner we switch over to attributes, the less we need to worry about that. https://www.doctrine-project.org/2022/11/04/annotations-to-attributes.html

Rebased and went through the code changes.

They look good to me and it seems to me that all concerns posted in this issue where addressed.

I would RTBC this, except for this remark by @mondrake in #128:

The changes required for the validation constraints, to avoid deprecation errors, tell me that there's no test coverage for them...

.

I think this should be addressed first before we can RTBC?

Is this being considered for 10.1.x ? This will be hugely disruptive and will break backwards compatibility. A contributed module that defines plugins will not be able to work in both 10.0.x and 10.1.x. For example, the patch for Rules will be almost as big as this patch for core, and I will have to create and maintain separate branches to work in both 10.0.x and 10.1.x. Or I guess we could keep the annotation and put duplicate data in the attributes as a transitional step, but that's cumbersome and prone to error when the duplicates get out of step with each other. Regardless, this introduces a LOT of work that will be required to support the new minor point release.

I would prefer if this was done as part of the major version transition to Drupal 11, rather than making a BC break between the 10.0.x and 10.1.x minor point releases.

Annotations are only being deprecated, you can still use them until D11. This needs a change record. Also needs an IS update for API changes and Release notes snippet. Setting to NW for these.

In #82 to #105 we couldn't decide whether to port the existing annotations directly to attributes with 1:1 mapping, or rework them as we go.

Thinking about this again, in order to achieve this in a manageable time, I think we should now go with a 1:1 mapping, and then we can optionally rework anything that turns out really awkward via deprecations in individual issues. Otherwise, I think we risk too much time bikeshedding the different options - we should aim to complete this early in the 10.x cycle rather than later, as this is quite a big change that touches a lot of contrib and custom code.

Added some review comments.

Thinking about this again, in order to achieve this in a manageable time, I think we should now go with a 1:1 mapping, and then we can optionally rework anything that turns out really awkward via deprecations in individual issues. Otherwise, I think we risk too much time bikeshedding the different options - we should aim to complete this early in the 10.x cycle rather than later, as this is quite a big change that touches a lot of contrib and custom code.

It seems like with a 1:1 mapping most of these changes could be scripted. Has anyone written that yet, and could a script be provided as part of this change?

I notice that some of this requires PHP 8.1, which is fine for D10 but it means that making these changes in contrib requires breaking compatibility with D9 (which may be running PHP < 8.1). It's not as simple as the attributes will be ignored in lower versions of PHP because there are language-specific things in this change like readonly properties which can't be ignored in lower versions. So again this concerns me because now I'll have to have a D9 branch and a separate D10 branch, which is one of the things I thought we were trying to avoid with semantic versioning.

@TR see #3265946: Provide automatic conversion from annotations to native attributes for automatic conversion and #111 for an option on polyfilling attributes to PHP 8.0. FWIW PHP 8.0 EOL is Nov 2023 if that helps to ease the pain.

One thing that was unclear to me (DX) while working on the MR was that it looks like we have two different ways to namespace the concrete attribute:

Action sits under namespace Drupal\Core\PhpAttribute; i.e. we have a collection of plugin attributes under a single directory

Block sits under namespace Drupal\Core\Block\PhpAttribute; i.e. we have the plugin attribute in a directory connected eith the plugin

IMHO this should be uniformed either in one way or the other. My preference would be for the second.

I think this is the same comment as @longwave's inline the MR.

@mstrelan @TR Drupal 9 EOL is also November 2023, so if you want to support both in contrib you can simply do nothing until after that date, as annotations will still work throughout Drupal 10's lifetime.

@longwave

Thinking about this again, in order to achieve this in a manageable time, I think we should now go with a 1:1 mapping, and then we can optionally rework anything that turns out really awkward via deprecations in individual issues.

100% agreed with this, we can open a new plan issue for spltiting into multiple attributes and pick one or two subsystems to try it out, but it's definitely too much to take on here. It'll also be easier to review if we're doing 1 attribute to x attributes instead of 1 annotation to x attributes.

@TR semantic versioning means we can introduce new APIs and deprecations in minor releases, and they can be ignored by contrib until preparing for the next major release. If we add this in Drupal 10.1, then a good time for contrib to adopt it will be when both 9.5 and 10.0 are out of support (i.e. after December this year) - since then all supported core branches will support attributes, so contrib modules can do a 1-1 switch and avoid having both at once. If we waited to add it in a later minor version, then the window for updating before Drupal 11 requires attributes just gets tighter for everyone.

@mondrake re #139:

I think the current layout in the MR is a historical accident. For core annotations we have both \Drupal\Core\Annotation and \Drupal\Core\[subsystem]\Annotation - Action in the former, Block is in the latter, so in the MR there has been a 1:1 translation between the two. I agree with your preference for the second version with the subsystem included the namespace.

The following classes live under \Drupal\Core\Annotation, here is how I think they should be relocated for attributes:

Do we need PhpAttribute in all the namespaces or should we just use Attribute? There is a clash with #2664570: Move Attribute classes under Drupal\Component but that is only in the \Drupal\Component\Attribute namespace, and there is no guarantee that issue is even going to land - there is more momentum here, maybe even that one should become \Drupal\Component\HtmlAttribute?

IMHO, do not use Attribute anywhere since it's too generic and prone to confusion. Implement PhpAttribute here, change to implementing HtmlAttribute in the other issue.

@mondrake re #143 I disagree with PhpAttribute because

• we are about php code and Attribute is built-in term (it's not about html/xml or whatever) - having Php* prefix in PHP code is confusing
• Symfony using Attribute in mostly every component
  • https://github.com/symfony/symfony/tree/6.3/src/Symfony/Component/Depend...
  • https://github.com/symfony/symfony/tree/6.3/src/Symfony/Component/EventD...

So it smells like NIH

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.

#2664570: Move Attribute classes under Drupal\Component 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 #3337325: 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" to deal with the new deprecation skip.
- "Cross-fixed" (together with @VladimirAus) the latest suggestions made by @longwave

This looks nice, some thoughts:

* #3265945: Deprecate plugins using annotations and plugin types not supporting attributes

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? And we would still need to figure out backward compatibility, I guess.

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: #3344303: [policy, no patch] Decide on a namespace for PHP Attribute classes.

What's left here except extra tests and CR?

#3344303: [policy, no patch] Decide on a namespace for PHP Attribute classes 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. 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 #3387400: GitlabCI should fetch less from git

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.

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 #3396165: [meta] Convert all core plugin types to attribute discovery as a follow-up and started looking into entity type annotations in #3396166: Convert entity type discovery to PHP attributes

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, #3265945: Deprecate plugins using annotations and plugin types not supporting attributes 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.

> Updating POTX module to work with translatable markup in PHP attributes. #3265942: Extract TranslatableMarkup() from PHP attributes. 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.

Following discussion with @Berdir in Slack, I added 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 #3397051: [PP-1] Write a console command to help converting annotations to attributes for plugins

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, and refactored it to use the Doctrine parser instead of reflection; 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

I suppose one way is to try and parse the file first to see if it uses anything that's not installed... but then we're back to parsing PHP manually :(

The layout builder block problem is an edge case that I think we should solve over in #3255804: Hidden dependency on block_content in layout_builder.

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 actually instantiated; the change here is that discovery loads the plugin class, which did not happen with annotations. 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 now 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 or:

$ ddev drush php
Psy Shell v0.11.9 (PHP 8.1.23 — cli) by Justin Hileman
Drupal 10 (Drupal 11.0-dev)
> try {
     new ReflectionClass('Drupal\layout_builder\Plugin\Block\InlineBlock');
} catch (\Throwable $e) {
    echo 'caught you!';
}
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

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 #3397318: Parse attributes before annotations 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.

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

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 #3396166: Convert entity type discovery to PHP attributes which is a child issue of #3396165: [meta] Convert all core plugin types to attribute discovery.

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; the same goes for action and block plugins which can now use attributes instead, and more core conversions are on their way. 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 #3400121: Allow opt-out of annotation parsing 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.