Define bundle classes via attributes

Agree 100%, this would be very useful.

+1

Just like regular entity classes, we'd need to commit to a namespace (subdir).

Currently with entity bundles, and with this, I'd like to be able to have profile/modules which contain multiple entity types in an organised fashion. E.g src/Entity/Node/Article.php, src/Entity/Term/Tag.php

So supporting Entity/ should be fine, so long as we natively support recursion.

Big +1. Reduces boilerplate scattered between different files.

Left a comment on the MR.

But new service seems like something that could use a CR, examples always welcome.

Also, after having groked hook attribute discovery in a compoler pass, this should also run in a compiler pass (not as plugins).

Nice! Love it. Not sure how I missed this issue, but glad to see it mentioned in Slack just now.

I doubt that i'll have time soon-ish, but how i'd approach it:
- In ServiceProvider, register a CompilerPass
- In CompilerPass, iterate classes in the blessed directory, check attributes, then set the collected classes as a constructor parameter of BundleClassAltererHooks service
- In BundleClassAltererHooks, alter the bundles

With Hooks, we now have an implementation in core that doesn't use plugin discovery to find stuff, we could something like that here as well? Could then also be inlined into the EntityTypeBundleInfo, that would also make it clear that you're not meant to use this directly as an API.

The discovery should probably make sure that it's not possible to register multiple classes for the same bundle. Can also happen with the alter hook, where it's much harder to detect, but at least here we could?

What about subclasses? Lets say a contrib module defines a node type and adds a bundle class for it. In a custom project you want to subclass and change that. I guess you still have the option to use the alter hook then and set it there.

The discovery should probably make sure that it's not possible to register multiple classes for the same bundle. Can also happen with the alter hook, where it's much harder to detect, but at least here we could?

What about subclasses? Lets say a contrib module defines a node type and adds a bundle class for it. In a custom project you want to subclass and change that. I guess you still have the option to use the alter hook then and set it there.

Its possible to accept this, I've looked into and addressed it for BCA. This model works for particular upstream-downstream codebases.

Maybe you'd be interested in reviewing #3459534: Determine which bundle class should take effect when multiple classes extend the same bundle and associated PR.

Added comments to the MR with a few thoughts.

I decided to explore a slightly different approach, because I think it might be better for the entity type manager to pick up the bundle class info in its definitions than to have that information processed at container compile time. (It also saves from increasing the container size.) I borrowed a lot from work @mstrelan did in the other MRs, and did the new work in MR 14763.

Originally my thought was to use a discovery decorator and implement getDiscovery() in EntityTypeManager, with a decorator class that would wrap the default discovery and then add bundle class information into each entity type definition. Unfortunately that doesn't really work, because in EntityTypeManager::findDefinitions(), it's possible to add entity type definitions after discovery in entity_type_build hooks. So I changed the discovery decorator to an AttributeBundleClassDiscovery class that does discovery by attributes and sets the bundle class information on each entity type definition after the entity_type_build hooks are invoked, but before the entity_type_alter hooks run.

I considered whether everything in AttributeBundleClassDiscovery shouldn't just live directly in EntityTypeManager, or if AttributeBundleClassDiscovery should be a separate service, but I don't have strong feelings which way to go with it.

I also changed the discovery directory for bundle classes from /Entity to /Bundle, because I think there are performance issues in play for entity type discovery and bundle class discovery when they live in the same directory. This may be minimal if there are not a large number of bundle classes anyway, so I don't feel that strongly about it.

Anyway, once the entity type defintions have bundle class information stored, then extracting them in EntityTypeBundleInfo::getAllBundleInfo() is pretty straightforward.

I guess one other thing to consider, regardless of discovery approach, is if two modules have #[Bundle] classes for the same entity bundle. Which should win? Should module weight factor into it?

@ghost of drupal past proposed a fourth idea: Use the existing entity type plugin discovery and treat bundle classes as derivatives. This approach is in MR 14764.

Basic idea is:

• Bundle classes declare themselves as entity type plugin derivativies, with ids like '{entity type id}:{bundle id}'. This is facilitated by the Bundle attribute
• Entity type manager excludes any derivative definitions from getDefinitions()
• A new method getBundleDefinitions() on EntityTypeManagerInterface is introduced to get only the derivative definitions
• EntityBundleInfo can retrieve these bundle definitions from the entity type manager

I think this approach has an obvious advantage in that it uses an already existing iterative attribute discovery and does not introduce a new one.

I think the derivatives approach in https://git.drupalcode.org/project/drupal/-/merge_requests/14764 is the best idea, and much better than my idea in https://git.drupalcode.org/project/drupal/-/merge_requests/14763, so I'm taking the latter out of the running. Back down to three.

Even if we don't provide a way to set the weight/priority now, at least it should be documented.

Since this approach leverages plugin discovery, it's basically last one discovered wins, where last is determined the order of container.namespaces. We can document this.

Thought I thought of a weird way to implement priority. A integer priority argument with default value 0 can be added to the attribute. This gets appended to the derivative plugin ID, e.g. node:blog:100. For every matching class largest priority wins, and otherwise for those with the same priority, it's last one discovered wins. The would be determined in the getBundleDefinitions() method on EntityTypeManager.

Nah, it's not worth introducing a whole new syntax for that, it's not different to any other plugin as you mention. Like, what happens if two modules define the same entity type or two formatters etc. May I ask why is this seen as a big problem? Did it come up already among users of Bundle Class Attributes?

Could the site specific site bundle be in a module -- perhaps even a standalone module just for this? -- with a weight of 1001? The profile has a fixed weight of 1000 is set in InstallerKernel and ModuleExtensionList and install.core.inc all. Someone needs to file an issue to extract it into a constant and document it on module_set_weight at least. https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... documents the weight too.

Note that #2968232: Deprecate module weights is trying to deprecate module weights, but also:

1. Profiles shouldn't be implementing runtime code any more, we're trying to deprecate that too (slowly), otherwise they can never be uninstalled.

2. For that specific use case, using an alter hook seems reasonable.

I wasn't sure whether container.namespaces were sorted respecting module weight (looks like it is after checking), but per #43, that is moot.

I'll document that the way to specifically choose one bundle class over another for the same entity bundle is using the alter hook.

OK, updated the MR:

• Refactored discovery some so that it does not rely on derivative ID parsing. The bundle plugin IDs are still formatted as derivatives {entity type ID}:{bundle} to prevent collisions with actual entity type definition IDs, but otherwise they are not used. This will make it easier to change if there's ever a different use case for entity type plugin derivatives
• Documented that the alter hook can be used to choose the desired entity bundle class
• Re-did some logic so that existing entity bundle info property data is not overridden if those properties are not set in the attribute

Added a CR: https://www.drupal.org/node/3574891

Separately, I ran into a bug in the EntityType/ContentEntityType/ConfigEntityType attribute classes, where the uri_callback property type is string|null. Since the property is supposed to have a callable value, this typing prevents setting array callables of [class name, method]. This is a bit more complicated because PHP does not allow the use of callable as the type of class properties, so I'm not sure what the fix would be, other type changing the type to string|array|null. Any others? I don't know if \Closure should be allowed, because I think there might be times when entity types are serialized? In any case, this seems to be a little used feature and hence why no test coverage has picked it up.

For uri_callback see #2667040: Deprecate uri_callback in routes for entities, not sure if we can skip adding support for it here entirely while we also remove it over there?

Re: #47
I'm fine with skipping it, since the feature seems so little used, and the deprecation issue seems really close.

That said, if we skip it and for some reason it doesn't get deprecated, and there's a future need to add it to the attribute, then there's an issue of contrib perhaps not being able to support multiple versions, since passing arguments with named parameters that don't exist results in fatal errors. See https://3v4l.org/p2IWT.

While it seems uri_callback will be ever be needed, it does apply generally about how to add constructor arguments to attributes, since they're often instantiated with named parameters. One idea I've had is to add a variadic argument, like this:

  public function __construct(
    string $entity_type_id,
    ?string $bundle = NULL,
    ?TranslatableMarkup $label = NULL,
    ?bool $translatable = NULL,
    ...$other,
  ) {
  }

So then named parameters can be used with different versions that do and don't have the argument defined. It's not a perfect solution, because the undefined parameters will need to be passed last.

instead of variadic pass in $others = [] and then you can do $others['uri_callback'] should push come to shove ; variadic afaik can't be named but this way it can be

also document that this argument must be passed as a named argument so that future additional named arguments can be added if needed before it

variadic afaik can't be named but this way it can be

It can, but maybe we mean different things, so I'll provide an example.

Say in core 12.1, there's a new attribute Feature that looks like this:

#[\Attribute(\Attribute::TARGET_CLASS)]
class Feature {
    
  public function __construct(
    public readonly mixed $first,
    public readonly mixed $second,
    ...$other,
  ) {}
    
}

And contrib modules start using it like this:

#[Feature(
  first: 'foo',
  second: 'bar',
)]

Then in core 12.2, let's say $second is deprecated and a new argument $third is added, so like this:

  public function __construct(
    public readonly mixed $first,
    public readonly mixed $second,
    public readonly mixed $third,
    ...$other,
  ) {
    if (isset($second)) {
      // Deprecated.
    }
  }

The contrib module can add the third parameter and have one version compatible with 12.1 and 12.2.

#[Feature(
  first: 'foo',
  second: 'bar',
  third: 'something',
)]

Then in core 13.0, where second is removed:

  public function __construct(
    public readonly mixed $first,
    public readonly mixed $third,
    ...$other,
  ) {}

Contrib can have a single version compatible with 12.1, 12.2, and 13.0 12.2 and 13.0 like this:

#[Feature(
  first: 'foo',
  third: 'something',
  second: 'bar',
)]

And actually, third could be placed before second as early as it was known that second was deprecated and third added.

Edited: Can't have compatibility with both 12.1 and 13.0, because third would need to be last for 12.1, and second last for 13.0.

Edited x2: Since second was deprecated in 12.2, perhaps it could just be dropped in the last version.

Removed support for uri_callback in the MR.

With some more testing, ran into a couple issues with this approach:

• EntityType objects have max ID length of 32 characters. Since the derivative ID is entity type id:bundle, the total ID length can be up to 65 characters. When the Bundle attribute instantiates the entity type class in ::get(), an EntityTypeIdLengthException gets thrown if the ID is too long
• Bundle definitions get passed to the entity_type_build and entity_type_alter hooks. This is probably not a big deal

Made some changes to address everything in #51. I think this is ready.

All MR feedback should have been addressed now.

Thanks @mstrelan for the clean up on the CR. Do you think this is good for RTBC, or does it another review?

I would mark this as RTBC, but have one question - do we need test coverage for overriding an attribute based bundle class using hook_entity_bundle_info_alter?

Other than that everything is looking good. I also reformatted the CR a bit, removing headings and fixing some formatting issues.

Added alter hook test per #58 and parameter rename per #59.

Awesome, thanks @godotislate! Also learned that we can have hooks in test classes from that :)

Also learned that we can have hooks in test classes from that :)

Yeah, in kernel tests since #3502432: Make hook testing with kernel tests very simple!

I haven't read all the comments, but I understand you can define bundle classes through this from scratch without anything else?

I think that's problematic, at least for entity types that use bundle_entity_type. node bundles should all be node_type config entities, the type is defined as an entity reference field to with the node_type target type. Having a type that does not match that will result in validation issues and likely breaks code that expects to be able to do node->type->entity and things like that.

I understand that you could do the same with hook_entity_bundle_info too, but this explicitly advertises the ability to do so and I think we should consider to disallow that.

The change record also has a slight typo, bundle classes don't implement EntityTypeInterface (that's the definition object) but EntityInterface.

Made the change from EntityTypeInterface to EntityInterface in the CR.

As for defining bundles in code: I think @ghost of drupal past mentioned that he had bundles created completely in code on a client project, and that's where he got that from. Though I guess we can edit the documentation to remove mentions that this is possible.

Yes, I think its' neat if it's the only type of bundles you have. But mixing them from different sources sounds like a bad idea. We could add a check and specifically verify that it's not an entity type with bundle_entity_type?

Fair enough, that doesn't sound too difficult.

Thanks, that sounds like a great idea, as far as I can see it's just:

foreach ($attribute_entity_type_bundle_info as $bundle => $info) {
  if ($bundle_entity_type && !isset($this->bundleInfo[$type][$bundle])) {
    throw new \LogicException
  }

Re #67:
Change there looks good, but a question came up while I was testing. I hit the exception right as I deleted the bundle entity, which is expected, but then that means that the bundle class needs to be removed before the bundle entity can be deleted.

Should we throw an exception or just do nothing, i.e.

foreach ($attribute_entity_type_bundle_info as $bundle => $info) {
  if ($bundle_entity_type && !isset($this->bundleInfo[$type][$bundle])) {
    continue;
  }

I was wondering about that yes, both removing but also the opposite, when you deploy a new node type as part of a config import or so.

I think silently ignoring those is fine considering those edge cases.

MR and CR updated per #65 - 69. Back to NR.

I also restored a missing commit that addressed previous MR comments.

Thanks for addressing that. I fixed a typo in the CR (it's new_bundle, not new_class in the example) and tried to clarify that paragraph a bit.

Someday I will write or edit a CR without typos or mistakes, but that day has not yet come.

Have opened this a few times and neither committed it nor found anything wrong with it either, gave it another end to end look now and I still can't see anything to complain about. Nice to get rid of even more boilerplate and the code change is pretty compact.

Committed/pushed to main and 11.x, thanks! Tagging for 11.4 release highlights.

This is causing problems for us in linkit, see #3596529: LinkitHelper::getEntityFromUri('https://'... triggers an exception in 11.4, not sure if core should throw that exception. maybe it should respect $exception_on_invalid?