Convert entity type discovery to PHP attributes

Would this be a good time to do #3048483: Explore splitting entity type definitions into two storage/non-storage related definitions as well?

+1 to only doing 1:1 mappings from annotations to attributes in these conversion issues, otherwise we risk dragging them out when we want to get them all done reasonably quickly. If we want to refactor them later at least attributes will hopefully make that easier.

I'm testing out some automation.

Got dev branch from drupal-rector:

composer require palantirnet/drupal-rector:dev-feature/annotation-configentitytype

Added a rector.php

<?php
declare(strict_types=1);

use Rector\Config\RectorConfig;

return static function(RectorConfig $rectorConfig): void {
  $rectorConfig->ruleWithConfiguration(\DrupalRector\Drupal10\Rector\Deprecation\AnnotationToAttributeRector::class, [

    // Setting remove version to 10.x means the comments are not kept.
    new \DrupalRector\Drupal10\Rector\ValueObject\AnnotationToAttributeConfiguration('10.0.0', '10.0.0', 'ContentEntityType', 'Drupal\Core\Entity\Attribute\ContentEntityType'),
    new \DrupalRector\Drupal10\Rector\ValueObject\AnnotationToAttributeConfiguration('10.0.0', '10.0.0', 'ConfigEntityType', 'Drupal\Core\Entity\Attribute\ConfigEntityType'),
  ]);

  $rectorConfig->autoloadPaths([
    './lib',
    './modules',
    './profiles',
    './themes'
  ]);


  $rectorConfig->skip([
    '*/upgrade_status/tests/modules/*',
    '*/ProxyClass/*',
    '*/tests/fixtures/*',
    '*/vendor/*',
  ]);
  $rectorConfig->fileExtensions([
    'php',
    'module',
    'theme',
    'install',
    'profile',
    'inc',
    'engine'
  ]);
  $rectorConfig->importNames(FALSE, FALSE);
  $rectorConfig->importShortClasses(FALSE);
};

Then ran rector in core (my setup is with joachim's core dev setup):

vendor/bin/rector process repos/drupal/core/

Opening a seperate MR to see how it does.

Php didn't want to run locally.

Phpstan wasn't happy with missing \ for the translatable markup. Manually fixed those.

There might be more needed, but out of time.

Might try later.

> public readonly ?string $field_ui_base_route = NULL,

This doesn't belong in core, it's added by Field UI.

We discussed the problem of 3rd-party annotation properties in the original issue to add attributes to core, but I don't remember how it was decided it would be dealt with.

Can we let modules like Field UI define additional attributes on plugins?

#[Entity()]
#[FieldUIBaseRoute]

> I agree it shouldn't be here, but having multiple attributes is not supported by our parser and I don't see an easy way to introduce that.

Is this going to break BC for modules which put their own top-level properties in entity annotations?

And what about the annotations we've already converted? I thought the general attribute issue was covering this and now it seems it hasn't.

return !($value === NULL && ($key === 'deriver' || $key === 'provider' || $key == 'entity_type_class'));

maybe

return isset($value) || in_array($key, ['deriver', 'provider', 'entity_type_class'], TRUE);

alternatively

return isset($value) || isset(array_flip(['deriver', 'provider', 'entity_type_class'])[$key]);

Per suggestion from @alexpott on Slack, handling the subclass discovery changes in #3427388: Update Drupal\Component\Annotation\Doctrine\StaticReflectionParser::hasClassAttribute() to allow attribute subclasses. So that's the blocker instead of #3420984: Convert Layout DisplayVariant, PageDisplayVariant discovery to attributes.

I'm seeing the constructor for the attributes specify some strings as ?string and others as string. Yet for all but the $id parameter we specify defaults, making them all count as optional. This is why php and phpstan aren't throwing a hissy fit when we put parameters with question marks before those without one.

Shouldn't we be consistent and mark them all as optional then? I just got bitten by this when I tried to copy this MR for Group and had some constructor args without default values, making both phpstan and php 8.1 explode.

Also, this means that you could technically create a new entity type by providing nothing but an entity type ID, right?

Ugh, never mind. I forgot that the question mark means "You can omit this or specifically set this to NULL", whereas omitting the question mark while providing a default value means: "You can omit this or set this to something else, but not NULL"

Sorry, I confused myself.

But I will most likely drop the current trickery anyway and re-define all properties on the child classes because I don't care about phpstan, but I do care about autocomplete support in PHPStorm and that can't handle this either.

Thanks for that, while converting to attributes I loved the autocomplete feature.

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

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

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

Applied my own suggestions and rebased.

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

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

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

I realize this may be out of scope but #2813895: Combine entity type labels in an array has been open for a while and would be a breaking change in any existing system. However the conversion to attributes might be the perfect time to make the grouping since people will have to re-write their annotations anyway (so it's not more or less breaking than what we're already doing).

I only just came across that issue so sorry if this is a bit late in the process.

However the conversion to attributes might be the perfect time to make the grouping since people will have to re-write their annotations anyway (so it's not more or less breaking than what we're already doing).

Per @Berdir in #31, it's probably best for this to land ASAP without additional blockers. Though maybe a compromise solution is to add support to set labels per #2813895: Combine entity type labels in an array now, convert maybe 1 entity type to confirm it works, then do the remainder of the conversion in the other issue? My thinking here would be something like this:

In the constructors of the new attributes, add protected readonly array $labels = [],.

Then in the get() methods of the EntityType attribute:

  $values = array_filter(get_object_vars($this) + [
      'class' => $this->getClass(),
      'provider' => $this->getProvider(),
    ], function ($value, $key) {
      return !($value === NULL && ($key === 'deriver' || $key === 'provider' || $key == 'entity_type_class'));
    }, ARRAY_FILTER_USE_BOTH);

  $label_map = [
    'label' => 'default',
    'label_singular' => 'singular',
    'label_plural' => 'plural',
    'label_count' => 'count' ,
    'bundle_label' => 'bundle',
    'group_label' => 'group',
  ];

  foreach ($label_map as $key => $labels_key) {
    if (isset($value['labels'][$labels_key])) {
      $values[$key] = $values['labels'][$labels_key];
    }
  }

  return new $class($values);

Or maybe do the mapping before the filtering so the property order is maintained.

> It's an existing, documented property, we're not introducing it here. I agree it shouldn't be here, but having multiple attributes is not supported by our parser and I don't see an easy way to introduce that.

What do you mean by our parser?

Native PHP can read multiple attributes from a thing:

$reflection->getAttributes();

> Field UI would then need it's own discovery and cache and it would be an API change to any code looking for that info.

I don't see why we can't have the entity type manager (or indeed any plugin manager) read ALL attributes from plugin classes, and merge the definitions.

Something like this would work?

#[Attribute()]
class PluginExtender {

}


#[Attribute()]
class EntityType {
  public function __construct(
    public readonly string $id,
    // etc...
  ){}
}

#[Attribute()]
#[PluginExtender()]
class FieldableEntityType {
  public function __construct(
    public readonly string $field_admin_route,
  ){}
}

#[EntityType(
  'node',
  // etc
)]
#[FieldableEntityType('myroute')]
class Node {

}

I don't see why we can't have the entity type manager (or indeed any plugin manager) read ALL attributes from plugin classes, and merge the definitions.

Currently attribute discovery only works with one attribute class passed from the plugin manager, though subclasses are allowed. Adapting for multiple attribute classes would be non-trivial.

protected function parseClass(string $class, \SplFileInfo $fileinfo): array {
    // @todo Consider performance improvements over using reflection.
    // @see https://www.drupal.org/project/drupal/issues/3395260.
    $reflection_class = new \ReflectionClass($class);

    $id = $content = NULL;
    if ($attributes = $reflection_class->getAttributes($this->pluginDefinitionAttributeName, \ReflectionAttribute::IS_INSTANCEOF)) {
      /** @var \Drupal\Component\Plugin\Attribute\AttributeInterface $attribute */
      $attribute = $attributes[0]->newInstance();
      $this->prepareAttributeDefinition($attribute, $class);

      $id = $attribute->getId();
      $content = $attribute->get();
    }
    return ['id' => $id, 'content' => $content];
  }

In addition, if the idea is for something like a FieldableEntityType attribute to live in the field_ui module and not core, then when field_ui is not installed, discovery will hit fatal errors trying to instantiate ReflectionClass on a class having an attribute of an unknown class.

It doesn't crash while you're just working with class names:

$reflection_class = new \ReflectionClass(Node::class);

if ($attributes = $reflection_class->getAttributes()) {
  foreach ($attributes as $attribute) {
    $attribute_class = $attribute->getName();

    // Skip attribute classes from uninstalled modules.
    if (!class_exists($attribute_class)) {
      continue;
    }

    $attribute->newInstance();
  }
}

Regarding the $field_ui_base_route discussion: What if we move those to constants (or properties) with attributes on the entity type's class?

We could declare those as Attribute::TARGET_CLASS_CONSTANT and only look for them in classes that are tagged with the EntityType attribute.

E.g.:

#[ContentEntityType(
  id: 'node',
  label: new TranslatableMarkup('Content'),
  label_collection: new TranslatableMarkup('Content'),
  // ... other stuff but not field_ui_base_route
)]
class Node extends EditorialContentEntityBase implements NodeInterface {

  #[FieldUiBaseRoute]
  public const FIELD_UI_BASE_ROUTE = 'entity.node_type.edit_form';

Then we can do away with putting non-core properties on the EntityType attribute and immediately have a way for other modules to declare their own third-party properties on entity type definitions.

This works in AttributeClassDiscovery:

    // Get plugin extension attributes.
    if ($extending_attributes = $reflection_class->getAttributes(PluginExtender::class, \ReflectionAttribute::IS_INSTANCEOF)) {
      foreach ($extending_attributes as $attribute) {
        $attribute_class = $attribute->getName();
        // Attribute classes may come from modules which are not enabled, so
        // skip these.
        if (!class_exists($attribute_class)) {
          continue;
        }

        $attribute_properties = $attribute->getArguments();
        foreach ($attribute_properties as $name => $value) {
          if (property_exists($content, $name)) {
            throw new InvalidPluginDefinitionException("May not reuse $name.");
          }
        }

        // TODO: decide how to add property $name to $content plugin definition.
      }
    }

What needs to be figured out is where we put 3rd party definition data on the EntityType definition object. For array definition plugins we just shove into the array! :D

Proof of concept pushed to branch 3396166-convert-entity-type-extension-annotations.

What needs to be figured out is where we put 3rd party definition data on the EntityType definition object. For array definition plugins we just shove into the array! :D

Nice! Though one additional thing that needs to be solved is to delete the plugin class data from APCU file cache when modules are installed or uninstalled.

That being said, I am +1 with @berdir and @longwave to getting this in as near 1:1 as possible so we can move the meta #3396165: [meta] Convert all core plugin types to attribute discovery forward and close in on deprecation #3265945: Deprecate plugins using annotations and plugin types not supporting attributes.

> Nice! Though one additional thing that needs to be solved is to delete the plugin class data from APCU file cache when modules are installed or uninstalled.

That's probably just a call to opcache_reset() in the extension system?

Though even without that, I don't think it matters:

- you uninstall module Foo
- plugins are re-discovered. Because the Foo attribute class is still in memory cache from the last discovery, Foo module's attributes are parsed and stored in the plugin discovery cache. But nothing is going to read them anyway.
- when opcache is eventually cleared, a subsequent re-discovery won't read the Foo attributes

I'm torn between making incremental improvements, and getting this right with attributes rather than carrying over a mess from the annotation system.

It also opens up lots of possibilities for plugins in general - there's a long-standing issue to get Views data handling declared on field type plugins, which is exactly the same problem of component and module hierarchy. That would be fixed by this concept too.

That's probably just a call to opcache_reset() in the extension system?

Though even without that, I don't think it matters:

- you uninstall module Foo

It'd probably have to be apcu_clear_cache(). That does reduce some of the advantage of using file cache, but modules probably aren't installed all that frequently.

And the issue is on install (I spent a week and a half chasing down a test failure similar to this for migrate source plugins):

• Module foo has a plugin class of a type defined in core
• Plugin class has a plugin extender attribute defined in module bar
• Module foo is installed, but bar has never been installed
• Plugin discovery picks up the plugin definition without the bar extension attribute values and stores the definition array in file cache
• bar is installed, and plugin discovery picks up the plugin definition data unchanged from file cache

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

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

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

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

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

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

Rebased after tour removal.

I think we should be doing 1-1 conversions in the first pass, then splitting attributes in a follow-up. It will mean another layer of bc for the single-vs-multiple attribute definitions but it also means we can get off annotations quicker once core is fully converted and we start implementing deprecations for contrib. A new issue for multi-attribute-plugin-discovery would be great though.

That sounds like a good roadmap to me.

One small problem is that I'd thought it would be good DX if supplementary attributes were not able to redeclare a property that's in the main plugin attribute:

        $attribute_properties = $attribute->getArguments();
        foreach ($attribute_properties as $name => $value) {
          if (property_exists($content, $name)) {
            throw new InvalidPluginDefinitionException("May not reuse $name.");
          }
        }

If we later on want to move a property like field_ui_base_route to a supplementary attribute, we need a way to make an exception to that rule, for BC handling, because there will be a period when the property is in both the main attribute and the supplementary attribute.

That could fairly easily be done by marking the property with an attribute to say it's expected that it does that, but it's an extra piece of complexity.

Seems to need a manual rebase.

Rebased and resolved merge conflicts.

Swear I reviewed and marked this one yesterday but guess it didn't save :(

As this is approaching the 2 month mark of #needs-review-queue going to give my best shot.

Applied the MR and verified all instances of @ConfigEntityType have been replaced.
Reverted the change to Media.php to make sure annotation still works (it did)
Tried to pull from reviews from other convert tickets and believe the main points have been hit.
Appears all threads in the MR have been resolved so believe this one to be good.

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

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

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

Rebased, back to RTBC.

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

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

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

Rebased again, back to RTBC.

Some test failures after rebasing.

Test fixed. Back to review to confirm changes after rebase.

This needs another rebase I think.

Rebased again.

I pulled this down and ran the same check as @smustgrave.

Everything seems good, and all @ContentEntityType's have been converted.

There are still a few references in comments, should those be cleaned up if the attribute is the way to go. If this should be a follow up then I think this issue is RTBC otherwise.

core.api.php:

To annotate a class as a plugin, add code similar to the following to the
 * end of the documentation block immediately preceding the class declaration:
 * @code
 * * @ContentEntityType(

entity.api.php:

*   \Drupal\Core\Entity\ContentEntityBase, with annotation for
 *   \@ConfigEntityType or \@ContentEntityType in its documentation block.

EntityStorageInterface.php:

 * \Drupal\Core\Config\Entity\ConfigEntityStorage for config entities. Those
 * implementations are used by default when the @ContentEntityType or

Example.php.twig:


* @ContentEntityType(


Think we should address the comments here too.

Will try and keep an eye out for this one so maybe we can land it in 11.1, else believe we would have to wait for 11.2

Updated comments in core/lib/Drupal/Core/Entity/entity.api.php and core/lib/Drupal/Core/Entity/EntityStorageInterface.php.

Example.php.twig

I think this file comes from chi-teck/drupal-code-generator and not in Drupal core?

core.api.php

/**
 * @defgroup annotation Annotations
 * @{
 * Annotations for class discovery and metadata description.
 *
 * The Drupal plugin system has a set of reusable components that developers
 * can use, override, and extend in their modules. Most of the plugins use
 * annotations, which let classes register themselves as plugins and describe
 * their metadata. (Annotations can also be used for other purposes, though
 * at the moment, Drupal only uses them for the plugin system.)
 *
 * To annotate a class as a plugin, add code similar to the following to the
 * end of the documentation block immediately preceding the class declaration:
 * @code
 * * @ContentEntityType(
 * *   id = "comment",
 * *   label = @Translation("Comment"),
 * *   ...
 * *   base_table = "comment"
 * * )
 * @endcode
 *
 * Note that you must use double quotes; single quotes will not work in
 * annotations.
 *
 * Some annotation types, which extend the "@ PluginID" annotation class, have
 * only a single 'id' key in their annotation. For these, it is possible to use
 * a shorthand annotation. For example:
 * @code
 * * @ViewsArea("entity")
 * @endcode
 * in place of
 * @code
 * * @ViewsArea(
 * *   id = "entity"
 * *)
 * @endcode
 *
 * The available annotation classes are listed in this topic, and can be
 * identified when you are looking at the Drupal source code by having
 * "@ Annotation" in their documentation blocks (without the space after @). To
 * find examples of annotation for a particular annotation class, such as
 * EntityType, look for class files that have an @ annotation section using the
 * annotation class.
 *
 * @see plugin_translatable
 * @see plugin_context
 *
 * @}
 */

The documentation here is not specific to entity types, and the three paragraphs+ of whole topic probably need a significant rewrite. I think that should be done in a follow up, if it's not already part of the Attribute conversion meta.

You're right on Example PHP.

I think it makes sense to handle core.api.php documentation in a follow up since it isn't directly about this annotation.

I'll create a follow up issue on the parent meta issue.

I reviewed the other two comment updates and they look great now.

RTBC since it seems ready now!

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

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

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

Rebased, back to RTBC.

Left some suggestions for comments on the MR.

Applied some suggested changes.

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

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

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

Rebased. Put it back to RTBC since that was where it was before the bot detected the merge conflict.

There's a couple of open threads on the MR

Pretty keen to see this in the beta

I believe all previous threads were addressed, but I can't resolve them because I didn't open the MR.

Will take a look at the new threads for the config entity attributes a bit later.

Removed some unneed arguments from ConfigEntityType attribute constructor and rebased.

I think I found some additional ConfigEntityType constructor arguments we don't need. We might want a follow-up to try to rationalise these anyway?

Upstream issue fixed. Rebased and tests are green again.

@catch, @larowlan can you clarify the follow up requests?

I'd love to see constants or enums for these (in a follow up), magic strings lead to typos

Is to use constants/enums for common form keys such "default" and "delete"?

I think I found some additional ConfigEntityType constructor arguments we don't need. We might want a follow-up to try to rationalise these anyway?

Is this to discuss restoring some properties that were removed?

@godotislate no I meant checking if there's anything else like bundle_entity_type to drop. There might not be.

@catch Follow up created: #3487038: Confirm which ConfigEntityType attribute contructor arguments are needed.

Ok I took a look, looks great!
I see no open MR comments other than for the follow ups.

There are two new things to do in a follow up:

1. Update docs around annotations: #3476278: Update *.api.php Annotation documentation
These are in core.api.php entity.api.php and EntityStorageInterface.php that I could find.
2. Trailing comma and bracket in these 10 files larowlan said we could address in a followup: #3487088: Fix trailing comma in entity type Attribute declarations

core/lib/Drupal/Core/Datetime/Entity/DateFormat.php
core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
core/lib/Drupal/Core/Entity/Entity/EntityFormMode.php
core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
core/lib/Drupal/Core/Entity/Entity/EntityViewMode.php
core/modules/config/tests/config_test/src/Entity/ConfigTest.php
core/modules/language/src/Entity/ContentLanguageSettings.php
core/modules/media/src/Entity/Media.php
core/modules/media/src/Entity/MediaType.php
core/modules/menu_link_content/src/Entity/MenuLinkContent.php

Committed to 11.x and backported to 11.1.x, thanks all. See you in the followups.

What was the reason that 'label_count' is moved away from other 'label_*' parameters?

I think such a big change deserves a change record and 11.1 release highlights.

I can't find a change record for this. If an existing one covers also this change, could it be linked?

"I think such a big change deserves a change record"
"I can't find a change record for this. If an existing one covers also this change, could it be linked?"

I agree. No change records mention ContentEntityType, for example, or any other of the types changed (in core) by this issue.

It is unclear to me whether I can use attributes for these types in 10.3 like with most of the other annotation->attribute conversions, or whether using attributes for these types is only supported in 11.1+. Usually that information is found in the change record.

The only change records that talk about annotation->attribute are:
Plugins converted from Annotations to Attributes in 10.3.0
and
Plugin implementations should use PHP attributes instead of annotations
The latter says Drupal 10.2, warns that not everything is done yet, and claims it will be updated with additional plugin types as they are converted in core. That hasn't happened.

Neither of these change records mentions any of the types converted by this issue.

Yes, this was missed. A new CR is needed for the conversions made in 11.1. I can do that, unless someone else has started. No, I don't see an new draft CR, so I will proceed.

The CR is published. It would be good to confirm it is correct.

Title might make it a bit harder to find, but I saw that it follows a pattern from previous CRs, so I guess it's fine.

I see the followup created in #85

I agree that such a big improvement deserves a 11.1 release highlight (as suggested by @chi) but it looks like it wasn't mentioned?
https://www.drupal.org/project/drupal/releases/11.1.0

There's been a regression here:

label_count used to be a PluralTranslation, which enforced the keys:

  public function __construct(array $values) {
    if (!isset($values['singular'])) {
      throw new \InvalidArgumentException('Missing "singular" value in the PluralTranslation annotation');
    }
    if (!isset($values['plural'])) {
      throw new \InvalidArgumentException('Missing "plural" value in the PluralTranslation annotation');
    }

Now it's just an array and there's no check of the keys:

    public readonly array $label_count = [],

Filed #3557963: [Regression] entity type label_count property no longer enforces its sub-properties.