AmbiguousBundleClassException if multiple entity types share the same class

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

I created a MR with a proposed fix.

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Moving back to NW. The MR is failing the test cases. If it's a bug it will need it's own test case also.

Would you say this is a duplicate of https://www.drupal.org/project/drupal/issues/3233143#comment-14631762 → ?

I fixed the existing test and added a new test demonstrating the bug.

The test-only patch is failing, the MR with the fix is passing. Setting to Needs review.

Can the MR be updated in 11.x?

Also will need a trigger_error for the new parameter. To not break existing sites.

I created a draft change notice → to mention in the deprecation message. Am I correct if I'm saying this will be deprecated in drupal:11.0.0 and required in drupal:12.0.0?

Not deprecated. Would have to look up an example but required in D11 but added in 10.2

11.x is actually going to be 10.2
Think of it as a master branch now and releases will be off 11.x till gitlab

Okay, I updated the versions in code and in the change record.

For the CC failure in the MR.

Thanks for taking care of that

Running the tests locally without the fix fails correctly

Drupal\Core\Entity\Exception\AmbiguousBundleClassException : Multiple entity types found for Multiple bundles are using the bundle class Drupal\Tests\Core\Entity\RoyalGala..

So even though the issue summary is talking eck tests show this is possible without a contrib too.

Coming from the original issue, this was added for following use-case

Say you have a node-type called page with a bundle class \Drupal\mymodule\Entity\Bundle\Page

You should be able to do the following

use \Drupal\mymodule\Entity\Bundle\Page;

// Note we don't pass ['type' => 'page'] here.
$page = Page::create(['title' => 'A page']);
// But this will still echo 'page'.
echo $page->bundle();

So it would be good to confirm we have test coverage for that, and that this doesn't cause a regression.

If you can identify an existing test and/or confirm this still works - fine to self RTBC

Searching for a test that has a namespace for \Entity\Bundle\ I don't find anything so guess we don't have test coverage for that scenario.

Added a line to BundleClassTest::testEntitySubclass() to verify that ->bundle() still returns the given bundle class. Not 100% sure though if that is what was meant in #19.

Left some comments on MR.

MR updated as per the feedbacks shared on MR.
Thanks!

Still failing.

Probably technically a problem with the coding standard itself as having a full stop to end the sentence after a version number probably should be allowed. Anyways I updated this to follow the sentence structure of similar deprecations.

Tests are green,

I see there is already a change record

Issue summary appears complete.

Believe this is good to go.

Is there anything left that needs to be done here?

Before this is RTBC, I believe this needs a closer look by the entity system maintainers, and probably more of the folks involved in the original bundle subclass efforts. I'm not officially an entity subsys maintainer, so I can't sign-off on that side. I would need more time to carefully review this to be able to "sign-off" (such as it is) as one of the bundle subclass "sub-subsystem maintainers". 😅

The issue summary needs to be updated, it doesn't explain what the patch is trying to do.

> Leave out is_subclass_of($class_name, $entity_type->getOriginalClass())? I'm not sure why that condition exists.

That condition exists because if you change the bundle class of Article, then Node::load() must still work, so we also always need to check and support the original class, and we must not break existing code that might not even know that there is a bundle class.

I would expect that tests fail in this scenario and that we have tests for this.

Okay, that changes things. Drupal assumes that EntityType->originalClass contains the entity type class, but in ECK's case it contains a common entity class that can be used for different entity types. Who's in the wrong here? I would think ECK, because it defines Drupal\eck\Entity\EckEntity as the entity type class, which it isn't. But on the other hand, there's no other way for ECK to define a common entity class for all its entity types. Users would be required to manually create classes for every entity type, but that would kinda defeat the purpose of the module. Or we could consider automatic code generation, but that's a can of worms I'd rather keep closed.

Thanks @berdir for taking a look!

Ok, I need to clarify/extend #31 a bit, after a closer look at the changed code and the context. I don't think the problem is the original class, the MR anyway doesn't just remove that, so that question/suggestion in the issue summary is misleading and definitely needs an update.

Unless it's altered, orignalClass is identical to the current class and I'm pretty sure that is the case here, entityClass is also going to be the same for all of them. The part that I mentioned in #31 should still work, a practical use case for that is the file_entity module in contrib, and that's not changed.

I understand the proposed change a bit better now and it makes sense. I was wondering if we could drop the duplicate check entirely but it's technically still possible to register the same bundle class for two different entity types, especially with ECK, and then you can't do BundleClass::load().

Apart from the issue summary update, I would propose that we change the logic to be a positive check and not negative with continue, this seems to be optimized for fewest line changes in the MR, but the resulting code is harder to read IMHO and that's what people will look at in the future. Instead I would do this:

if (isset($info['class']) && $info['class'] === $class_name) {
  $entity_type_id = $info_entity_type_id;
  // same class check
}}

that means an indentation change for the same class check, but we also have an extra loop now, the check itself is closer to what we have now in HEAD.

Attempt at updating the issue summary. Removed subsystem maintainer review needed, as @Berdir has reviewed (or should it stay there, apologies if wrong to remove).

Okay updated the code as per #34 and best I could do with issue summary from my understanding of it. Tests are passing again (issue fork was missing 11.x branch), so I believe this is now ready for review again.

Changes look good.

Left 2 small nitpicky suggestions.

Will leave in review for additional eyes.

Thanks for checking! Fixed the nits and some coding standards issues, tests green again. Ready for review.

Been about a week and don't want the issue to stale so will mark.

Nice to see this close, couple of questions on the MR - thanks!

Issue credits

Fixed the phpcs:ignore issue.

Are there any existing test entity-types we could use to test this in a kernel test? The amount of mocking has me wary of a false sense of security here.

Hmmm maybe @Berdir or someone could point me in the right direction and I could work on implementation within the tests? For now all I can think of is creating a test module that effectively reproduces ECK as minimally as possible. Not sure if that is what you are after though @larowlan?

Okay, new test added that creates two bundle classes of the same entity class dynamically. Tests only triggers the ambiguous bundle class exception on it, with the change, problem solved. This is as minimal as I could manage to mimic what ECK is doing there.

Perhaps someone else might have a cleaner idea, but I think it resolves @larowlan's concern about the amount of mocking.

Some notes/further thoughts:

1. Was not sure whether I should remove any of the existing new tests from this branch or keep them in
2. The full test run first failed on core/tests/Drupal/Tests/Core/Command/QuickStartTest.php, I could not understand why - running locally no issue and re-running no issue. Seems unrelated..

Believe all feedback from @larowlan has been addressed.

Applied 2 very nitpicky return types.

PHPUnit tests look like they are failing.

Rebased the MR.

I've left some comments on the MR - we just need to tidy up a few things I think.

@scott_euser don't want to step on any toes. let me know if you want me to try and take a shot at it.

@smustgrave all good if you want to give it a shot, I likely won't be able to for a couple of weeks. Thanks!

Today is a contribution day of mine so had time to take a look, let me know what you all think.

Looks good to me, thanks for taking care of it! Back to RTBC

alexpott → changed the visibility of the branch 11.x to hidden.

In #34 @Berdir asked for the subclass check to be kept around... but I think we already have that covered in \Drupal\Core\Entity\ContentEntityStorageBase::getEntityClass() and the \Drupal\Core\Entity\Exception\BundleClassInheritanceException it can throw.

Committed and pushed 644d8f2abd to 11.x and f342c259e9 to 10.3.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

DieterHolvoet → closed merge request !2563