Allow entities to be subclassed using "bundle classes"

+1 to this.

Hm. Kind of yet another variant of #1867228: Make EntityTypeManager provide an entity factory, a factory class would allow this too. But this is definitely a lot smaller.

And it's also totally compatible with #1867228: Make EntityTypeManager provide an entity factory, so I guess no problem with going this way for now.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -535,4 +537,11 @@ public function getAggregateQuery($conjunction = 'AND') {
+   * Gets the class-name of the entity.

Missing @param docs.

While I don't disagree with the change in this patch per se, I don't think that being able to change an entity's class is a complete or a correct solution to issues like #2720283: add plugins to Contact form types, remove special-casing for 'personal' contact form type, or for configurable and swappable functionality for entities in general.

First reason is that with swapping an entity class, your new class has to inherit the entity class, or implement its interface. That seems like a lot of work. Also, if you want the functionality to be selectable in the UI, the swappable entity classes need to be plugins. Having classes which implement entity interfaces and are also plugins seems messy to me.

Secondly, and more importantly, is that entities might want more than one swappable behaviour, and swapping just the class doesn't allow that. For example, in Flag module, we have a Flag config entity. That can take two plugins: one the represents the entity type the Flag is on, and one that provides the type of link the Flag uses. (And we might add a config property that holds a second copy of the Flag link type, so you can have a different link type for flagging and unflagging.)

Lastly, different entity types will have vastly different needs from swappable functionality. For Contact module, there's a lot of special-casing for the 'personal' form that should be moved to a plugin, eg, overriding the Contact form type listing row, denying access to the sitewide form for that config entity, setting the message recipient, and so on. For Flag, the things we want the plugin to be able to change or control are completely different. It's not one size fits all, so it's best to leave each entity class to work in its own way with the plugins it uses.

What might be useful though is seeing if there are any ways in which the DX for creating an entity - plugin combination could be improved.

When it comes to having per-bundle Entity classes or adding custom behavior to an Entity I think it's better to use composition instead of inheritance. There are going to be scenarios where two modules are going to want to add behavior to the same bundle or Entity Type (User for example).

I think it would be best to create Entity behaviors through the Plugin system and ultimately make something like this possible:

$user->getBehavior('commerce')->getBillingAddress();
$user->getBehavior('mycustombehavior')->getFullName();

$node->getBehavior('article')->doSomething();

A disadvantage of this is that manual type hints will need to be inserted to get autocompletion, but I think the pros outweigh the cons.

A disadvantage of this is that manual type hints will need to be inserted to get autocompletion, but I think the pros outweigh the cons.

Actually, that's a pretty big disadvantage. I heavily rely on an IDE (PHPStorm) which automatically indexes and identifies when syntax is deprecated, methods or parameters are missing, class no longer exists, etc.

Quite simply, having the ability to specify a custom class for any entity bundle/type, which can inherit from the entity class, and have its own methods unique to that bundle is far more beneficial than having to obfuscate this kind of functionality behind yet another plugin.

In fact, this "plugin" would really be a wrapper. A wrapper that would require the entity to be referenced constantly; not to mention all the type hinting that would need to be everywhere.

It's the difference between:

use \Drupal\node\Entity\Node;
/** @var \Drupal\my_module\Plugin\EntityBehavior\Node\Article $article */
$article = Node::load($nid)->getBehavior('article');
$article->doSomething();
$article->getEntity()->save();

vs. a far cleaner and more human readable DX:

use \Drupal\my_module\Entity\Article;
$article = Article::load($nid);
$article->doSomething();
$article->save();

I've worked on a re-roll, will upload it shortly.

> In fact, this "plugin" would really be a wrapper. A wrapper that would require the entity to be referenced constantly; not to mention all the type hinting that would need to be everywhere.

I don't see it as a wrapper at all.

It's a pluggable helper object. Exactly the same as all the entity handlers (storage, list builder, views data, routing, etc). We're fine with the DX for those, aren't we?

I don't see it as a wrapper at all.

$article->getEntity()->save();, specifically ->getEntity() is what would make it a "wrapper".

Exactly the same as all the entity handlers (storage, list builder, views data, routing, etc).

No, those are services which consume entities (which are already plugins).

---

This issue is, primarily, to allow said entity plugins to be subclassed from their original parent class.

Allowing for new, specific to the bundle, functionality that could be added and inherited methods to be overridden.

This functionality cannot be done as an external "behaviors" plugin, which is what you are proposing (and could be useful in its own right).

This is confusing enough without mixing up terminology.

\Drupal::entityTypeManager()->getDefinition('node') is the Node entity type. That's a plugin.
\Drupal\node\Entity\Node::load(7) is a Node entity. That is NOT a plugin.

See #22 :)

Meh, semantics. The node entity type plugin is also the class used to create the node entity object itself.

\Drupal::entityTypeManager()->getDefinition('node') is the Node entity type. That's a plugin.
\Drupal\node\Entity\Node::load(7) is a Node entity. That is NOT a plugin.

Actually, that is wrong :)

\Drupal::entityTypeManager()->getDefinition('node') is the plugin *definition*, aka the annotation class basically. \Drupal\node\Entity\Node *is* the plugin class, it's just that entities use weird and non-standard mechanisms to create plugin instances.

Too true, Berdir.

Also, "meh semantics" is not really a great reply when you're discussing changing how an entire subsystem works.

I only said that because I didn't want to get into a title war over how some may perceive "entity types" vs "entities" as plugins. The reality is that entities, while they don't use the normal plugin manager mechanism, are plugins as @Berdir confirmed in #22. Also, this isn't really about the "entire subsystem", it's a very small part of that subsystem allowing it to be extended just a little more.

Here's an 8.4.x version of #25.

> $article->getEntity()->save();, specifically ->getEntity() is what would make it a "wrapper".
> This issue is, primarily, to allow said entity plugins to be subclassed from their original parent class.

I think you're coming at this from the wrong side.

You wouldn't need to do that.
Your entity class's save() method would check for the plugin & call through to that, if your entity type is such that you need to have your behaviour plugin affect the save process.

Essentially, using plugins for entity behaviour is composition, whereas what you're proposing here is inheritance.

I think you're coming at this from the wrong side.

No, I'm not.

This is a legitimate use case: gain the benefits of all the work that has gone into a "node" entity, but want to add helper methods or override existing methods on the entity itself based on a specific bundle.

You wouldn't need to do that. Your entity class's save() method would check for the plugin & call through to that, if your entity type is such that you need to have your behaviour plugin affect the save process.

This means that functionality is technically determined by the "behavior" itself and thus requires the entity to perform these checks in each "supported methods" before proxying to said behavior. Again, not the use case here.

using plugins for entity behaviour is composition

Which is an entirely separate feature/use case (and could very well be used in addition to this), but not what this issue is about.

whereas what you're proposing here is inheritance

Correct, which is what this entire issue has been about: don't use $this->entityClass directly, abstract it so it can be overridden when needed or desired.

---

Re-assigning to myself to fix tests.

Sigh, a rouge .orig file made it in the 8.4.x patch.

The issue summary doesn't actually say what the use case is...

Per @webchick's request, I've added an example use case from a recent project to the issue summary. It's a little different from the use cases that have been mentioned in the comments, but if I understand correctly it's in the same spirit and implies the same solution.

Adding some pseudocode to the example per @webchick's request. The proposal pseudocode reflects the approach we took on my project, for which we used https://github.com/amcgowanca/discoverable_entity_bundle_classes. We were very happy with the outcome. It certainly delivered the benefits argued for in this issue. If we had it to do over again, I think we'd take the same approach. We wouldn't create a separate entity type because the entities should appear in the admin/content view, which they wouldn't if they weren't nodes.

I'm not entirely sure when @webchick commented on this issue, but the use case is quite simple: leverage Node entities (and all the stuff that's baked into that: UI, revisions, translations, etc.) while providing bundle specific classes for extra custom functionality (as needed).

We've been using this patch for https://quo.tag1consulting.com to completely refactor all of our custom procedural based "alter hooks" and tons of "if/switch" statements (based on ~8 bundle types) to simply abstract the unique code (which is a lot) into their own dedicated classes.

It just offers the ability for our code to be more OO and testable.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -293,17 +308,31 @@ public function loadMultiple(array $ids = NULL) {
+        $function($items, $this->entityTypeId);

Workspace module uses hook_entity_load() to swap revision however that change remains within EntityStorageBase::postLoad() scope because we pass to the hook wrong variable.

I suppose patch in #42 did not fail because in 8.7.x Workspace relies on hook_entity_preload() #928888: Please create a release.
I applied the same fix as in #46.

Here's an update that:

1. Cleans up a tiny bit of phpdoc to follow other optional parameters.
2. Adds a getEntityClasses method in EntityStorageBase to clean up some duplicate code.
3. Fixes not checking for ambiguous class definitions across both "exact" and "subclass" matches in EntityTypeRepository. I discovered this by refactoring out some methods. I'm not entirely happy with the $same_class pass-by-ref, so let's see if tests pass first. There also needs to be a test added for this case.

I misunderstood how the class matching was working (thanks tests!), so this restores only checking for subclasses if an exact match is not found.

This is working wonderfully!

And so here's a real life use case for how it is used, with nodes to use a subclass of Node for a given content type (bundle):

In a .module:

/**
 * Implements hook_entity_type_alter().
 *
 * This makes use of the patch from https://www.drupal.org/node/2570593
 */
function drutopia_findit_organization_entity_type_alter(array &$entity_types) {
  /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
  $entity_types['node']->setStorageClass('\Drupal\drutopia_findit_organization\FinditOpportunityNodeStorage');
}

In src/FinditOpportunityNodeStorage.php:

namespace Drupal\drutopia_findit_organization;

use Drupal\drutopia_findit_organization\FinditOpportunityNode;
use Drupal\node\NodeStorage;

/**
 * Provides a subclass for Find It opportunity bundles of the node entity type.
 *
 * @package Drupal\drutopia_findit_organization\Entity\Node
 */
class FinditOpportunityNodeStorage extends NodeStorage {

  /**
   * {@inheritDoc}
   */
  public static function bundleClasses() {
    $bundle_classes = parent::bundleClasses();
    $bundle_classes['findit_organization'] = FinditOpportunityNode::class;
    return $bundle_classes;
  }

}

In src/FinditOpportunityNode.php

/**
 * Provides a subclass for Find It opportunity bundles of the node entity type.
 *
 * @package Drupal\drutopia_findit_organization\Entity\Node
 */
class FinditOpportunityNode extends Node {
    /**
   * Return TRUE if an opportunity is outdated (hasn't been updated for a while)
   */
  public function isOutdated() {
    // ...
  }
}

And now anywhere we need to ask "is this opportunity stale" i can use this method, including in Twig templates:


{% if node.isPublished() and node.isOutdated() %}
Warning! This is old!
{% endif %}

The test fail is genuine (the patch uses deprecated code) so can't be RTBC. But fixing that is a novice task.

Fixed test

My bad I generated the patch using a codebase instead a clean checkout from the repository.

The patch (since #25) adds public methods to ContentEntityStorageBase but not to ContentEntityStorageInterface. At the least this might need a comment or discussion?

The patch (since #25) adds public methods to ContentEntityStorageBase but not to ContentEntityStorageInterface. At the least this might need a comment or discussion?

Adding a new method to an existing interface is a BC break. Not sure if this is explicitly documented anywhere, but I think it's mostly considered general knowledge by now. Regardless, it is documented that a public method not specified in an interface is considered @internal. Unclear if it actually needs an @internal tag though (seems a little overkill).

We could add a new interface and implement it on ContentEntityStorageBase. That actually might be better now that I think about it because then code can detect whether an entity storage instance supports sub-classible entities by checking if it's an instance of new said interface.

I have no comments on the details of the implementation, but just want to express my enthusiasm for this proposal. The ability to easily subclass node bundles is wildly helpful in all sorts of scenarios. The ability to implement custom pre and postSave hooks directly in a dedicated entity class is worth the price of admission alone!

Just ducking in to add a vote of approval for the concept being advanced, here. The general approach — using the generic Entity class by default but allowing config to specify a custom subclass for each entity type — is how the Timber framework for Wordpress operates. On smaller projects it's been a godsend.

Cleanly exposing entity-type specific business logic to Twig, rather than building helper libraries that exist in parallel to the entity types, is a big boon and eliminates a lot of very nasty special-casing that often migrates into the preprocess and template layer by necessity. It also keeps said code in a place that's easier to expose in APIs and non-HTML output types.

I wrote an article (https://www.lullabot.com/articles/maintainable-code-drupal-wrapped-entities) recently where I mentioned this issue.

I mention some nuance in the article that may be relevant for this discussion:

In the future, it may be possible to specify a custom class for entities of a given bundle. In that scenario, Node::load(12) will yield an object of type \Drupal\physical_media\Entity\Book, or a \Drupal\my_module\Entity\Building. Nowadays, it always returns a \Drupal\node\Entity\Node. While this is a step forward, it’s better to hide the implementation details of entities. Adding the business logic to entity classes increases the API surface and is bad for maintenance. This approach also creates a chance of naming collisions between custom methods and other added-to entities in the future.

The rationale is that adding more API surface to entity classes to, also, deal with business logic goes against of the single responsibility pattern.

In any case this patch will improve the current state of things, so big +1 from me.

In response to @e0ipso,

1) That's a great article.

BUT

2) The reality is that one often needs to attach all kinds of business logic to the existing insert / pre-save / post-save / delete processes. At present, we do this with hooks, which is inelegant, confusing, and often hard to debug. Using a wrapper to add new and different functionality to a node is great. But when it comes to altering the logic used in existing methods, a wrapper does not seem to help. The wrapper approach also adds the DX problem mentioned in #14 and elsewhere.

The issue summary is outdated, it doesn't reflect the current state of the ticket any more. Also tagging for a change record, it would be nice to include a real life example in it.

I've tried this out and this is really exciting, it is a game changer for working with entities, awesome work!

Reviewed the patch:

1. diff --git a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   index 23e1f32c56..6514124581 100644
   --- a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   +  public function getBundleFromValues(array $values, $throw_exception = TRUE) {
   

   I don't see a reason that this should be a public method, it is internal logic for ContentEntityStorageBase which is called during the creation of an entity.

2. diff --git a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   index 23e1f32c56..6514124581 100644
   --- a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   +  public function getBundleFromClass($class_name) {
   

   This can be a static method.

3. diff --git a/core/lib/Drupal/Core/Entity/EntityStorageBase.php b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   index 067b08acb5..2ef3beb6ca 100644
   --- a/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   +  /**
   +   * Return an array of entities indexed by class name and ID.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface[] $entities
   +   *   The array of entities to index.
   +   *
   +   * @return array
   +   *   An array of the passed-in entities, indexed by their class name and ID.
   +   */
   +  protected function getEntityClasses(array $entities) {
   

   Maybe Indexes the given array of entities by their class name and ID.?

4. diff --git a/core/lib/Drupal/Core/Entity/EntityStorageBase.php b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   index 067b08acb5..2ef3beb6ca 100644
   --- a/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   +  protected function getEntityClasses(array $entities) {
   +    $entity_classes = [];
   +
   +    foreach ($entities as $id => &$entity) {
   +      $entity_class = get_class($entity);
   +      if (!isset($entity_classes[$entity_class])) {
   +        $entity_classes[$entity_class] = [];
   +      }
   +      $entity_classes[$entity_class][$id] = &$entity;
   +    }
   +
   +    return $entity_classes;
   +  }
   

   Is there a reason that the &$entity values are returned by reference while iterated over? We are constructing a new array and not modifying the entities. Is there some performance benefit by doing this? My understanding is that in PHP objects are always passed by reference.

5. diff --git a/core/lib/Drupal/Core/Entity/EntityTypeRepository.php b/core/lib/Drupal/Core/Entity/EntityTypeRepository.php
   index f63965a8b3..597985bd6d 100644
   --- a/core/lib/Drupal/Core/Entity/EntityTypeRepository.php
   +++ b/core/lib/Drupal/Core/Entity/EntityTypeRepository.php
   +  /**
   +   * Get the entity type ID, ensuring only a single class implements the bundle.
   +   *
   +   * @param string $class_name
   +   *   The fully-qualified class name to look up.
   +   * @param int &$same_class
   +   *   A counter to keep track of if multiple classes implement the same entity
   +   *   type.
   +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   +   *   The entity type corresponding to the class name.
   +   *
   +   * @return string
   +   *   The entity type ID.
   +   *
   +   * @throws \Drupal\Core\Entity\Exception\AmbiguousEntityClassException
   +   *   Thrown if multiple classes implement the same entity type and bundle.
   +   */
   +  private function getUnambiguousEntityType($class_name, int &$same_class, EntityTypeInterface $entity_type) {
   +    if ($same_class++) {
   +      throw new AmbiguousEntityClassException($class_name);
   +    }
   +
   +    return $entity_type->id();
   +  }
   

   This could use some dedicated test coverage, to ensure that the exception is thrown correctly in case multiple classes implement the same bundle.

Updated issue summary.

Working on addressing #66 and #60

This is so awesome! I can not wait to start using this!

#60: I now added a new interface (SubClassableEntityStorageInterface) and implemented this on ContentEntityStorageBase.

#66:
1: I do not see that either. Changed to protected.
2: Fixed. Can see that maybe another storage would want to have it non-static though?
3: Good suggestion, changed to that.
4: Agree with you. I don't see why either.

I did not address #5 because I have to run. Leaving at needs work for now. Will pick it up tomorrow if no-one does before me :/

Thoughts/reviews on the changes are still very welcome though

Oops, forgot the files :D

Instead of SubClassableEntityStorageInterface (which seems rather abstract), perhaps BundleEntityStorageInterface would be better considering that is what the methods it defines is in reference to.

edit: I know this might seem like a nit, but... "naming things"--

Fair point!

I actually came up with the name before I realized what the methods would all reference and never circled back. Just for the whole background :)

Changed the class to BundleEntityStorageInterface and added a test for #66.5

+++ b/core/modules/system/tests/modules/entity_test_subclass/entity_test_subclass.module
@@ -0,0 +1,18 @@
+    $entity_types['entity_test_no_label']->setStorageClass('\Drupal\entity_test_subclass\EntityTestSubclassStorage');

+++ b/core/modules/system/tests/modules/entity_test_subclass/src/EntityTestSubclassStorage.php
@@ -0,0 +1,22 @@
+  public static function bundleClasses() {

Wouldn't this approach make it hard to let more than one module subclass a bundle for the same entity type?

I didn't review the patch yet, but agreed. Imho this should be put into the bundle info, which is currently a simple array with an alter hook, then anyone can set a class there.

\Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo(), then we have alter hooks and so on already available.

Great idea! Assigning to change this to use hook_entity_bundle_info().

Here is the minimal implementation to make the test pass. Since we now define the bundle classes in the entity type bundle info there is a possibility to have multiple bundles that define their own bundle class, so I have introduced a new AmbiguousBundleClassException alongside the existing AmbiguousEntityClassException.

I am going to see if we now can simplify the changes in the ContentEntityStorageBase.

Updated issue summary with a new example of how to define a bundle class using hook_entity_bundle_info_alter().

Shedding some weight, and documenting the new class property for the entity bundle info hooks + providing an example.

So there is a good reason to check the entity classes and bundle classes separately, since an entity class can subclass another entity class. This is e.g. heavily used in the entity_test classes who all inherit each other.

The patches of #78 and #82 are equivalent, I tried to simplify the code in #80 but it is in fact necessary to detect the entity classes and bundle classes in separate loops. The patch in #78 solves it by providing two separate methods, while in #82 the two loops are executed subsequently in the same method, which makes it unneeded to pass a counter around by reference.

This would be a big improvement for adding business logic to entity bundles. I have some generic remarks, followed by few nits:

• I like the idea from #76 for its simplicity and DX. However, altering is something that you do to change or fix an existing behaviour. At least when a bundle is provided as a config entity, I would like to be able to define its (sub)class before altering it, in the main definition. And this is the config entity. I want to be able to define the (config entity) bundle node.type.article in this way:
  

  name: Article
  type: article
  # Of course this needs a schema entry.
  class: Drupal\my_module\Entity\Article
  description: 'Use <em>articles</em> for time-sensitive content like news, press releases or blog posts.'
  …
  

  This can be achieved in EntityTypeBundleInfo::getAllBundleInfo(). To be discussed.

• The whole approach is governed by the assumption that bundle classes are extending the entity class. However, this is not tested anywhere. If we plan to allow bundle classes that are not extending the entity class, then we should fix EntityTypeRepository::getEntityTypeFromClass() accordingly. But I think we should enforce a bundle class to extend the entity. And I think we should test this and throw an exception early, in EntityTypeBundleInfo::getAllBundleInfo().

Few inline remarks:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -84,6 +85,34 @@ public function __construct(EntityTypeInterface $entity_type, EntityFieldManager
   +    // In some cases the entity bundle may be provided by the entity class'
   

   There's a quote ending this line.

2. +++ b/core/lib/Drupal/Core/Entity/EntityBase.php
   @@ -560,7 +560,14 @@ public static function loadMultiple(array $ids = NULL) {
   +    if ($storage instanceof ContentEntityStorageBase && ($bundle = $storage->getBundleFromClass($class_name))) {
   

   Questioning whether EntityBase should "know" about the existence of ContentEntityStorageBase. I think this is more ContentEntityBase's business. Wouldn’t it be better to move the logic there?

3. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -104,6 +104,20 @@ public function __construct(EntityTypeInterface $entity_type, MemoryCacheInterfa
   +   * @param string $bundle
   

   s/string/string|null

4. +++ b/core/lib/Drupal/Core/Entity/Exception/AmbiguousBundleClassException.php
   @@ -0,0 +1,23 @@
   + * @ee \Drupal\Core\Entity\ContentEntityStorageBase::getBundleFromClass()
   

   s/@ee/@see

5. +++ b/core/modules/system/tests/modules/entity_test_subclass/entity_test_subclass.module
   @@ -0,0 +1,21 @@
   +  if (\Drupal::state()->get('entity_test_subclass_enable_ambiguous_entity_types', 0)) {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntitySubclassTest.php
   @@ -0,0 +1,78 @@
   +    $this->container->get('state')->set('entity_test_subclass_enable_ambiguous_entity_types', 1);
   

   It's a boolean expression, let's use booleans instead of integers.

6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntitySubclassTest.php
   @@ -0,0 +1,78 @@
   +  public static $modules = ['entity_test_subclass'];
   

   s/public/protected

7. +++ b/core/lib/Drupal/Core/Entity/EntityTypeRepository.php
   @@ -89,6 +91,20 @@ public function getEntityTypeFromClass($class_name) {
   +        if (is_subclass_of($class_name, $entity_type->getOriginalClass()) || is_subclass_of($class_name, $entity_type->getClass())) {
   

   I'm noticing here an assumption: the bundle class *always* extends the entity class. But do we test this somewhere? See my generic comment.

Setting to Needs work for change record creation, clarifying the two main points and fixing the nits.

I'm also wondering if it's not too late for 8.9.x, this looks more like a 9.1.x.

> I want to be able to define the (config entity) bundle node.type.article in this way:

That's IMHO a nice DX improvement, but it's just that, an improvement on top of the bundle info system, so it has to be optional anyway and could be a follow-up. Trying to push things into the config entity structure too early/first has resulted in some issues getting stuck, like #2765065: Allow plurals on bundle labels.

Agreed on enforcing that bundle classes need to be subclasses, otherwise BC would be impossible to support when we add new methods and so on to entity types. Nice counter-example to the "everything must be final and then all our BC problems will be resolved" argument :)

@Berdir

so it has to be optional anyway and could be a follow-up

I agree that it should be optional, ::getAllBundleInfo() should test the existence of this property. However, now it's a good time to do it, because this change has a very small potential of BC break, as some existing custom bundle config entities might have defined already a class key for other purposes.

I'd also prefer to see that as a follow up to not widen the scope of this issue to much, I like that idea, but we'd still need to manage dependencies (i.e. that config would need to depend on the module providing the class?), potential BC as mentioned in #86.

@johnwebdev, well right now, if you define a class for bundle w/o config entity, in hook_entity_bundle_info(), a third party module is able to swap the class, in hook_entity_bundle_info_alter(). But a bundle defined via config entity is not a 1st class citizen because its class can only be defined in hook_entity_bundle_info_alter() and, as an effect, it cannot be swapped anymore by a 3rd party.

Fixing in this ticket or as a follow-up would require to handle the BC problem in this way:

• Create a patch against 8.9.x where we deprecate usage of a class key in bundle config entities.
• Create a patch against 9.0.x that throws an exception if a class key exists in bundle config entities
• Move this and the potential follow-up in 9.1.x.

Assigning, going to address the remarks. I also like the idea of being able to define the class in entity type config, but since this is still under debate I won't yet implement this. I think that the B/C break of the `class` key being already used in entity type config is quite unlikely to be a problem in the wild BTW. I also think that it would be very little work to implement it, but I'm fine if this is handled in a followup also.

Addressed all remarks from #84 except point 6 because it results in:

Fatal error: Access level to Drupal\KernelTests\Core\Entity\EntitySubclassTest::$modules must be public (as in class Drupal\KernelTests\Core\Entity\EntityKernelTestBase) core/tests/Drupal/KernelTests/Core/Entity/EntitySubclassTest.php on line 13

.

The protected modules patch (#2822382: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase) is only merged in 9.x.x.

The patch was using "subclass" and "bundle class" interchangeably. I settled on "bundle class" since it is more descriptive than "subclass".

Fixing failures.

Nice work, I see all the points got addressed. I have only a question and, depending on the answer, it might require a change:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1125,6 +1125,23 @@ public function __unset($name) {
+    if ($storage instanceof ContentEntityStorageBase && ($bundle = $storage->getBundleFromClass($class_name))) {

I wonder if the $storage instanceof ContentEntityStorageBase check really makes sense. Is there a storage of an entity, extending ContentEntityBase, that is not extending ContentEntityStorageBase?

Apart from that this still needs work for the following reasons:

1. The issue now is in 9.1.x. It needs reroll.
2. The change record is still missed.
3. It needs a follow-up, as per #90.

I also think that this is a Feature request, rather than a Task.

Just a reroll from #93.

Tests are failing, it seems this "Build successful" message is misleading.

Thanks for all the work on this feature!

I noticed today that it makes entity deserialization more painful, in a way that highlights a certain kind of subtle BC problem here.

In my contrib use case when an entity is deleted I'm serializing it and putting it into a queue to perform some remote API cleanup later. I can't enqueue the class name (as the class might not still be installed at the time of queue execution) so I have to enqueue the bundle as well as the entity type and infer the class name at the time of queue execution. Therefore I need to additionally inject the entity bundle service and conditionally check it:

      // $data is the array for a single queue job .
      $entity_class = NULL;
      if (isset($data['entity_bundle'])) {
        $bundle_info = $this->entityTypeBundleManager->getBundleInfo($data['entity_type']);
        $entity_class = $bundle_info[$data['entity_bundle']]['class'] ?? NULL;
      }
      if (is_null($entity_class)) {
        $entity_definition = $this->entityTypeManager->getDefinition($data['entity_type']);
        $entity_class = $entity_definition['class'];
      }
      $entity = $this->serializer->deserialize($data['entity'], $entity_class, 'json');

I wonder if a public method getEntityClass($entityTypeId, $bundle) on the EntityTypeManager would be a good idea now it has got harder to identify the right class for an entity?

Yes I think this is a good idea, surely more use cases will pop up where it will be necessary to be able to call getEntityClass().

Tests are failing because of this error:

The 'core_version_requirement' constraint (9.1.0-dev) requires the 'core' key not be set in core/modules/system/tests/modules/entity_test_bundle_class/entity_test_bundle_class.info.yml 

Back to needs work for #99.

We already added a getEntityClass method on EntitystorageBase. But this method is protected. I updated the access modifier so now we can call it like this:
$this->entityTypeManager->getStorage('node')->getEntityClass('page') which fixes #99.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -412,21 +428,24 @@ public function delete(array $entities) {
+    foreach ($entity_classes as $entity_class => &$items) {
...
+      foreach ($items as $entity) {
           $this->invokeHook('predelete', $entity);
+      }
...
+      // Perform the delete and reset the static cache for the deleted entities.
+      $this->doDelete($items);
+      $this->resetCache(array_keys($items));
...
+      // Allow code to run after deleting.
...
+      foreach ($items as $entity) {
+        $this->invokeHook('delete', $entity);
+      }

If I'm reading correctly the code, the predelete and delete hooks are called for each module as many times as different entity classes we have. The same is true for:

      $this->doDelete($items);
      $this->resetCache(array_keys($items));

I guess we only have to iterate $entity_class::preDelete($this, $items); and $entity_class::postDelete($this, $items);. I didn't check, but this may be true also for other hooks (load, etc.)

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -412,21 +428,24 @@ public function delete(array $entities) {
          $keyed_entities[$entity->id()] = $entity;
   ...
   +    $entity_classes = $this->getEntityClasses($keyed_entities);
   

   AFACIS this $keyed_entities logic should be removed here and handled in getEntityClasses() instead.

2. I can't see that #106 is right. Here's the complete loop:

   +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -412,21 +428,24 @@ public function delete(array $entities) {
   +    foreach ($entity_classes as $entity_class => &$items) {
   +      $entity_class::preDelete($this, $items);
   +      foreach ($items as $entity) {
   +        $this->invokeHook('predelete', $entity);
   +      }
   ...
   +      // Perform the delete and reset the static cache for the deleted entities.
   +      $this->doDelete($items);
   +      $this->resetCache(array_keys($items));
   ...
   +      // Allow code to run after deleting.
   +      $entity_class::postDelete($this, $items);
   +      foreach ($items as $entity) {
   +        $this->invokeHook('delete', $entity);
   +      }
   

   $entity_classes is produced by getEntityClasses() which actually returns an array, keyed by entity class, of subarrays of entities belonging to that class. $entitiesByClass and getEntitiesByClass() would be clearer names maybe.

   So currently in the patch hook predelete and hook delete are called once per entity, which seems right.

   $entity_class::preDelete() and $entity_class::postDelete() are called once per entity class, with only the entities from that class, which seems right.

   doDelete() and resetCache() are called once per entity class, with only the entities from that class; this doesn't seem wrong, though it would be possible to change the code so they are called only once with all the entities. I'm not aware of an advantage to doing that.

Addressed #2570593-107: Allow entities to be subclassed using "bundle classes". As far as I can see all remarks have been addressed now.

I would also like to mention that we are using this patch in production in a large project at the European Commission for 6 months with great success.

We use bundle classes for months in our project and it's charming that we can add business logic to bundles, as they are business objects. Everything looks good here, great work.

However, there are still 2 points preventing me to RTBC this:

1. Needs a change record
2. Needs a followup to implement #84 (first bullet point), as per #85, #87 and #90.

Setting as NW to fix the above.

Created:

• CR: https://www.drupal.org/node/3191609
• Follow-up: #3191620: Config entity bundles are able to declare their class in config

I have found this causes an issue with Webprofiler https://gitlab.com/drupalspoons/devel/-/issues/361

I think that is expected but adding a method to entity storage is allowed. I'm not sure if the serialzation issues have been addressed though, see #99

Looks like the ::getEntityClass() method was added (made public) in #105 to address #99 so that might handle serialization.

It's OK if this requires an update in devel module. I still need to do a proper review of this issue, just nothing these bits for later...

I pledge to update Devel once this issue lands. I've used DEBC on several projects and am a huge fan of bundle classes ... Would be great to get reviews from framework and release managers,

On #39 I also worked on quo, which was built very early in Drupal 8 (might even have been pre-8.0.0), and iirc the main reason it used node bundles rather than custom entity types was because at that time, group module support for different entity types (especially access), as well as core features like revisions wasn't there yet. Since 8.0.0 a lot of those issues with custom entity types have been fixed, so it could probably use entity types quite happily and nodes for one thing or not at all.

IMO this makes the use-case for this patch a lot narrower, since some of it is now handled by custom entity types. So the question is what are the use-cases where you really, really want to use bundles, but also need the bundle classes.

With configuration entity types there is no way you can just go and make a new entity type, so that seems like a stronger use case for this than content entities - but do we have contrib examples for that? At the moment all the examples in the issue summary are for content entities. Would be good to document existing examples where this would be used in the issue summary.

IMO this makes the use-case for this patch a lot narrower, since some of it is now handled by custom entity types. So the question is what are the use-cases where you really, really want to use bundles, but also need the bundle classes

One answer might be: when you want to extend/modify the behaviors of an entity type provided by core or contrib. In these cases creating your own new entity types doesn't help.

For example, the order entity type from commerce_order or the group entity type from group. These entity types are at the heart of a complex ecosystem of code and I cansimply drop-in my own entity types as a replacement and trust it all to work. But I could easily want to have a dozen different flavours of orders or groups, with overlapping sets of logic and fields. Bundle classes really help here.

or the group entity type from group.

Ahh I actually have a contrib/client use-case for this - where there are multiple bundles for groups and quite different logic between them.

@berdir in irc also mentioned using getters on bundle classes directly in Twig templates instead of preprocess, which was a direction we discussed moving in when Twig was first added to core (but has not really progressed in core that much yet).

Untagging for release manager review, the potentially disruptive change has been deferred to #3191620: Config entity bundles are able to declare their class in config.

Discussed this issue with @catch and @berdir. Berdir pointed out:

1. Adding getSomething() can help us remove preprocess and make things really nice for twig
2. It's a starting point for automated creation of methods for configurable fields
3. It's really hard to create a revision UI for custom entity types at the moment (there are a few existing issues about this)

Points 1 and 2 are compelling for me and @catch. I also think 3 is important if you are weighing up using a node type vs a custom entity type on a current site build.

Are we concerned about existing generic entity storage code using \Drupal\Core\Entity\EntityStorageBase::$entityClass and now doing the wrong thing? I think to commit this we need a good review of the bc implications and whether they'll break contrib and custom - especially any generic entity modules.

Affected modules according to grep.xnddx.ru for $this->entityClass:

Generic modules
bundle_override (41 installs, tries to do what this issue does so no surprise it clashes)
developer_suite (8 installs, doing all kinds of bleeding edge weirdness)
sparql_entity_storage (no installs)

Modules for external/plugin-based entities
Any attempt to set a bundle class on these modules would be to fight against their whole architecture - they already have a system of their own non-standard systems for entity flavours.
external_entities (215 installs)
apigee_edge (820 installs)
quiz (350 installs)
course (55 installs)
civicrm_entity (83 installs)

Seems like a pretty low BC impact in contrib.

Since I was pinged in Slack, I will comment (yesterday I found this issue already linked from the Lullabot article but I wouldn't have commented otherwise): https://www.drupal.org/project/agcobcau generates an entity class per bundle , stores it using phpstorage and this class overrides the load/loadMultiple/create methods. The class also has a @property in class doxygen for every field for better IDE integration. You can write things like $page = NodePage::load which will be properly type hinted to be a NodePage object and $page->field_foo will also be typehinted correctly. Again, this was written with IDE integration in mind. It has no releases and it's a gigantic hack anyways so it does not really affect this issue at all.

At our company we have been using our own module to be able to use bundle classes, wmmodel. It works using a core patch, which can be found in the repository. Bundle classes are defined using annotations. load, loadMultiple & create methods are overridden using a trait.

Another module, wmscaffold, provides the ability to generate these bundle classes using a Drush command, including different kinds of getters for fields of different types.

These modules aren't published on Drupal.org though.

Amen to:

Adding getSomething() can help us remove preprocess and make things really nice for twig
It's a starting point for automated creation of methods for configurable fields

Exciting stuff! Thank you to all who are working this actively.

Just wanted to add another use case we are using it for:

Deprecating and moving fields.

You can keep calling ->getMyField () after renaming field_my_field to my_other_field

We have been using this in a pilot project that has a large number of bundles, and it is in our view a game changer for working with complex code. We cannot wait to roll this out across all our Drupal projects. Use cases for contrib and core are limited, but for actual websites this is amazing.

Benefits

Object oriented

The main benefit is that it allows to define bundles and interact with entities in a full object oriented way. A complex project might have dozens of bundles that share business logic with other bundles. Things like content that is promoted to the front page, is subject to workflow, has a tagline, ...

Encapsulation of business logic

The bundle classes allow to add interfaces to bundles that identify which specific business logic they perform, and abstract away all the gnarly interaction with the Field API that normally is needed. We found that it greatly simplifies the way we write code.

For example we have declared a standard node type called "custom page" as a bundle class. In our project custom pages are group content (using Organic Groups), they have a publication date (from the Publication Date module) and they might have a logo (a normal image field). Our interface reflects the business logic that is encapsulated in this node type (full code: CustomPageInterface.php).

interface CustomPageInterface extends NodeInterface, GroupContentInterface, EntityPublicationTimeInterface, LogoInterface {
}

The bundle class itself can be mostly composed of traits, but has a few methods to declare things like field names that are unique to this bundle (full code: CustomPage.php):

class CustomPage extends Node implements CustomPageInterface {

  use EntityPublicationTimeTrait;
  use LogoTrait;
  use NodeCollectionContentTrait;

  public function getLogoFieldName(): string {
    return 'field_custom_page_logo';
  }
}

The traits are shareable across all bundles that share the same business logic. For example we have 6 bundles that have an optional logo, and they all use the LogoTrait (code: LogoTrait.php):

trait LogoTrait {

  public function getLogoAsRenderArray($display_options = []): array {
    return $this->get($this->getLogoFieldName())->view($display_options);
  }

  public function getLogoAsFile(): ?FileInterface {
    $value = $this->getFirstReferencedEntity($this->getLogoFieldName());
    return $value instanceof FileInterface ? $value : NULL;
  }

}

Discoverability of business logic

It is at all times fully clear what kind of entities you are dealing with. Writing things like alter hooks becomes a breeze. Let's say we need to control access, but only on bundles that are organic groups and have a workflow field:

function mymodule_entity_access(EntityInterface $entity, $operation, AccountInterface $account) {
  // Only act on entities that are subject to workflow.
  if (!entity instanceof EntityWorkflowStateInterface) { // <<<< AWESOME
    return;
  }

  // ... 
}

DX

Being able to use the IDE autocomplete to discover methods on practically any entity is also a very nice bonus. NodeInterface doesn't say which fields are available on this custom page node you're working with, but CustomPageInterface shows me everything it is capable of.

Example with bundle classes (yay!! :)))

Working with an entity that declares its responsibilities in this way is very convenient. For example if we are implementing a controller that should show information about an entity, and in case the bundle supports workflow then we should show the workflow state, this looks like this:

// Show the workflow state if the entity supports workflow and a workflow has been set.
if ($entity instanceof EntityWorkflowInterface && $entity->hasWorkflow()) {
  $build['workflow'] = [
    '#markup' => $entity->getWorkflowState(), // <<< Easy to use API
  ];
}

The whole complexity of interacting with the Field API is nicely encapsulated in the bundle classes.

Without bundle classes (boo! :((( )

In contrast, this is the code we were using before:

// Show the workflow state if the entity supports workflow. We can find out
// by checking if it has a reference to a workflow field.
$field_name = array_reduce($entity->getFieldDefinitions(), function (string $carry, FieldDefinitionInterface $definition) {
  if (empty($carry) && $definition->getType() === 'state') {
    return $definition->getName();
  }
  return $carry;
}, '');
if (!empty($field_name)) {
  $item_list = $entity->get($field_name)->first();
  try {
    $item = $item_list->first();
    // The item list can be empty in case the entity is new.
    if (!empty($item) && $item instanceof StateItemInterface && !$item->isEmpty()) {
      $property = $item->mainPropertyName();
      $value = $item->$property;
      // The value is empty if no workflow state has been assigned yet.
      if (!empty($value)) {
        $build['workflow'] = [
          '#markup' => $item->$property,
        ];
      }
    }
  }
  catch (MissingDataException $e) {
  }
}

The increase in DX is staggering. Of course you can create numerous "helper" services and classes to encapsulate this kind of logic, but bundle classes allow to use an entity centric approach.

Where to use bundle classes?

It can be applied to any kind of business logic you are dealing with on a day to day basis. No matter how large or small. Does the bundle have a logo? Slap a LogoInterface on it. Do we track the number of visitors in Matomo? VisitCountAwareInterface. A classic node type for an online event? EventInterface.

Tip

We have a trait to robustly interact with the Field API (ref JoinupBundleClassFieldAccessTrait), with the code taking care of all the edge cases of the Field API. This makes it quick and easy to develop bundle classes.

Just want to thank @pfrenssen for such a terrific comment. I've experienced similar major benefits but I could not have described them so well.

Looks like we need to expand test coverage a bit based on recent MR comments.

Reviewed the MR.

I addressed all the feedback that I found actionable. I'll try to implement anything else thats needed if folks can elaborate a bit more. Hoping we can get this one in.

+ // The bundle value is a field item array, keyed by the field's main
+ // property name.
+ $bundle = reset($bundle_value);

Please don't introduce more of #3178794: FieldItemBase::setValue() relies on code order thanks.

AFAICT, That’s pre-existing code that’s moved now into a conditional https://git.drupalcode.org/project/drupal/-/merge_requests/200/diffs#not....

I made that $entities_by_class change and rebased the MR.

If anyone is available to provide a final review, I'd appreciate it.

@moshe weitzman, Still needs work because of PHPCS

Hello, I'm posting here because it's not related to the functionality, but for the patch from the MR. When applying it, I get some duplicate files and folders inside web/core/ folder (check screenshot attached for an example). (web/core/b/ or web/core/core/)
Does this happen to anybody else?

@dimitriskr you're running into a patch level mismatch, probably using cweagans/composer-patches; see https://github.com/cweagans/composer-patches/tree/1.x#allowing-to-force-... and https://github.com/cweagans/composer-patches/issues/60#issuecomment-3940...

Ah I see, thank you very much! The file structure is now as expected

It sounds like this issue may resolve a lot of annoyances I have with using bundles.
I would love to check this out but I cant seem to find the patch file in the MR, or do I have to check the branch out and then create a patch file?

@James Byng there is a Download as -> Plain diff menu adjacent to Check out branch button.

Added some comments.

Rebased against 9.3.x on a different branch - I'm still not quite sure the etiquette of rebasing and force-pushing onto someone else's branch (not sure the remote would accept it but I wasn't about to try... the docs are silent on that?) It also looks like I couldn't change the target branch on the existing MR, anyway, which would add a lot of noise to the diff.

I'm interested in using this on a current project and so will take a stab at a commit addressing dpi's comments.

If I should just do this on the exisitng MR, someone pipe up as to what's appropriate?

Good thing I didn't force-push, that rebase is no bueno. Going to do a squash and then not have to rebase 18 commits.

https://git.drupalcode.org/project/drupal/-/commit/ebcecbe435a7ca1cc516c... and related (removing alpha modules form 9.2.x) seem to muck with a rebase against 9.3.x; I squashed the commits from the main MR here and cherry-picked them, which makes this more atomic.

Per Slack exchange in #contribute with @larowlan, we just fixed up MR 200 for this issue.

• He changed the MR to point to 9.3.x as the target.
• I force pushed a rebase of the original 2570593-bundle-classes branch relative to the HEAD of 9.3.x.
• I then pushed a few commits to address some of the pending review feedback in 200. More to fix, so leaving NW, but I have to stop here for the night.
• @bradjones1 then closed MR 994 to try to avoid confusion.

So I think we're full-steam ahead at 200 again. ;) I'll see if I can resolve anything else pending tomorrow...

Thanks!
-Derek

Status update:

• Lots of progress.
• Most review points are now addressed.
• Expanded Test coverage.
• I went through all changes with a fine-toothed comb and pushed some additional minor tweaks / cleanups / fixes.

Without being able to resolve threads, it's pretty hard to keep track of what's going on in the MR. The noise in here isn't exactly easy to follow, either. At the current moment, most of the review points have been addressed and could be resolved. The open questions are:

1. https://git.drupalcode.org/project/drupal/-/merge_requests/200#note_14489 ("What happens if the preCreate sets a bundle that is altered to use a class?")
2. https://git.drupalcode.org/project/drupal/-/merge_requests/200#note_27604 (postLoad())
3. https://git.drupalcode.org/project/drupal/-/merge_requests/200#note_27608 (doDelete())

Then there's new test coverage to review for these:

• https://git.drupalcode.org/project/drupal/-/merge_requests/200#note_14490 (commit 8f692b6b)
• https://git.drupalcode.org/project/drupal/-/merge_requests/200#note_33537 commit 2dfce5f38

Tentatively putting this back to NR, although it probably is still NW for something. ;)

@catch Asked for a summary update in #118, but the CR seems to have pretty clear examples of where/why you might want to do this. I guess we could harvest more of #128 into the summary, but that seems like a lot of noise. Added a link for now.

More importantly, I just changed the "None" lie in the API changes to try to document everything public this patch is now touching. ;) Everything seems pretty kosher, except that I believe this is a BC break:

• Added a getEntityClass() method to Drupal\Core\Entity\EntityStorageInterface

Although since we add an implementation in EntityStorageBase, I think we're still allowed under this rule:
https://www.drupal.org/about/core/policies/core-change-policies/drupal-8...

Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.

Also added a 1st draft of a release note snippet. This surely seems like it'll want a release note. Probably a "highlight", too. But I'll leave that to release managers / product managers to decide. ;)

Exciting! This is going to be really great to officially support in core...

Thanks,
-Derek

Although the work is all happening in MR 200, here are some patches based on the current state of the art that apply to 9.3.x and 9.2.x for folks (like myself) who need them for composer.

Simple video to maybe sell the benefits. https://youtu.be/DtA7YZ1sFKc

I believe everything in the MR is now addressed. Here's a test-only showing all the latest test coverage, with only commit 0b9a49968 reverted. This shows that the new tests catch the bug I spotted by eye.

Here's also a new pair of full patches for composer folks, and an interdiff relative to #155 to see what all has changed.

I believe this is now viable for RTBC if anyone else is willing to do a thorough review.

Thanks!
-Derek

Yay, just as expected / advertised:
https://www.drupal.org/pift-ci-job/2165792

Testing Drupal\KernelTests\Core\Entity\BundleClassTest
F....                                                               5 / 5 (100%)

Time: 00:03.869, Memory: 6.00 MB

There was 1 failure:

1) Drupal\KernelTests\Core\Entity\BundleClassTest::testEntitySubclass
Failed asserting that 4 matches expected 2.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/core/tests/Drupal/KernelTests/Core/Entity/BundleClassTest.php:153
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:722

FAILURES!
Tests: 5, Assertions: 68, Failures: 1.

Whoops. ;) @larowlan and I stand corrected. The "fix" for postLoad() actively breaks Drupal\user\Entity\Role::postLoad():

  public static function postLoad(EntityStorageInterface $storage, array &$entities) {
    parent::postLoad($storage, $entities);
    uasort($entities, 'static::sort');
  }

Once we're invoking postLoad() with $items, not $entities, that sort is only sorting the subarray, but the original $entities array itself is untouched. 😢 That's why https://www.drupal.org/pift-ci-job/2165788 is failing in Drupal\Tests\user\Functional\Views\HandlerFieldRoleTest and related things that are testing for role weights / ordering.

I pushed commit 71a3a3b66f to the MR about it. This undoes the "fix", adds comments about why, and updates the test coverage about method invocations and counts. Here it is in patch form.

Thanks, tests and testbot! Once again, you're saving the day...

After further discussion with @Berdir and @larowlan in Slack just now, we agreed to a slightly weird compromise. If EntityStorageBase::postLoad() only has a single class to deal with, we invoke that class's postLoad() with the original $entities array, to allow things like \Drupal\user\Entity\Role::postLoad() to reorder the roles based on weight, etc. However, if there are multiple bundle classes in the picture, each one only sees the $items subarray of entities that match the class's bundle. Restored the test coverage that to effectively what we had at #157. But with the special-case in play, all the role related tests are still passing locally now.

https://git.drupalcode.org/project/drupal/-/merge_requests/200/diffs?com... for the gory details.

p.s. Attached the Slack transcript of our discussion for history / posterity / context.

Will try to review this soon.

One thought I had in to another issue would be to use this in forum.module for its special vocabulary and node type. That could be a big step to transform that module from a forgotten and ignored thing to a decent example and a real-word demonstration of this feature. Might also allow us to fix #2010132: Canonical taxonomy term link for forum vocabulary is broken although I'm still unsure about that feature, should probably use aliases instead. Follow-up issue already, this is already complicated enough. An early proof of concept might help to convince anyone who still doubts that this is useful though :)

Can we, please, also deprecate the potential use of the class key in the bundle config entities as per #84, first bullet point? It should be done now, so that we can implement that in Drupal 10

see #171

I'm not sure what you mean by deprecate in #167 - it appears this is a feature you want to implement based on the work here? There is an issue for this: #3191620: Config entity bundles are able to declare their class in config

I will admit that I haven't followed all the recent changes here, but I am struggling to grok exactly what the specific issue is, here, that you say needs work by deprecation.

Yeah, I don't think that we need to deprecate a possible usage of that key now so that we could use it later. Even if someone would implement that right now for their config entity, assuming they use it or the same use case, it would not actually break anything. FWIW, I'm not quite convinced we need this to be on the config entity. That's definitely not something we want to expose in a UI anyway, at least not without a safe, restricted discovery and selection mechanism. If we go there we could just as well define entity bundle classes as their own plugin system and define their for which entity type and bundles they are, similar to formatters/widget. But that too can be explored in a follow-up issue.

I am currently using the patch for Drupal 9.2.x and ran into a minor issue. In my module, I extend Node with the bundle class Project. Additionally, I want to configure the bundle in /config/install/node.type.project.yml. This seems reasonable to me, since my module should not only provide the logic, but also the configuration for the new content type.

With this fairly simple setup the module installation throws the warning

Warning: Undefined array key "label" in Drupal\Core\Entity\Plugin\DataType\Deriver\EntityDeriver->getDerivativeDefinitions() (line 108 of /var/www/html/web/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php) 

Debugging the issue showed that hook_entity_bundle_info_alter() is called during the installation before the configuration is finished. Consequently, with the current suggestion on how to implement the hook, the bundle class gets overwritten before the configuration of the entity type exists.

This poses no issue because configuration will be provided at some point. Still, one might make the alteration of the bundle class more robust. In my use case the following was proficient

function mymodule_entity_bundle_info_alter(&$bundles) {
  if (isset($bundles['node']['project'])) {
	$bundles['node']['project']['class'] = Project::class;
  }
}

@Berdir,

FWIW, I'm not quite convinced we need this to be on the config entity.

Please see #88 why, I think, bundle config entities should get a chance to define the class before is being altered.

I'm not sure the situation described by @simonboy in #171 is a bug in so far as there are a number of similar areas where modules must be aware of the order of operations in how their default configuration is imported (think places where you check for $syncing = true) ... right?

a) Looks like #171 was a re-post and cross-post, and clobbered the previous status update.

b) I agree with @Berdir that we don't need to preemptively deprecate the usage of a class key now. I'm not clear that #88 is cause to further delay this issue. Seems possible to sort that out in a follow-up. This already risks never landing if it's too complicated for review and too much scope creep.

Therefore, restoring to 'needs review'.

Meanwhile, I opened the follow-up for #165: #3233398: Add a bundle subclass for forum.module's business logic

@dww You are right. I did not intend to change the status. I think I started writing my comment before the previous comment was published.

@bradjones1 I agree that this not a bug that is caused by this patch. The sequencing of installation and configuration is not entirely clear at the moment (see for example discussions around pre- and postinstall hooks). I would suggest to change the instructions on how to alter the bundle info (see e.g. #171). It is not comforting having warnings thrown that are not easily understood when using this feature in a 'natural' way.

@simonboy: Do _not_ call getBundleInfo() in your alter hook, that will cause a recursion. Drupal is currently building the bundle info. You already have the bundle info, just wrap it in a isset($bundles['node']['project') and you're fine. And yes, it makes sense that the code example in the change record uses that syntax, to avoid problems like that.

@Berdir Thanks for the advice.

Also agree with @Berdir (#170) and @dww (#174).

We're definitely going to need a follow-up issue (or two?) to look at what mechanisms should exist (other than in a hook).

For example, I also like the idea of config entities being able to declare their associated bundle class in configuration, but I don't think that should be exposed in the UI (you would only really want that for installing a bundle from config, a field in the UI just wouldn't make sense). I also like the approach of Bundle Plugins, that the Entity API contrib module provides, where you can define plugins as bundles instead of config entities.

However, this issue is already long, and I second the concern that if we dive down these rabbit holes here, we'll never get this done. Let's get this in, it's a fantastic addition, and use follow-up issues to properly explore and discuss each mechanism that should be available for taking advantage of this.

The patch looks good to me. Nice work everyone. I only have one question, What if as a contrib/core maintainer I don't want my entity to support bundle classes? Can I disable that for an entity?

Yes and no? You could make your entity class final, you could replace the storage and disable the logic that checks for that? But someone can re-replace the storage class or copy the entity class. Almost everything can be overridden, with a patch as a last-resort, GPL simply doesn't grant us the power to deny anyone from doing something they want ;)

Of course if you use this, then you need to live with the fact that there are BC risks. If you override any method that then changes in an update then you are in trouble.

It seems like the last remaining thing before this is RTBC is sorting out how postSave() should work. Is #164 sufficient? We had a big Slack thread about it, and I'm posting the highlights here so they're accessible to folks not on Slack, and so they're saved for posterity.

TL;DR: is #164 Good Enough(tm), or do we need to do something like this with doPostLoad()?

EntityStorageBase::postLoad() would be:

    // Invoke the right postLoad() depending on the kind of entity we are.
    $this->doPostLoad($entities);

    // Invoke all the external hooks when entities are loaded. Rest of the current function...
    // Call hook_entity_load().
    foreach ($this->moduleHandler()->getImplementations('entity_load') as $module) {
      $function = $module . '_entity_load';
      $function($entities, $this->entityTypeId);
    }
    // Call hook_TYPE_load().
    foreach ($this->moduleHandler()->getImplementations($this->entityTypeId . '_load') as $module) {
      $function = $module . '_' . $this->entityTypeId . '_load';
      $function($entities);
    }

Then EntityStorageBase::doPostLoad() is basically just:

   $entity_class = $this->getEntityClass();
   $entity_class::postLoad($this, $entities);

But ContentEntityStorageBase::doPostLoad() would become:

  $entities_by_class = $this->getEntitiesByClass($entities);
  ...

But we'd probably want the same hack to count the # of bundle classes and invoke postLoad() on the main $entities array if there's only 1 class. So I'm not really sure what all this refactoring would buy us...

But we'd probably want the same hack to count the # of bundle classes and invoke postLoad() on the main $entities array if there's only 1 class. So I'm not really sure what all this refactoring would buy us...

If this is the case, i think we should go ahead with what we have.

I've updated the CR to have the isset check suggested in #171 and confirmed as useful in #176.

The CR may still need an update for branch/version since 9.2.0 is already released.

In Open Social we currently have this functionality implemented using a custom solution. We were not aware of this issue. Our solution quite closely matches with what has been come up with in this issue. Very happy to see if I can provide this the final review towards an RTBC.

1. Although this is perhaps an edge case. Developers that want to use bundle classes and perform sorting on all results are not able to achieve their goal. This is explained by a comment but it's an either OR situation.
   

   +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -353,8 +361,23 @@ protected function preLoad(array &$ids = NULL) {
       *   Associative array of query results, keyed on the entity ID.
       */
      protected function postLoad(array &$entities) {
   -    $entity_class = $this->entityClass;
   -    $entity_class::postLoad($this, $entities);
   +    $entities_by_class = $this->getEntitiesByClass($entities);
   +
   +    // Invoke entity class specific postLoad() methods. If there's only a single
   +    // class involved, we want to pass in the original $entities array. For
   +    // example, \Drupal\user\Entity\Role::postLoad() sorts the array to enforce
   +    // role weights. We have to let it manipulate the final array, not a
   +    // subarray. However if there are multiple bundle classes involved, we only
   +    // want to pass each one the entities that match.
   +    if (count($entities_by_class) === 1) {
   +      $entity_class = array_key_first($entities_by_class);
   +      $entity_class::postLoad($this, $entities);
   +    }
   +    else {
   +      foreach ($entities_by_class as $entity_class => &$items) {
   +        $entity_class::postLoad($this, $items);
   +      }
   +    }
        // Call hook_entity_load().
        foreach ($this->moduleHandler()->getImplementations('entity_load') as $module) {
          $function = $module . '_entity_load';
   

   The workaround in that case would be to push the sorting into a hook_entity_load which still receives all the entities at once.

   This same section of code as 1. also receives $items by reference so that the input to EntityStorageBase::postLoad is mutated and available to the caller. We also loop over the results of getEntitiesByClass by reference. However, I believe, the getEntitiesByClass itself breaks the reference between the input to EntityStorageBase::postLoad and the $entity_class::postLoad call. This means that any sorting changes that a bundle specific postLoad function would not propagate as would happen for a postLoad call on a single class.

   I see that this was discussed in the MR: https://git.drupalcode.org/project/drupal/-/merge_requests/200#note_42131 but it may be good to document the limitation for bundle classes.

2. Not an issue but something noteworthy. This changes the value with which entity constructors are called. The value is used by ContentEntityBase which doesn't propagate it and checks for a FALSEY value (which NULL still is). However, this could break entities -- parenthesis for logical grouping -- that (do not extend ContentEntityBase OR overwrite the constructor) AND check against FALSE strictly [i.e. $bundle === FALSE] rather than using !$bundle. I think this is an edge case that can be ignored.
   

   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -503,9 +503,10 @@ protected function mapFromStorageRecords(array $records, $load_from_revision = F
   
        $entities = [];
        foreach ($values as $id => $entity_values) {
   -      $bundle = $this->bundleKey ? $entity_values[$this->bundleKey][LanguageInterface::LANGCODE_DEFAULT] : FALSE;
   +      $bundle = $this->bundleKey ? $entity_values[$this->bundleKey][LanguageInterface::LANGCODE_DEFAULT] : NULL;
          // Turn the record into an entity class.
   -      $entities[$id] = new $this->entityClass($entity_values, $this->entityTypeId, $bundle, array_keys($translations[$id]));
   +      $entity_class = $this->getEntityClass($bundle);
   +      $entities[$id] = new $entity_class($entity_values, $this->entityTypeId, $bundle, array_keys($translations[$id]));
        }
   
        return $entities;
   

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1128,6 +1128,23 @@ public function __unset($name) {
   +    // Always explicitly specify the bundle if the entity has a bundle class.
   +    if ($storage instanceof BundleEntityStorageInterface && ($bundle = $storage->getBundleFromClass($class_name))) {
   +      $values[$storage->getEntityType()->getKey('bundle')] = $bundle;
   +    }
   

   This change was introduced in #2570593-25: Allow entities to be subclassed using "bundle classes". I wonder if it should only do this when the bundle was not explicitly specified. For example existing code (even though it may be weird) that introduces bundle classes for nodes and has invocations such as $article_node::create(['type' => 'blog']); will now change behaviour and no longer return the blog bundle but instead start creating articles.

Beyond these three remarks everything else pointed me to "Reviewed & Tested by the community". However, leaving this at "Needs review" until someone else confirms whether the above points are indeed not an issue or should be addressed.

Replying to #2570593-183: Allow entities to be subclassed using "bundle classes":

1. This either-or situation is included to prevent this from regressing in the Role module, and possible other similar cases. The documentation makes it sound like it endorses using this method for sorting entities, but this is not actually something that should be done here. Entities returned from storage have no guaranteed order. Case in point, there is no documentation for Role::loadMultiple(), RoleStorage::loadByProperties() or RoleStorage::loadMultiple() that informs a developer that the role entities will be sorted by weight. The reason this was introduced is to be backwards compatible with the user_roles() function in Drupal 7 that was returning them in this order. Even back then this order was not documented. It is just something that is there "because it always has been", and possibly something that we should deprecate.
   On the other hand, for the intended use cases of EntityInterface::postLoad(), which is to enrich the loaded entities with additional data, we definitely should let each bundle class have a go at it, and they should only be passed entities of their bundle.
   I think the current implementation strikes a good balance between backwards compatibility and allowing this to be used like intended. If any existing code is abusing this method to impose an order on entities, then it is most likely to be done by config entity types (like Role) which are inherently bundleless. As soon as multiple bundles are in play, sorting entities at load time regardless of bundle doesn't really make much sense any more.
2. Interesting find! I agree that this will probably not be a big issue in practice.
3. I also raised this same point in an earlier review (ref. https://www.drupal.org/project/drupal/issues/2570593#mr200-note14489), but I now think that this is not really an issue. If a developer writes Article::create(['type' => 'blog']) and then get an entity of type Article then they 100% deserve to get what they ask for :D

Thanks for the reviews and comments.

Re: #183:

1. Agreed, we could probably improve the docs on this point.
2. Yes, good find.
3. We've already got a follow-up for that edge case at #3230792: Decide what to do in ContentEntityStorageBase::create() if preCreate() tries to change the bundle.

Re: #184:

1. Agreed all around. Thanks for writing this up.
2. 👍
3. Yup, see #3230792

I rebased the MR, and pushed commit c729434f0 to try to improve the docs. Is that sufficient to address point 1?

Can we consider #3230792: Decide what to do in ContentEntityStorageBase::create() if preCreate() tries to change the bundle equivalent to the question of point 3?

Given that @catch has signed off on #164 as cleaner than the alternative in #181, anything else before this is RTBC?

Thanks!
-Derek

Sorry, I rebased to a slightly stale copy of 9.3.x, and the MR was saying the merge was blocked. Re-rebased so it's merge-able.

I rebased the MR, and pushed commit c729434f0 to try to improve the docs. Is that sufficient to address point 1?

Yes, this is very clear now and the point is addressed, thanks!

I think this looks great now. I would RTBC if I could :)

Everything is addressed. Thank you!