Allow attribute-based plugins to discover supplemental attributes from other modules

Pushed the work I did on 📌 Convert entity type discovery to PHP attributes Needs review to a MR.

Still to do:

- > unless there is a deprecation marked.

Bikeshedding: what do we call these?

- I initially went for 'extending plugin attributes' but I think that's misleading because there is no class inheritance going on
- '3rd party' doesn't apply, I feel, because with config 3rd party settings, those properties are completely controlled by another module - they are set, and read. Here, the properties are set by modules that implement plugins, and read by the module providing the extra attribute
- additional plugin attribute?
- supplemental plugin attribute?

- I initially went for 'extending plugin attributes' but I think that's misleading because there is no class inheritance going on

Maybe not extending the plugin attribute, but you are extending the plugin. So maybe just rearranging the words to "plugin extension attributes" or something to that effect.

How about `Non-discovery attributes` ?

I'd go with Third-Party for two reasons:

1. We already have that naming pattern for config
2. While you're right that the implementing module doesn't set the data like they do with field_ui_base_route, it's arguably still a third-party piece of information that more often than not is added via a hook_entity_type_build/alter. The Field UI base route is more the exception to the rule than the rule itself. I wouldn't get too hung up on that.

Thanks for the suggestions!

I've added functionality for allowing 3rd-party (let's go with that!) attributes to override a main attribute property, if that property is marked as deprecated.

This is the mechanism that will allow us to move properties like field_ui_base_route from the main attribute into a 3rd-party attribute.

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.

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.

All tests appear to be failing

Yes but I'd like a review of the approach before I write tests.

Maybe can help/

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.

This issue would make a difference and other modules like Navigation or Dashboard could take advantage of this new feature. Just one question about a possible scenario that I'm not sure whether current implementation would cover.

* Navigation adds a new 3rd party attribute for Block Attribute
* Module X provides multiple feature, one of them is a Navigation block, defining the corresponding Navigation specific block attribute allow_in_navigation
* For any reason, Navigation module is not enabled in the site
* Should the Error: Unknown named parameter $allow_in_navigation in ReflectionAttribute->newInstance()... be triggered?

How this scenario should be handled?

@plopesc I don't think that would happen:

- Navigation module provides an attribute class NavigationBlock.
- MyModule adds a NavigationBlock attribute to its block plugins, in addition to core's Block attribute
- If MyModule is enabled but Navigation is not, the NavigationBlock attribute is simply ignored.

Not sure if that could be a handled in the same issue, but I think that specifically the entity type attribute could really benefit from splitting up the attribute class, to make it more similar to how Doctrine and also for example Drush command attributes:

#[CLI\Command(name: self::GET, aliases: ['cget','config-get'])]
#[CLI\Argument(name: 'config_name', description: 'The config object name, for example <info>system.site</info>.')]
#[CLI\Argument(name: 'key', description: 'The config key, for example <info>page.front</info>. Optional.')]
#[CLI\Option(name: 'source', description: 'The config storage source to read.')]
#[CLI\Option(name: 'include-overridden', description: 'Apply module and settings.php overrides to values.')]
...
    public function get($config_name, $key = '', $options = ['format' => 'yaml', 'source' => 'active', 'include-overridden' => false])

Right now, it looks like this:

#[ContentEntityType(
  id: 'node',
  label: new TranslatableMarkup('Content'),
  label_collection: new TranslatableMarkup('Content'),
  label_singular: new TranslatableMarkup('content item'),
  label_plural: new TranslatableMarkup('content items'),
  entity_keys: [
    'id' => 'nid',
    'revision' => 'vid',
    'bundle' => 'type',
    'label' => 'title',
    'langcode' => 'langcode',
    'uuid' => 'uuid',
    'status' => 'status',
    'published' => 'status',
    'uid' => 'uid',
    'owner' => 'uid',
  ],
  handlers: [
    'storage' => NodeStorage::class,
    'storage_schema' => NodeStorageSchema::class,
    'view_builder' => NodeViewBuilder::class,
    'access' => NodeAccessControlHandler::class,
    'views_data' => NodeViewsData::class,
    'form' => [
      'default' => NodeForm::class,
      'delete' => NodeDeleteForm::class,
      'edit' => NodeForm::class,
      'delete-multiple-confirm' => DeleteMultiple::class,
    ],
    'route_provider' => [
      'html' => NodeRouteProvider::class,
    ],
    'list_builder' => NodeListBuilder::class,
    'translation' => NodeTranslationHandler::class,
  ],
  links: [
    'canonical' => '/node/{node}',
    'delete-form' => '/node/{node}/delete',
    'delete-multiple-form' => '/admin/content/node/delete',
    'edit-form' => '/node/{node}/edit',
    'version-history' => '/node/{node}/revisions',
    'revision' => '/node/{node}/revisions/{node_revision}/view',
    'create' => '/node',
  ],
// continues on and on

We'd split up specifically repeatable but also optional and also but not only third party keys into separate attributes. Haven't really thought about how to merge it together again, since the result still needs to be a single definition.
#[ContentEntityType(
id: 'node',
label: new TranslatableMarkup('Content'),
...
)]
#[Handler('storage', NodeStorage::class)]
// Or maybe even more specific subclasses for common handlers
#[StorageHandler(NodeStorage::class)]
#[Link('canonical', '/node/{node}']
?>

I think that would be a lot closer to how other PHP frameworks deal with this and while it probably be just as long, it would be less complex and easier to format. Pretty sure that our entity type attribute class is going to give any developer familar with Doctrine a heart attack ;)

Big +1 to #18. The hard part with splitting them up will be to figure out which attributes need to be aggregated into the entity type info and which are completely unrelated. It's not unthinkable people would add non-entity type attributes there for whatever reason.

I also plan to one day suggest we turn entity type handlers into services like I did in Group and having these separate attributes would go a long way to keep the declaration validation logic in one place. But that's for a more distant future as the current handler interfaces tend to be quite big.

I think #18 would need a different issue. It's not just the changes to how we assemble data in the discovery class, but also we need a new way for the plugin type manager to say that there are multiple 'official' attributes. And then there'll be tons of bikeshed about how to slice up the entity type attribute...

Oh yeah, definitely a separate issue. But it could be a good idea to explore that avenue first and then return to this issue with its findings and/or commits.

This issue is a contrib blocker though, now I think of it.

Suppose you have a contrib module which invents the 'floopiness' property on core's Block plugins.

If you want to add that to existing plugins, nothing changes -- you use the alter hook.
But if you provided a Block plugin, up until now you could do this in an annotation

   @Block {
   id = "My block"
    floopiness = 42
   }

With attributes, that will crash, and your module needs to implement the alter hook for its own plugins.

> With attributes, that will crash, and your module needs to implement the alter hook for its own plugins.

Which means it's not a blocker, just an inconvenience.

The thing with the third party discovery added here is is that it assumes that the plugin definition is a array-dumping-ground. object based plugin definitions need to explicitly support additional properties too, which entity types actually do, also on the attribute. They have an explicit additional array property where extra stuff can be put into, there are IIRC also a few test entity types that make use of that.

Any plugin type that wants to make it easier to support additional stuff can add that property as well.

Agree that #18 is kind of a different scope, but maybe this issue is just a more specific use case of that, and we if support additional attributes as a concept, based on a common base class/interface and have a mechanism that allows for them to control how the result is merged, then it doesn't really matter if the extra bits are official or not.

And of course, the ability to have that and implementing it for entity types would not be the same issue either, we could have a proof of concept for our test plugin types, just like this issue currently also does.

> The thing with the third party discovery added here is is that it assumes that the plugin definition is a array-dumping-ground.

Yup. I considered having values from 3rd party attributes go into something like a $definition['third-party'][$attribute_provider] array, but I wasn't sure whether that was over-complicating.

Do you think that would be cleaner?

I think it's worth investigating how much overlap there is in a use case like splitting up the entity type attributes and the use case of having other modules provide supplemental attributes.

A few thoughts:

• If we're providing "optional" properties, maybe the base attribute class name can be "PluginOption" instead of "PluginExtender." But this is a bikesheddy nit
• ::get() called on a Plugin attribute does not necessarily return an array; it can also return an object. See CKEditor5Plugin for an example. In which case, we can't assume that the $content variable in AttributeClassDiscovery::parseClass() is an array that new properties can just be appended to. I think we can solve this with a method on the base option/extender class like addToDefinition(array|object &$definition). The base method can be gated to only work an array definition, leaving child classes that work with plugin attributes that return objects to override that on their own
• I think it's OK to allow the option attribute to overwrite an existing property value, even if it's not deprecated? We can provide guardrails if needed by adding a property on the option attribute like "allowOverride", defaulted to FALSE
• The option attribute should probably specify which plugin attribute classes it can work with

I'm pushing a PoC MR here that hopefully demonstrates what I'm talking about a little better.

One additional concern that is not yet accounted for:
Plugin definitions discovery is backed the FileCache stored in APCu. The FileCache is only invalidated by plugin class file modified time, so Drupal cache clears do not affect its contents. That means that installing or uninstalling a module will not invalidate the FileCache. In the example of Field UI, if entity type definitions are discovered with Field UI uninstalled, then that entity type definition is stored in FileCache without the field UI base route. Even after installing Field UI, FileCache data is not invalidated, so the field UI base route is not added to the entity type definition.

I'm not sure how to get around this without needing to invalidate the FileCache on every module install. This is also a very difficult thing to capture in automated tests, because all classes are autoloaded during tests regardless of whether their modules are installed.

Draft MR 10179 → opened per #26 ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active . I think the Nightwatch test failure has to do with field UI as mentioned, but I haven't confirmed.

I used attribute classes for Handler/StorageHandler/FormHandler for entity types, but I forgot that those plugin definitions aren't arrays, so they aren't a good example for how the base class works with array definitions, but I think it still shows how the attributes would work overall.

Gave it some thought, and refactored my PoC MR 10179.

My new approach to deal with the file cache: It won't be allowed to use attribute classes from modules. Only the base extender attribute (which I've bikeshedded myself again to be name PluginProperty) and some subclasses that all live in \Drupal\Core can modify plugin definitions. In order to annotate a plugin with PluginProperty attributes that such apply only when a module is installed, the provider property of the PluginProperty attribute must be set to name of the module dependency.

Attribute class discovery is changed so that the definitions parsed from Plugin attributes and altered by core provider PluginProperty attributes are saved in the file cache. PluginProperty attributes that have a provider other than core are saved in the "third_party_properties" in the array returned by parseClass and saved to file cache. The third party attributes are applied after retrieval from file cache if their module provider is installed.

I included examples of PluginProperty subclasses and applied them to Node, which looks like this:

#[StorageHandler(storageClass: NodeStorage::class)]
#[Handler(type: 'storage_schema', value: NodeStorageSchema::class)]
#[Handler(type: 'view_builder', value: NodeViewBuilder::class, allowOverride: TRUE)]
#[Handler(type: 'access', value: NodeAccessControlHandler::class, allowOverride: TRUE)]
#[Handler(type: 'views_data', value: NodeViewsData::class)]
#[FormClass(operation: 'default', formClass: NodeForm::class)]
#[FormClass(operation: 'delete', formClass: NodeDeleteForm::class)]
#[FormClass(operation: 'edit', formClass: NodeForm::class)]
#[FormClass(operation: 'delete-multiple-confirm', formClass: DeleteMultiple::class)]
#[Handler(type: 'route_provider', value: ['html' => NodeRouteProvider::class])]
#[Handler(type: 'list_builder', value: NodeListBuilder::class)]
#[Handler(type: 'translation', value: NodeTranslationHandler::class)]
#[Property(
  key: 'field_ui_base_route',
  value: 'entity.node_type.edit_form',
  provider: 'field_ui',
)]

MR is still a PoC, so there's some clean up, documentation, and more tests to be included.

> (which I've bikeshedded myself again to be name PluginProperty

What about ThirdPartyProperties, to match how the config system calls this concept?

> In order to annotate a plugin with PluginProperty attributes that such apply only when a module is installed, the provider property of the PluginProperty attribute must be set to name of the module dependency.

That makes sense.

It's a shame though, as the huge benefit of attributes is that you get IDE support for property names and types.

Thinking about it, this filecache problem is not just a Drupal problem, it's a PHP problem.
Consider we have package A, which has annotated classes, and it has a class Foo with an annotation for package B which is not a dependency. When the project owner installed package B, the annotation on class Foo is not picked up. Same problem as with modules, just with Composer packages instead. Is it something that's known about upstream?

godotislate → closed merge request !10179

godotislate → changed the visibility of the branch 3443882-plugin-option-attribute to hidden.

I've closed PoC MR 10179. Taking learnings from that MR and reducing scope to remove most of the entity type definition subclasses, I've opened MR 10218. I created a follow up for the entity type property attributes: 📌 Make it possible to define entity types with multiple smaller attributes Active

I also removed checking whether existing properties can be overridden. If there is a use case for making sure properties aren't overridden, that functionality can be reinstated, but it complicates logic when setting values with nested keys.

I've also added additional tests and updated the IS. I believe this is ready for review.

godotislate → changed the visibility of the branch 3443882-allow-supplemental-plugin-attributes to hidden.

Made a revision to the MR. Instead of using provider, added moduleDependencies to the PluginProperty class, so that multiple modules can be set as requirements for the property to be added to the plugin definition.

> The FileCache is only invalidated by plugin class file modified time, so Drupal cache clears do not affect its contents. That means that installing or uninstalling a module will not invalidate the FileCache.

We've seen at 🐛 Class "Doctrine\Deprecations\Deprecation" not found Active that any package update could require a clearing of the file cache.

If we were to update our requirements to say that the filecache needs to be cleared whenever modules or packages are updated, it would give us more flexibility here.

Rebased and refactored a little bit so that stuff that has to do with "providers" or modules is moved to the core AttributeClassDiscovery class instead of component.

There will be a merge conflict with 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active depending on which gets in first. Ideally #3490322 goes in first, but it is not a hard blocker.
Either way, it will be good to get both in to help with 📌 Convert MigrateSource plugin discovery to attributes Active and finally finish off moving core plugin types to attributes.

Discussed this issue with one of the plugin subsystem maintainers over Slack. He is not completely against having supplemental attributes, but he thinks it might be addressed more easily by allowing arbitrary properties to be set on Plugin attributes: $this->additional[$key] = $value. (Splitting the EntityType attributes into separate attributes is still valid and would not be tied to this.)

I know Drupal core does have a version of this with one plugin type: \Drupal\Core\Layout\Attribute\Layout, which looks like this:

/**
   * Any additional properties and values.
   *
   * @var array
   *
   * @see \Drupal\Core\Layout\LayoutDefinition::$additional
   */
  public readonly array $additional;

  /**
   * Constructs a Layout attribute.
   *
   * @param string $id
   *   The plugin ID.
       <snip ...>
   * @param class-string|null $deriver
   *   (optional) The deriver class.
   * @param mixed $additional
   *   (optional) Additional properties passed in that can be used by a deriver.
   */
  public function __construct(
    public readonly string $id,
    <snip ...>
    public readonly ?string $deriver = NULL,
    ...$additional,
  ) {
    // Layout definitions support arbitrary properties being passed in, which
    // are stored in the 'additional' property in LayoutDefinition. The variadic
    // 'additional' parameter here saves arbitrary parameters passed into the
    // 'additional' property in this attribute class. The 'additional' property
    // gets passed to the LayoutDefinition constructor in ::get().
    // @see \Drupal\Core\Layout\LayoutDefinition::$additional
    // @see \Drupal\Core\Layout\LayoutDefinition::get()
    $this->additional = $additional;
  }

We could possibly add $additional to Drupal\Component\Plugin\Attribute\Plugin, and merge the contents of $this->additional to AttributeBase::get() in Plugin::get(). This would involved updating all the plugin type attribute constructors as well.

I think I had a similar discussion with the same maintainer :)

I'm really not keen on putting things in an $this->additional array. We lose all the benefit of attributes if we do that -- better documentation, better picking up of mistakes, IDE completion, etc.

I put up MR 10668 with a POC of having a variadic property in the plugin attribute constructor. The EntityType attribute already has an readonly additional property that's used differently, so I couldn't use additional as the property name in the base Plugin attribute, and for now it's just $other. Also updated the Block attribute and added the allow_in_navigation property to the appropriate navigation block plugin classes per 📌 Provide a way for other modules to flag block plugin implementations as 'navigation safe' Active to demo this.

This approach works as far as I can tell, but I have reservations: setting arbitrary properties in the attribute is not self-documenting, so it's not immediately obvious where these properties come from or how they're used. Also, there would need to be some effort to update all the attribute constructors, and account for attributes that return arrays from get().

Since there's still desire to allow the EntityType attribute to be split up into multiple attributes, discovery will need to be updated to handle multiple attributes. Whether those additional attributes are used to set defined properties, in the EntityType case, or additional properties needed by other modules, such as in the Navigation case, do not seem to be a significantly different implementations.