Add a method to ContentEntityBase for getting its Bundle entity

Hmm not sure what to do about adding this to an interface.

Also, bundle-related methods are on the base Entity class, even though config entities don't have bundles. So this should probably follow the same pattern, and just return NULL on Entity.

> A stubbed-out method on some subclasses indicates that the method is too high in the inheritance tree. And "null if it doesn't apply" is a code smell, without question. :-)

I was aligning this with the bundle() method, which is on EntityInterface. But yes, it does seem a bit wrong.

Made that change, and the name change as well, since we have getEntityType/getEntityTypeId/etc.

> although a few other pieces of the patch seem to be missing? The last bit in particular, in the ViewsUIObjectTest. Was that not important? :-)

That was necessary when the method was on EntityInterface. Now it's moved up, that's no longer needed.

> It's still something-or-null,

I think it's still possible for a content entity to opt not to use config entities for its bundles. I heard of Commerce doing something like that with (possibly?) its payment method entities.

Seeing to needs work in light of:

> This code still assumes there is always a bundle entity type, which is not true for users for example.

> Maybe some nice little test would be cool as well.

and also:

> + $bundle_entity_type = $node->getEntityType()->getBundleEntityType();

$node??? What was I thinking?!?!?

> And "null if it doesn't apply" is a code smell, without question. :-)

Working on this to cover the case for user entity, I see that this is a code smell that's already there:

  $user_entity_type = $\Drupal::service('entity_type.manager')->getDefinition('user');
  // Returns NULL.
  $user_entity_type->getBundleEntityType();

Users have no bundle entity type, and so getBundleEntityType() returns NULL (though it's not documented that it can do so! -- #2801551: getBundleEntityType() can return NULL)

> Maybe some nice little test would be cool as well.

I had a look at where this might go, but the tests I see for the entity system don't appear to have anything as complex as a content entity & bundle entity combination. Any suggestions on where this could go?

Made a start on a kernel test, but I can't figure out how to get any output from the test while I develop!

debug() crashes the test (without giving a good explanation). print_r() outputs nothing. var_dump() of two entities brought my whole machine to its knees and I had to kill the test (something which Devel Kint does easily...)

> Wouldn't recommend var_dumping objects. I'd use xdebug.

In which case I'm going to have to leave writing the tests to someone else.

I've *once* managed to get xdebug working (after a DrupalCon session that walked through the setup) and about a month later it broke.

With young kids at home, my non-work coding is done in short stretches of usually only an hour or so, and I just don't have the time or the energy to go down the rabbithole of trying to get xdebug working.

I'm attaching the test file with as far as I got -- it doesn't work, and I can't figure out why.

On https://www.drupal.org/core/d8-bc-policy it says:

Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features.

> debug() uses trigger_error() to print it's HTML message, which causes the test runner to fail. Which is good, errors are bad. Also it prints HTML, which is intended for a GUI runner like Simpletest, not CLI like PHPUnit.

That's a shame. I'm sure debug() used to work with the old Drush test-run command.

> Additionally, tests will fail whenever a test causes output to be printed.

That must be a Drupal thing. I'm using PHPUnit for the tests for Drupal Code Builder, and my local copy has the test classes littered with print_r()s. Test produce a ton of output, but pass!

Thanks for writing the test!

It occurs to me that if the entity_test entity wasn't working because it doesn't have bundle entities, then it would be suitable for testing that case where we expect a NULL return. Here's that added to the tests.

Do you mean that the return type hint should be the config entity interface?

I'll update with changes on #31 and #32 later today.

Core seems to allow that a bundle entity *could* be content:

> protected function loadBundleDescriptions(array $bundles, EntityTypeInterface $bundle_entity_type) {

> * The name of the entity type which provides bundles.

Made changes from #31, and removed mention of config entity from the docs.

(Interdiff seems a bit odd, as surely the install schema in the tests was in the last patch...?)

I don't see how that's related to this.

An entity type whose bundles aren't config entities still has the Entity->bundle() method, which returns the bundle name.

What should we do here for these entity types?

Test fail doesn't look related to this patch.

Update.Drupal\system\Tests\Update\UpdatePathTestBaseFilledTest

fail: [Browser] Line 248 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
GET http://localhost/checkout/update.php/start?id=4&op=do_nojs returned 0 (0 bytes).

Still doesn't look like this issue is related to the test fail.

But there is already EntityTypeInterface::getBundleEntityType() -- the possibility of entities having bundles and those being themselves entities is present throughout the core entity component.

If we want to clean that up then fair enough, but then should we file an issue to move that to a trait too? I feel this is unfairly holding this issue up.

> Also is there anywhere we can use this in core?

I'll have a poke around.

Only thing I've found in core is in Content moderation, which has this method:

  protected function loadBundleEntity($bundle_entity_type_id, $bundle_id) {
    return $this->entityTypeManager->getStorage($bundle_entity_type_id)->load($bundle_id);
  }

which is only ever called like this:

loadBundleEntity($entity->getEntityType()->getBundleEntityType(), $entity->bundle())

I've made a start with adding a trait, but I think it's not ideal: presumably you mean that ContentEntityBase wouldn't use it, but Node, Comment, Term, and so on would. The problem I see here is that it makes this a whole lot less useful, because then a module like Content Moderation can't call it on a generic content entity.

The more I think about it, the more I think that a trait doesn't provide good DX.

Suppose I am working with general content entities. With a method, I do this:

if ($bundle_entity = $entity->getBundleEntity()) {
  // Do things with the bundle entity
}
else {
  // Fallback
}

with a trait:

if (method_exists($entity, 'getBundleEntity') {
  // Do things with the bundle entity
}
else {
  // Fallback
}

The latter doesn't look as nice!

#59 is very elegant, but it still requires you to know a fair bit about the internals.