Convert entity type discovery to PHP attributes

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

I'd rather not mix that. This only affects the attribute/discovery class. We then get all values from it and instantiate the definition object form it, those two are completely disconnected apart from sharing the same properties (for the most part). While some options related to that were discussed, the current discovery only supports a single class and a discovered definition currently also needs to be a single array/object thing.

I would expect the BC implications of that issue massive also on the entity type level, this doesn't affect it at all, what you interact with once plugins are discovered is 100% identical to before. Even the build and alter hooks 100% identical to before.

+1 to only doing 1:1 mappings from annotations to attributes in these conversion issues; 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]

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. 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.

The EntityType attributes class both defines an additional key where extra stuff can be put and also supports variadics for extra stuff that is then put it additional what of that exactly makes it in is still to be determined.

> 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'));

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

#18: This only affects the annotation/attribute discovery. alter hooks and resulting definitions work unchanged. That's exactly why we're trying to do this 1:1. It will not break/change until you convert your specific annotation to an attribute and whether or not there will be changed in the structure is still to be defined.

Recent changes to discovery broke the ability to have subclasses. This is being implemented in 📌 Convert Layout DisplayVariant, PageDisplayVariant discovery to attributes Needs review , so blocked on that.

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

One thing I'm wondering is how this MR gets away with not putting the optional arguments last in the attribute constructors. I know that some of it is being covered/hidden by variadics (...$base) and the use of named arguments. However, when I do something similar in Group it complains about the argument order in both phpstan and throws php errors.

https://git.drupalcode.org/project/group/-/merge_requests/147/diffs#813d...

How is phpstan and php not tripping over this MR?

E.g.:

#[\Drupal\Core\Entity\Attribute\ConfigEntityType(id: 'date_format', label: new TranslatableMarkup('Date format'), handlers: ['access' => 'Drupal\system\DateFormatAccessControlHandler'], entity_keys: ['id' => 'id', 'label' => 'label'], admin_permission: 'administer site configuration', list_cache_tags: ['rendered'], config_export: ['id', 'label', 'locked', 'pattern'])]
class DateFormat extends ConfigEntityBase implements DateFormatInterface {

Does not specify config_prefix as seen here:

  public function __construct(
    public readonly ?string $config_prefix = NULL,
    public readonly array $lookup_keys = [],
    public readonly array $config_export = [],
    ...$base
  ) {

That should make phpstan complain and even PHP >= 8.1 crash because you can no longer put optional arguments before required ones.
See: https://www.php.net/manual/en/migration81.incompatible.php#migration81.i...

phpstan is complaining, but it's on a level that core ignores atm. Your configuration might be more strict?

It's not a PHP error exactly because of named arguments, they can be in any order as long as they *are* defined, the phpstan error is just a consistency thing.

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.

* Note: There are tons of test fails because there are undefined named properties, that part is indeed not working, it might work if I implement logic that puts unknown stuff directly in aditional, will need to test. But I'm honestly also fine with just requiring that those need to move inside the explicit additional array. That's what happens after after discovery internally on the EntityType plugin definition class already now anyway (that is a different class than the one we use for parsing the plugin definition)

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.

I suspected already that this would be related to config entity cache tags.

There's an interesting and subtle change here between the way the annotation and attribute work. The entity annotation classes didn't really have any properties defined, we have them on the plugin definition object and were too lazy to do both. So the list_cache_tags property was by default NULL/not defined at all. Now we define it as an array with a default value of []. That means that \Drupal\Core\Entity\Attribute\EntityType::get() no longer filters out out, because an empty array is not NULL.

Then, \Drupal\Core\Config\Entity\ConfigEntityType::__construct() has this strange copypaste snippet from the parent that sets a default value to the $this->list_cache_tags before calling the parent constructor, so that the similar snippet there is skipped. However, no $definition has a list_cache_tags entry set to an empty array and replaces that again.

Fix is pretty easy, we just set the default on $definition['list_cache_tags'] if that is empty, instead of the completely bogus if (empty($this->list_cache_tags)) { which at that point definitely always was empty.

Note sure if there are other subtle differences like this, hopefully not as the plugin definition object should have property defaults for things, lets see if any test fails remain I guess.

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.

godotislate → changed the visibility of the branch 3396166-convert-entity-type to hidden.

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. Rebasing this is pretty tough with the formatting commits, we might want to squash that all together. And just get it in asap.

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 📌 Convert entity type discovery to PHP attributes Needs review , 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])) {
      $value[$key] = $value['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 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.

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 📌 [meta] Convert all core plugin types to attribute discovery Active forward and close in on deprecation [#326594].

> 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 bar has never been installed
• Plugin discovery picks up the plugin definition without the bar extension attribute values and stores in the definition data 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.