Allow attribute-based plugins to discover supplemental attributes to set additional properties

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

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

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?

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.

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.

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

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

ETA. Actually, I think PoC does suggest that splitting up the entity type attribute into multiple attributes might be better as a separate issue, especially since all the entity type attributes would live in core, so there wouldn't be any worry about stale caches during discovery.

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.

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: #3488054: Make it possible to define entity types with multiple smaller attributes

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.

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.

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 #3490322: Use nickic/php-parser for attribute-based plugin discovery to avoid errors 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 #3421014: Convert MigrateSource plugin discovery to attributes 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 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 #3443833: Provide a way for other modules to flag block plugin implementations as 'navigation safe' 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 objects instead of 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.

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.

Re-based https://git.drupalcode.org/project/drupal/-/merge_requests/10668

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 rebased the feature branch for MR 10668, and there were no conflicts.

It is surprising how simple the change is (+33/-15) given the complexity of the discussion here. I guess this simple approach is just creating a "dumping ground" instead of a way to add first-class attributes to an annotation class. I am still working on understanding the discussion here, so maybe I do not have it quite right.

I am still working on understanding the discussion here, so maybe I do not have it quite right.

Summary of discussion/issue so far:

• The original proposal was to allow any module to provide attribute classes that could add supplementary property values to plugins for plugin types defined by core or other modules. POC of this work is in MR 7793, but the main issue with this approach is that it does not account for the behavior of plugin attribute discovery file cache
• Alternate approach, which is what is currently described in the IS, is for core to provide an attribute class that can be used to set supplementary properties on plugin definitions. This class can be extended in core to apply to plugin types that have object-based definitions (such as entity types) instead of array-based (almost every thing else). The class could also be extended by modules defining the plugin type, but not by other modules. This is done in MR 11218
• One of the plugin subsystem maintainers prefers allowing arbitrary values being set on the plugin attribute itself. This is done in MR 10668. The current small diff might be a little misleading: constructors for every plugin type attribute, or at least, the ones that should allow arbitrary attributes to be set, need to be updated.

@catch has suggested in #3009349: Revert migrate_drupal source annotations to attributes conversion #70 that the file cache issue could be addressed by making the file cache entry keys include the install modules list. Doing so would make the original idea possible.

I also just thought of another possibility to make the original possible once #3502913: Add a fallback classloader that can handle missing traits for attribute discovery gets in, but I'd need to POC it first.

Marking needs work for an issue summary update, the excellent summary from @godotislate at #51 would be the best starting point for that.

Updated the issue summary to describe the three approaches.

Thanks to @phenaproxima for updating the IS!

@phenaproxima also mentioned this is a Canvas blocker, and he and @catch seemed to have expressed preference on Slack for option 3, which was also the suggestion by one of the plugin subsystem maintainers, maybe it's worth continuing on the POC work in MR 10668. Also, maybe it's fine to do only the Block attribute here, add tests for a "generic" plugin attribute, then add follow up issues to update other core plugin attribute constructors as needed?

To me, #3 is the most straightforward option. If we mess with the file cache, we're inviting ourselves to enter a world of pain. I also think the third option is the most "obvious" one, and therefore best for DX. I also like that it allows different plugin types to decide if they want to support additional properties or not.

I bet we could reduce implementation pain if we added a trait for plugin attributes that support additional properties. To make it even easier, we could use that trait in \Drupal\Component\Plugin\Attribute\Plugin so that most plugin types across core and contrib get the capability by default.

Fair! I know what I personally would like the most, but I have no particular dog in this fight as long as it gets done somehow.

The lack of consistency is also bad DX.

Makes sense. Why not just add this capability to Plugin then, and all plugin types get it by default?

I bet we could reduce implementation pain if we added a trait for plugin attributes that support additional properties.

The changes to the Plugin base class in the MR basically do everything a trait could do. The problem is that, in order to get all plugin attributes to support being passed in arbitrary values, the constructor of each attribute class needs to be updated to capture the additional arguments in to the class property. By rough count, there are 50-60 plugin attribute classes that would need updating, though some are test classes, and some specific attribute classes already allow arbitrary properties to be set in different ways.

Possibly another reason in favor of allowing arbitrary parameters in plugin attribute constructors (sparked by #3524377-50: Allow to skip OOP hooks and services for modules that are not installed): since we use named parameters, this makes adding parameters in the future more difficult for BC, because a contrib plugin trying to support multiple versions will hit a fatal error passing a named parameter that does not exist in the older version.

Allowing arbitrary named parameters now might make adding any defined parameter later a bit easier, since contrib/custom modules would only need to reorder the parameter list.

Per @penyaskito in https://drupal.slack.com/archives/C072JMEPUS1/p1762261704549799?thread_t...

this is yet another issue where https://www.drupal.org/project/drupal/issues/3443882 would be helpful

OK, I think I found a way to let modules implement their own attributes that add properties to a plugin definition. This is somewhat based on the work from !10218, which added a PluginProperty attribute class, but extending the class in other modules was not allowed, in order to avoid issues with the file cache.

I've addressed the issue with the file cache by changing MissingClassDetectionClassLoader to collect all the missing classes that it comes across when parsing every plugin class during discovery. The idea is that if there are missing classes/traits/interfaces that do not cause an error or exception, then likely the missing classes are attribute classes, because PHP ignores missing attribute class definitions and treats them like comments. All the property attributes, both existing and missing, are saved for each cache item associated with the plugin class file path. When definitions are retrieved from the file cache, the property attributes are iterated through, and the ones that are available add their property values to the plugin definition.

To provide sample use cases, an AllowInNavigation attribute class is implemented and used on navigation module block plugin classes, so that those blocks are available to be placed in the navigation side bar. Also, a FieldUiBaseRoute attribute is implemented in the field_ui module and added to the Node entity type definition, so that the field_ui_base_route property is added to the Node entity type definition only when field_ui is installed. Conceivably, field_ui_base_route could be deprecated and removed as an argument from the ContentEntityType attribute constructor in a follow up issue.

The base PluginProperty attribute only works with array plugin definitions, and it will be left to follow ups to provide attributes that work with plugin types that have non-array definitions. For example, a generic EntityTypeProperty attribute could be implemented in #3488054: Make it possible to define entity types with multiple smaller attributes.

The implementation might be a little complex, so I'm leaving creating a CR for later if there's agreement that this is a good way to go.

New MR is !13976.

Would the file cache problem be resolved now because of the new DrupalInstalled::VERSIONS_HASH that's recently landed?

No. While the file cache will be invalidated more, because of package changes on disk or commits to the main project, the plugin discovery file cache won't be invalidated by Drupal cache clears or module (un)installs.

For example, uninstalling or installing Field UI won't invalidate the 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.

Rebased MRs after retarget to main branch.

Is the file cache something we can trigger the invalidation of?

We'd basically be going the route @catch suggested before where the cache key includes the list of modules installed and the file cache is saved to DB instead of APCu, but there've been blockers to getting the file cache to work with DB storage.

Since it's been so long elevating to framework manager sign off.

I'm not sure that we need framework manager review.

One of the subsystem maintainers expressed their opinion via slack in #42, and their preference was for something like the approach in https://git.drupalcode.org/project/drupal/-/merge_requests/10668 allowing for arbitrary properties and values being added to the attribute. I probably should have removed the tag then. But it doesn't look like there's a lot of support otherwise that would make it seem worth doing all the work to finish it.

Only elevated because it was tagged subsystem maintainer and if they don't respond we usually elevate. Can add that tag back if you want

My point is that a subsystem maintainer did already review in #42, I just forgot to remove the tag.

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.