AmbiguousBundleClassException if multiple entity types share the same class

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 ?

Can the MR be updated in 11.x?

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

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

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. Also rebased it.
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.

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.

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.

Edit: just thought it would be helpful to note I did also retest this using ECK creating multiple bundle classes and using eg BundleClassNameHere::create($properties) - which subsequently calls the problem code in question. 11.x branch it fails and on this branch I can successfully run the code.

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

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!