Bundle class changes mean the entity class is now determined by the active entity-type definition

If the class varies, set the class on the $this->entityType so that it matches and trigger a deprecation error advising that the entity class also needs to be set in an update hook.

IMO we should not be storing the entity class in installed definitions and the class should always be determined by code and not state. The state should be about the schema and we should only be comparing schema when determining entity definition updates. An entity definition mismatch due to changing the cache class for example is really icky.

Does it mean that static methods like load() and loadMultiple() could be broken?

no, it means that existing custom entity classes will be broken if you've been altering them in hook_entity_type_alter().

Thanks for finding this now! Yay for alpha releases. 😉 We tried to think of such things in the parent, but this was a hidden gotcha.

@alexpott also posted #2 to a Slack thread about this bug, to which there was some further discussion, not captured here. Pasting now so folks just following the issue can see:

- - -

larowlan
so that would require merging the two definitions somehow?

alexpott
Yeah I dunno - I’ve been ostriching this issue since I realised what we did in 8.8

berdir
that's not an easy change to make

berdir
but yes, if it is dynamic, you shouldn't set it on entity type, installed or not, but override getEntityClass()

berdir
for the bc layer, I would suggest that we set the BC property that we already support

berdir
that bc layer can go away together with that property

berdir
for the recommended change, for now, you have to install that change, untangling that definition into an installable and declared part is a big change that we can't block this BC layer on

- - -

Not exactly sure how to proceed right now. I certainly don't want to ignore @alexpott's wishes / perspective, but it sounds like that proposal will be a much bigger, harder change. Perhaps @larowlan's original proposal is more pragmatic for the short term to fix 9.3.0 before beta1, and we open a follow-up to do the larger surgery to stop using state for this entirely and aim that for 9.4.x? Or I guess since it'd be a disruptive, deprecation-inducing change, we'd have to do it for 10.0.0 and remove it entirely in D11?

In the constructor of SqlContentEntityStorage, check the $entityType argument against the active version returned from the entity type manager.
If the class varies, set the class on the $this->entityType so that it matches and trigger a deprecation error advising that the entity class also needs to be set in an update hook.

If this works, it sounds reasonable, and we should do it ASAP.

Otherwise I think we need to roll this back in 9.3.x and address it in the original issue.

Fixes the whitespace bugs and a few docs nits.

There's still no new deprecation layer here, or deprecation tests, etc. I think this is just POC that the solution works. But not sure about NW for that, let's still get some more eyes on this.

Whoops. 😉 Lucky 13? sh ./core/scripts/dev/commit-code-check.sh --branch 9.3.x is finally passing locally... Sorry for the noise. Also forgot interdiffs.

If we're going to keep this approach, we'll want to update some @todo comments since the plan in #3244802: Remove BC layers in entity system was to remove baseEntityClass entirely. But if we want to keep it, that can just remove the __get() and __set() stuff and the tests, but leave baseEntityClass alone now that we're using it.

Whoops, those fails in the pass patch makes sense. I was too specific updating those in #2570593: Allow entities to be subclassed using "bundle classes". I'll fix.

The fail test already failed as expected, since we hit that before the failures in the pass test. Not uploading another fail test. But this should now actually pass. 🤞

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -100,6 +100,7 @@
        $this->entityTypeId = $entity_type->id();
        $this->entityType = $entity_type;
   +    $this->baseEntityClass = $entity_type->getClass();
        $this->idKey = $this->entityType->getKey('id');
   

   I think that makes sense. The bug we have is specific to content entities, because only that storage overrides the entity type property with the installed version.

   Only wondering if we do want to change the name of this property now that we're likely going to keep it, but not sure what else to call it. it's private, so we could always go back to just entityClass in D10 when we remove the __set()/__get() bc layer.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/BundleClassTest.php
   @@ -248,4 +249,17 @@ public function testBundleClassShouldExtendEntityClass() {
   +   */
   +  public function testEntityClassNotTakenFromActiveDefinitions(): void {
   +    $this->container->get('state')->set('entity_test_bundle_class_override_base_class', TRUE);
   +    $this->entityTypeManager->clearCachedDefinitions();
   +    $this->assertEquals(EntityTestVariant::class, $this->entityTypeManager->getStorage('entity_test')->getEntityClass());
   +  }
   

   what about testing to actually create an entity and asserting the class of that, instead of just this method? not sure if it's really worth it.

I think we should change the active/installed definition to always get this from code and not store the value in state at all. This would allow us to discuss removing other things from the installed entity type definitions to make our lives easier. For example the persistent_cache and the render_cache properties.

The current solution means we need to update the plan to remove \Drupal\Core\Entity\EntityStorageBase::$baseEntityClass

I agree that we should do that, but not in this issue. It's a big mess, but it's an existing mess, while this specific issue is a pretty major regression and I don't think we can refactor the entire entity type definition handling in time for 9.3.0. This is an easy fix that keeps the status quo that so far was kinda implicit and just happened to continue work as $this->entityClass was set before we switched out the entity type definition.

Thanks for the reviews, folks.

Re: #19:

1. Yeah, I was wondering about the name, too. I came to a similar conclusion, $baseEntityClass is still a pretty reasonable name for it, given how we're now using it. I think going back to just "entityClass" in D10 would perhaps be even more confusing. I think we can just leave it.
2. Not sure it's worth it, either. We're already testing actual instantiation in other spots in BundleClassTest. Happy to add it if committers want it.

Re: #20:

1. Per #21, that sounds like a good plan, but way too complex to resolve on a tight timeline for 9.3.0. Would one of y'all be willing to open a new issue about it to more clearly spell out your proposal? I'll confess I'm not familiar enough with this active definition stuff, what's in state, what shouldn't be, how to untangle it, etc. If I were write that issue, it'd either be a useless stub, a confusing mess, or both. 😉 Tagging for a followup.
2. Agreed on the comments (see #14). Fixed here.

I also realized while fixing the comments for $baseEntityClass that we can already simplify EntityStorageBase::getEntityClass(), but not how we originally imagined. So I removed that @todo/@see and fixed it now ($baseEntityClass will always be set and we'll never call $this->entityType->getClass()).

While I was at it, I added the @todo/@see #3244802: Remove BC layers in entity system comments to the tests for the BC layer (the __get() / __set() of EntityStorageBase::$entityClass so those aren't forgotten. Hope that's cool.

Agreed with going for the quick fix here - it'd either be that or rolling back bundle classes. I feel like we should already have an issue for taking various entity type definition things out of state, but I couldn't find it looking for a few minutes, so the follow-up is good until/if we do find the other one.

Committed cefb93c and pushed to 9.3.x. Thanks!

This change introduces appears to be causing a compatibility issue described at #3259941: getEntityClass() must be of the type string, null returned where the EntityStorageBase::baseEntityClass is not set, and returns a badly typed result:

 public function getEntityClass(?string $bundle = NULL): string {
    // @todo Simplify this in Drupal 10 to return $this->entityType->getClass().
    // @see https://www.drupal.org/project/drupal/issues/3244802
    return $this->baseEntityClass ?? $this->entityType->getClass();
  }

That function returns a NULL when the baseEntityClass is not set and $this->entityType->getClass() returns NULL too (the signature of that function allows an empty return), but the function signature does not allow for NULL results.

Rewriting it like this fixes the issue, but it may well be a symptom of a deeper problem : it does not seems correct that entityType->getClass() should return NULL.

    return ($this->baseEntityClass ?? $this->entityType->getClass()) ?? "";

> Rewriting it like this fixes the issue, but it may well be a symptom of a deeper problem : it does not seems correct that entityType->getClass() should return NULL.

On multiple levels. This is already a fallback.

Sounds like you have a custom storage class that does _not_ call the parent::__contruct() method() That's where that is set.

And yes, this is a fallback when the installed entity type definition is missing the class, in that case you should write an update function and apply that change. load the installed definition, set the class, and update it.

Just double-checked in the specific case of #3259941: getEntityClass() must be of the type string, null returned:

• there is no custom NodeStorage
• there is a custom NodeStorageSchema extending Drupal\node\NodeStorageSchema to add
• custom NodeStorageSchema does not have a constructor at all, so it should be using the parent constructor automatically unless the instantiation uses non-standard mechanisms

Just to be sure:

• I added a constructor doing nothing but calling the parent constructor, but that changed nothing, as expected.
• I removed the custom schema, and that changed nothing either: during batches, this still returns NULL

As mentioned by the original poster on the other issue, this is a problem specific to batch situations : the same node loads work normally in other contexts. Maybe we should followup on the other issue ?

A follow-up was asked for in #22 and it was created in #24. Therefor I am removing the tag.