EntityTypeBundleInfo::getAllBundleInfo() should always populate an entity type key with at least an empty array

Issue is related to the following bit of code:

          if ($bundle_entity_type = $entity_type->getBundleEntityType()) {
            foreach ($this->entityTypeManager->getStorage($bundle_entity_type)->loadMultiple() as $entity) {
              $this->bundleInfo[$type][$entity->id()]['label'] = $entity->label();
            }
          }
          // If entity type bundles are not supported and
          // hook_entity_bundle_info() has not already set up bundle
          // information, use the entity type name and label.
          elseif (!isset($this->bundleInfo[$type])) {
            $this->bundleInfo[$type][$type]['label'] = $entity_type->getLabel();
          }

When the entity is confirmed to have bundle types entities then the following foreach loop will load all bundle types entities for the parent entity, if none exist then nothing is added to the $bundleInfo array.

Proposed solution is to change the elseif to an if statement to catch all entities that have not been set and add the minimum info required.

Here's a patch..

Yep see what you mean, not ideal and it also fails the tests.

Here's a re-write, created an if statement before the foreach, if it fails it will return an empty array based on the $type.

Here's the simplified patch. Had a look at the test failures and not entirely sure that this one won't also fail.. I don't have the entire test suite for BrowserTestBase setup.. will sort this and hopefully take a look at adding some tests. In the meantime ill let the test runner do its thing.

Assigning to me, tests coming ;-)

Dug into this a little further and now not sure if the approach above is best. Firstly found that the current method is covered by tests:

/**
 * @coversDefaultClass \Drupal\Core\Entity\EntityTypeBundleInfo
 * @group Entity
 */

These are still passing fine, so no problem here. The issue is that the content_translation tests fail in: "/core/modules/content_translation/tests/src/Functional/ContentTranslationEnableTest.php" as the module is calling:

    $bundles = $this->entityManager->getAllBundleInfo();

On the admin settings screen "admin/config/regional/content-language" entities that don't have bundles should not be displayed. Therefore for the patch above to work and pass the other content_translation tests we would need to add an array filter after the initial look up to remove the empty entity array:

    $bundles = $this->entityManager->getAllBundleInfo();
    $bundles = array_filter($bundles);

But i don't think we can presume this how other modules would want to use the method in the future and the change might lead to contrib modules displaying an entity when there are no entity bundle types created for it. In most cases I think the current implementation is probably correct as it does not require additional clean up for displaying the correct list of entity types. I've added the patch to illustrate the change that would be required for tests to pass currently. But i'd be interested to know what the best solution would be in this case, there's probably a better way todo this.

I've been discussing this with @roborew and we think that we need to raise a separate issue against the Content Translation admin page.

1. To correct the test, so that this patch passes.
2. Instead to improve the UI and display the entity type but then show "No bundles available".

@joachim found some time to dig a little deeper, and your right the test coverage was not scratching the critical part of the getAllBundleInfo() i.e:

          if ($bundle_entity_type = $entity_type->getBundleEntityType()) {
            foreach ($this->entityTypeManager->getStorage($bundle_entity_type)->loadMultiple() as $entity) {
              $this->bundleInfo[$type][$entity->id()]['label'] = $entity->label();
            }
          }

And therefore there was no way of testing that '$this->bundleinfo' array was created correctly based on whether bundles existed or not. I've patched 'EntityTypeBundleInfoTest' to provide the setup to mock these elements and to enforce the right implementation of the method.

This change has a knock on affect to the Content Language settings form 'admin/config/regional/content-language' and will cause test failures. I've included an amend in 'ContentLanguageSettingsForm' to account for this change. Wasn't sure whether to create a separate issue and patch for this, and risk an issue dependancy. Therefore, i've included it here for brevity and to make sure all tests are still passing.

I've created an issue #2913824: Use of getAllBundleInfo() in ContentLanguageSettingsForm based on incorrect/buggy implementation to review the amend to 'ContentLanguageSettingsForm' and to proposed an alternative solution.

I took the liberty of modifying the patch according to @joachim's comment

Reroll patch against 9.4.x & also added reroll diff for reviewers

Resolved CSpell errors.

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -79,7 +79,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      if (!$entity_type instanceof ContentEntityTypeInterface || !$entity_type->hasKey('langcode') || !isset($bundles[$entity_type_id])) {
+      if (!$entity_type instanceof ContentEntityTypeInterface || !$entity_type->hasKey('langcode') || empty($bundles[$entity_type_id])) {

For me the fact that we have to make this change means that I'm not sure the behaviour change is actually worth it. Contrib code could be doing exactly the same and would be broken by this change. I think we should close this as works as designed. Going to ask the other framework and entity subsystem maintainers.

Yes I tend to agree with #34, seems like that could equally break things the other way.

Discussed with @Berdir, @longwave and @catch. We all have concerns about making this change. @longwave suggested that rather than making the change we should update the docs to be clear about the behaviour. @catch's suggested change is:

-   * Get the bundle info of all entity types.
+   * Get the bundle info of all entity types with bundles.

@alexpott i made changes as per your comment #36. Needs Review

Contrib code could be doing exactly the same and would be broken by this change.

I'm not sure how contrib would be broken? It'd be nice to have an array that doesn't need additional isset and count checks so that we can use array manipulation functions more easily.

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -79,7 +79,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      if (!$entity_type instanceof ContentEntityTypeInterface || !$entity_type->hasKey('langcode') || !isset($bundles[$entity_type_id])) {
+      if (!$entity_type instanceof ContentEntityTypeInterface || !$entity_type->hasKey('langcode') || empty($bundles[$entity_type_id])) {

Can we use count($bundles[$entity_type_id]) === 0?

@mglaman in exactly the same way core is broken - see the change to core/modules/language/src/Form/ContentLanguageSettingsForm.php - anything in contrib or custom that does something similar will be broken.

😬 oh duh, that wasn't done just for the fun of it. Sorry! Then yeah I guess that makes this a breaking change and something we'll need to live with forever?

I see #36, but unfortunately we still need this patch. I've rerolled #31 against 9.5.x and 10.1.x.

The patch from #38 was made for 9.5.x, but it also applies to 10.1.x.

@timohuisman why do you need this patch?

@alexpott, I should have been clearer; we are using this patch, I'm not entirely sure if we still need it. I've run a few tests on our project and it seems to work fine without it, but I'm a bit hesitant to just remove the patch. I will discuss it with my team and report back as soon as I know more.

At least for now I've fixed the code style issues of #43.

There's a better way.

1. In Drupal 10.x Introduce a flag as an argument on \Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo() which is set by default to FALSE but triggers a deprecation when set to FALSE. If set to TRUE we have the new behaviour.
2. In Drupal 11.x remove the flag and deprecate passing in an argument at all. (By this time we might rely on PHPStan for argument removal notifications so we might not have to add a deprecation for calling with an used argument.
3. In Drupal 12.x remove the deprecation of passing in an argument (if necessary)

This way we have less impact on callers from contrib/custom - they only need to make two changes. I.e to add the TRUE to the call in D10 and to remove it in D11.

@timohuisman the only reason you'd need this patch is because you custom code or some contrib code relies on it. You can search that code for calls to this method and see. This patch does fix not an actual bug.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.