EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -699,6 +699,17 @@ public function getFieldDefinitions(array $constraints) {
+      if (count($bundles) == 1) {
+        // If there is only a single bundle, default to it.
+        $bundle = key($bundles);

That we cannot do. That would mean the metadata of an entity changes once you add another bundle - wait, what happened?? ;-)

+++ b/core/modules/field/field.module
@@ -431,8 +431,6 @@ function field_entity_field_info($entity_type) {
-    $optional = $bundle_name != $entity_type;

imho the wtf stems from entity::bundle() defaulting to $entity_type. As you say we do not really know whether it's just a bundle that's called that way or there is no bundle. This really is a field-API left-over we should fix to have the function return FALSE if there is no bundle.

Then, the current concept of having entity-type fields + bundle specific fields saves to have lots of base-fields in bundle maps and allows you to properly add a field to an entity-type without having to care about adding it to all bundles (what is a WTF also). Even worse, this code would have to adapt if a entity-type is altered (to get bundles later on, as first adding fields to "entity_type would suffice while later on not - yuck.

The idea is that you always have a set of fields per entity-type + optional extensions per bundle. That's imho the straight-forward appraoch one would expect.

So what about fixing Entity::bundle() instead??

Wouldn't bundle() being FALSE prevent us from differentiating between a entity type that has no bundles one where we don't know the bundle?

First off that should not matter e.g. when code conditionally incorporates bundle specific logic. Howsoever, we do not support entities without a bundle yet - if they have bundles.

I guess we could have something like $entity_type be our default-bundle if someone adds in bun

I think we could solve per instance modifications like that by relying on methods. So you have a static field definition that's used to create the typed data object. But once you have the object it can take contextual information into account (= the entity's bundle) and add make use or provide us with even more specific metadata then. I.e. it could add the entity type to the reference then.

I gave the bundle issue some more thought. We still have the inconsistency in the system between some code that handles the no-bundle case differently; there is field API related code which does $bundle == $entity_type then and there is code related to the new entity field API that does $bundle == NULL.

Strategy 1: For the new entity field API differentiating between $bundle == NULL and $bundle == 'something' is necessary as it supports the case of having fields attached to NO bundle (e.g. node base fields), while having fields attached to some other bundles at the same time (e.g. fields attached to node article).

Strategy 2: While field API and related code does not support having no bundle fields plus bundle-specific fields at the same time, it only needs to support he no-bundle field case when there are no other bundles - so assigning the no-bundle fields to the $entity_type bundle is fine then. However, as pointed out above, we cannot do that for all entity fields.

Summarizing so far - we've have code that supports no-bundle fields and bundle specific fields at the same time by using strategy (1), and other code which doesn't by using strategy (2). I think this is totally fine as long as this meets the desired use cases.

So what are the use cases we have right now? I'd mostly think of
a) - Defining fields no-bundle fields in code (strategy 1)
b) - Defining configurable fields (strategy 2)
c) - Defining form and view displays (strategy 2)

b&c are perfectly covered by strategy (2) I think, having screens that split up between no-bundle and bundle-specific fields would be certainly confusing. So while code has the power to do strategy (1), only strategy (2) is exposed to end-users.

Let's consider how that works out:
1) The default bundle of every type is $entity_type, as defined by $entity->bundle(). This lets strategy (2) work with the no-bundle case. This means we should always have at least one bundle.
2) Considering a module adding bundles to an entity type which originally has none, there will be new bundles in addition to the default bundle. This means configuration/code using strategy (2) won't apply to the added bundles, but strategy (1) would.
3) Entity types having bundles by default, work fine with strategy (1) and (2).

My conclusions:

• We've built-in support for a default bundle $entity_type, it would make sense to ensure that $manager->getbundleInfo() honors that and returns the default bundle also. (However, we'd have to take care that the default does not vanish as soon as a module adds a non-default bundles).
• Given that, the default bundle does not necessarily exist if entity types come with built in bundles, thus an assumption of $bundle == $entity_type -> entity-generic is wrong.
• The question raised in #2114707: Allow per-bundle overrides of field definitions is how to assign configurable fields to bundles now - should they be assigned using $entity_type or using NULL. The simple approach and from a DX standpoint logical approach seems to be using $entity_type bundle, while from a user-perspective the desired behaviour is probably having them be no-bundle fields. However, that would have more UX implications given displays etc. will have to keep following strategy (2), so this is probably best investigated/covered by a dedicated module.

How about using a class constant to define a default bundle. This would help differentiate between a no-bundle case and the case where the bundle name is simply the same as the $entity_type.

Not sure where the class constant would reside (maybe EntityManager::NO_BUNDLE or Entity::NO_BUNDLE)

Not sure, whether this is something that is bound by beta but it would make a lot of sense to clarify it before. Reading my conclusions from #11 they still make a lot of sense.

The problem we currently have is basically:

The default bundle of every type is $entity_type, as defined by $entity->bundle(). This lets strategy (2) work with the no-bundle case. This means we should always have at least one bundle.

Also, that means $entity->bundle() is not necessarily a bundle that the entity manager knows about, what seems to be bogus to me, i.e. it is inconsistent.

To fix, I think we should add a bundle with the name $entity_type_id always *by default* unless the entity type providing module already provides bundles. We can check that later on also by only considering the entity type providing module implementation.

That looks good, it still leave the issue of an 3rd party module adding bundles later on open though.

Moving to needs work as per

it still leave the issue of an 3rd party module adding bundles later on open though

So this rears its head and causes problems in two ways. It breaks content translation UI when there are no node types and it breaks the Views UI Node wizard when there are no node types.

Steps to reproduce:

1. Install minimal profile
2. Install Content translation module
3. Install Views UI module
4. Go to admin/config/regional/content-language and check the Content tickboxs that appear
5. Press save... kaboom WSOD.
6. Go to admin/structure/views/add and you'll be able to create a view listing nodes of type content - but note that no content types exist! If you do add such a view it'll break when you save it because the bundle does not exist.

I think EntityTypeBundleInfo::getAllBundleInfo() should not return anything for an entity type which says this other entity type is my bundle type and there are none of them.no node types. The patch attached is a slightly different approach from #17 and things have been moved a lot so no interdiff.

Discussed with @alexpott, given this is a fatal error via the UI and should be non-disruptive, tagging rc target.

@catch I'm not sure about the non-disruptiveness of this change since we're fixing what is returned by EntityTypeBundleInfo::getAllBundleInfo()

Fixing the tests - it looks the behaviour of Drupal\migrate\Plugin\migrate\destination\EntityContentBase::processStubRow() needs fixing to cope with the fact that the an entity type now might not have bundles - this is an example of the type of disruption this patch will cause.

I think this is a critical bug. With the current EntityManager::getAllBundleInfo() we are saying that bundleable entity types have bundles when they have none. This can lead to all sort of odd unpredictable behaviour for anything that uses this information. The only reason why we haven't come up against it is most times an entity type that uses bundles has at least one. However, imagine if you had one node type with the machine name 'content' and you then deleted it. According to EntityManager::getAllBundleInfo() it would still exist. This is nuts.

I've made this critical because:

• I don't think it will as easy to change this behaviour after 8.0.0
• I think we need to fix this.

Looks great, one nit fixable on commit.

+++ b/core/lib/Drupal/Core/Entity/EntityTypeBundleInfo.php
@@ -97,18 +97,16 @@ public function getAllBundleInfo() {
+          // First look for entity types that act as bundles for others, load them

Over 80 chars due to the indentation change.

Given we're short on time, bumping this to RTBC. I'll give this several hours and commit either this evening or tomorrow if no objections.

Big RTBC +1

Just working on some explicit test coverage

Here's some tests - I tried to add to the unit test but the just was too much mocking to be done and would have required large changes to the way the test is setUp.

Thanks @Berdir

1. Fixed and added test coverage.
2. Yeah I agree there is not much point in having the views wizard and allowing node views if no bundles exist. But this seems the quickest fix.

Unrelated test fails.

Back to rtbc - these installer fails have nothing to do with this patch.

Committed/pushed to 8.0.x, thanks!