Make it possible to define entity types with multiple smaller attributes

Since it was suggested that this maybe could go forward independently of ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active , I pushed up MR 10682. This is based on PoC work I originally started in the other issue, but now with additional attributes for setting entity type definition properties. Essentially, there is an attribute that corresponds to every set*() method in EntityTypeInterface, since that seemed the most convenient. It is possible to add other attributes regardless of whether there is a specific set method, because we can just leverage set(). But for now, started with these attributes:

• AccessClass (setAccessClass())
• Constraints (setConstraints())
• EntityTypeProperty (set())
• FormClass (setFormClass())
• HandlerClass (setHandlerClass())
• LinkTemplate (setLinkTemplate())
• ListBuilderClass (setListBuilderClass())
• StorageClass (setStorageClass())
• UriCallback (setUriCallback())
• ViewBuilderClass (setViewBuilderClass())

Also made converted the Node and Comment entity classes to use the new attributes.

> Essentially, there is an attribute that corresponds to every set*() method in EntityTypeInterface, since that seemed the most convenient.

I'm not keen on special-casing some handler types over others. I'm not sure why EntityTypeInterface does that anyway!

#[HandlerClass(type: 'route_provider', value: ['html' => NodeRouteProvider::class])]
#[HandlerClass(type: 'storage_schema', value: NodeStorageSchema::class)]
#[HandlerClass(type: 'translation', value: NodeTranslationHandler::class)]
#[HandlerClass(type: 'views_data', value: NodeViewsData::class)]

I've got to say, as much as I was looking forward to this, this layout is MUCH noisier and harder on the eyes than the annotation with its indentation for nested arrays. With the attributes it's just one massive block of text.

I'm not keen on special-casing some handler types over others. I'm not sure why EntityTypeInterface does that anyway!

I think someone in the other issue mentioned there'd be plenty of bikeshedding over which attributes to create and what they should be named, so might as well start now :).

I'm not that opinionated about this, though I will say it's nice to align with the set*() methods in that it leverages API as opposed to making assumptions about the definition schema. (The HandlerClass attribute is special cased because setHandlerClass doesn't deal with nested hander definitions.) On the other hand, with adequate test coverage, the point is largely moot.

Separately, I don't love how long "EntityTypeProperty" is, but "Property" on its own is useless as a search string.

With the attributes it's just one massive block of text.

I don't know if it looks better this way, either. Though it does look better in an IDE (or at least PHPStorm) than it does in Gitlab UI. And the attributes that have multiple arguments can be made to be multiline if preferred.

> (The HandlerClass attribute is special cased because setHandlerClass doesn't deal with nested hander definitions.)

There's an issue related to that! 📌 EntityTypeManager::getHandler() doesn't allow for nesting Needs work .

The problem with aligning with the existing set* handler methods is whether there was actually any planning that went into which handler types get a dedicated method, or whether they were just added piecemeal whenever one turned out to be needed.

I'm also not entirely sold yet on the whole thing. There are two things to consider, one is readability of the attribute(s), the other is discovery and finding what you can do. Attributes are much better at that already, except the nested array stuff. Hard to see if separate classes are really an improvement at this point without having used it.

I think we should worry too much about existing set() methods, they were added a long, long time ago. That form classes have a set method and route providers don't simply is because form classes existed when the EntityType class was created while route providers were added later.

We already have 3 route providers now in core (html (badly named in hindsight), revisions, and permissions. I think it's definitely worth adding a RouteProvider attribute as well, that greatly simplifies the syntax: #[RouteProvider('html', NodeRouteProvider::class)]. And you don't have to remember if it's route_provider or route_providers. Maybe even reverted so the key could default to html, which is the most common one. not sure, makes it different again.

In general, I'm unsure about the Class suffixes. I think I'd prefer that thy all use handler somehow, so for example AccessHandler, not AccessClass. (EntityType is also inconsistent on access vs access control). We could add more if we want too, although we're quickly getting into the topic of dependencies again. translation handler is a content_translation concept for example.

About the order, they are kind of alphabetical now except the EntityTypeProperty one, but I think I'd prefer having all handlers together.

In general, I'm unsure about the Class suffixes. I think I'd prefer that thy all use handler somehow, so for example AccessHandler, not AccessClass.

Changed all the handlers from something like AccessClass to AccessHandler. Given that...

I think it's definitely worth adding a RouteProvider attribute as well, that greatly simplifies the syntax: #[RouteProvider('html', NodeRouteProvider::class)]

Added this as RouteProviderHandler, since now all the other handlers have that suffix. I do not feel strongly about this, though.

I also added a EntityKey (which I guess could just be "Key") and a Label attribute, because they are commonly used enough. Adding Label also provides headway for #2813895: Combine entity type labels in an array → , or at least, a start to a way to provide new types of labels more easily. (There'd still need to be an API change on how to get those labels.)

Along those lines, here are some pros to splitting up the entity type attributes like this:

• It is more schema-agnostic for developers working on their own entity type definitions (maybe somewhat mitigated by devs using drush generate)
• Can set arbitrary properties on entity type definitions (though entity types already have $additional)
• Typehinting for property values otherwise within nested arrays
• Each attribute can provide more documentation about its property, though the classes are currently only lightly documented since this is POC

For cons:

• As mentioned in other comments, it's not necessarily more readable
• It's more typing, unless an IDE helps out with that via tab completion or similar
• Conversion to use them on existing entity types would be a large effort (though this maybe this is optional?)

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.

Made docblock update for nr bot code sniff and rebased.

Is this not considered an API change?

Is this not considered an API change?

Yes, but this really is in "Needs review" only to solicit opinions/feedback, and if there is more clarity and enthusiasm for a path forward, we can document the agreed approach and API change.