Set default property bundle_entity_type in EntityType to NULL

Not a blocker, but

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -158,7 +158,7 @@ public function calculateDependencies() {
     $bundle_entity_type_id = $target_entity_type->getBundleEntityType();
-    if ($bundle_entity_type_id != 'bundle') {
+    if ($bundle_entity_type_id) {

Here are some improvements

Depends on what you want to know. If you want to know if an entity has bundles, use hasKey(). If you want to know if it has bundles that are stored as entities, use getBundleEntityType().

Yeah not sure about this, lets try to get some feedback from yched on this.

To summarize the problem in one sentencene, the problem is that field_ui relies in a weird way on bundle_entity_type being "bundle" when it is actually no entity type, which makes checks for this more complicated in other parts of the code.

I think that we should not make the API weird to make it easier for field_ui, but not sure what the best way to fix that is.

Uff.

We discussed this a bit. We're not sure what to do, but here is an idea:

Instead of supporting a dynamic parameter (which we do not understand works for FieldOverview), what if we always use {bundle} for our own routes:

Meaning, do $path = str_replace('{' . $bundle_entity_type . '}', '{bundle}', $path) in case we have a $bundle_entity_type. Then all the route parameters generation code can hardcode 'bundle'.

We don't know if anyything will break because of that, but it might be worth a try?

If it s not already documented, we should also expand the documentation for field_ui_base_route that you either have to put in the bundle entity type if you have one or bundle in there for field_ui to work.

Might be woth pinging @tim.plunkett about this ? IIRC he's the one who was kind enough to take care of moving field_ui's callbacks to routes :-)

Yeah, what I meant is to replace {node_type} to {bundle} in field_ui routes, but that breaks on the link templates, local tasks and so on.

We need beta evaluation in issue summary.

Changes look good to me. Let's see what @Berdir or @yched thinks about it.

@amateescu wanted to work on field_ui to clean up how the bundle route parameter works, that would simplify this a lot I think.

I remember someone (probably @Berdir) asked me to link here the issue for cleaning up Field UI routes, so here it is: #2428035: Bring some sanity into Field UI routes and forms

Rewritten this patch on top of #2428035: Bring some sanity into Field UI routes and forms. We can switch back to NR when that issue gets in.

#47.1 Done.
#47.2 I think it should be there all the time so contrib can depend on it for all Field UI routes.

@andypost, do you have any other remarks or can we get this to RTBC? :)

I'm not sure that the beta evaluation is correct. Have we really justified the disruption here? Also some of the issue summary sounds like a bug.

Rerolled and updated the beta evaluation. I agree with #52 that the current behavior smells more like a bug.

#2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles is post postponed on this.

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -693,4 +693,17 @@ public function getConfigDependencyKey();
+   *   - 'type': The config dependency type (e.g. 'module', 'config').

This is minor. While we are using 'key' and 'name' for dependencies (::getConfigDependencyKey() and ::getConfigDependencyName()) why not be consistent here? I would rename 'type' to 'key'.

#55: Drupal\Core\Entity\DependencyTrait::addDependency() has $type and $name as arguments, and the return array of Drupal\Core\Entity\EntityTypeInterface::getBundleConfigDependency() is always used to pass the dependency type and name to the addDependency() method mentioned above, so changing type to key would actually be very confusing IMO.

Rerolled the patch.

Missed a spot.

Bot fluke.

Needs a reroll.

Patch rerolled.

We need a change record to do this.

Needed a reroll.

And needs *another* reroll, 5 minutes later. :-)

More reroll.

I could apply the patch, so no reroll is needed it seems.

Let's see.

use statement missed.

CR https://www.drupal.org/node/2545280.

Better.

It'd be nice to get a +1 on the final solution from @Berdir or @yched

+1 ;)

This is a small API change that will affect some modules, a quick grep showed DS and field_group. But the current behavior is really weird and exposes field UI internals in the entity type API.

Maybe give @yched some time to give his +1? He already confirmed almost a year ago that he is onboard with the general idea but the patch changed a lot since then.

Back to rtbc.

The fail in #91 was due to not be able to checkout code on testbot and nothing to do with the patch.

+1 indeed, minor remarks below :

1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -256,18 +256,9 @@ public function calculateDependencies() {
   +    if ($bundle_config_dependency = $target_entity_type->getBundleConfigDependency($this->bundle)) {
   

   Here and for other callers of the new getBundleConfigDependency() : the method either returns something or throws an exception, so not sure why we bother with the "if ($dep = getBundleConfigDependency())", it's always gonna be TRUE ?

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -764,4 +764,30 @@ public function addConstraint($constraint_name, $options = NULL) {
   +  public function getBundleConfigDependency($bundle) {
   +    // If the target entity type uses entities to manage its bundles then
   +    // depend on the bundle entity.
   

   Minor : the comment was copy/pasted along with the code from EntityDisplayBase to EntityType, but the notion of "target" entity type makes little sense in that new location (it's now about "this" EntityType)

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -764,4 +764,30 @@ public function addConstraint($constraint_name, $options = NULL) {
   +      if (!$bundle_entity = \Drupal::entityManager()->getStorage($bundle_entity_type_id)->load($bundle)) {
   +        throw new \LogicException(sprintf('Missing bundle entity, entity type %s, entity id %s.', $bundle_entity_type_id, $bundle));
   +      }
   

   Probably not for this issue to change since it is just being moved around, but isn't that the kind of stuff asserts are for now ?

@yched,

1. #95.1: Good catch.
2. #95.2: Fixed.
3. #95.3: I don't understand. Asserts? Sorry, I'm missing something :)

Please re-RTBC if looks OK for you

Thanks @claudiu.cristea. Looks good.

Minor, can be fixed on commit :

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -764,4 +764,30 @@ public function addConstraint($constraint_name, $options = NULL) {
+    // If the this entity type uses entities to manage its bundles then depend
+    // on the bundle entity.

"The this" :-)

Ouch! Sorry :)

Setting back to RTBC per #98.

I'm committing this under the maintainer discretion provision in the beta evaluation policy. This makes bundle behave like other entity values - when an entity does not have a bundle it is NULL. Committed 877fd18 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.