Remove ability to update entity bundle machine names

I would actually recommend to kill this feature completely. The bundle name is an ID that is not supposed to change. Changing it requires a migration process of a potentially unbounded number of entities.

I suggest we make this form field disabled and remove the broken supporting code.

Missing node_field_data table.

Sounds reasonable - but also sounds like we should have tests for this ?

Here we go. :) Let's try this.

10: 2172843-10-only-test-should-fail.patch queued for re-testing.

It seems that there were new commits.

I agree with Damien here - we should disable the feature entirely when there are any nodes of the type, or possibly altogether - we already do similar for fields.

There's plenty of other places that would have to respond to a bundle rename, for example Views filters and any other kind of configuration entity that might reference a bundle. Don't think the complexity of checking all those places is worth trying to maintain the feature.

As far as field storage is involved, the specific case of nodes doesn't really matter. Are we saying we're removing support for bundle machine names changing, for all entity types ? vocabularies, whatever formulation some contrib entity type has for its own bundles ?

If some entity types have "renamable" bundles, then field storage has to support it.

I think we should remove it yes. Could leave the option to rename if there's no content possibly, but then delete/re-create is available then anyway.

For views/entity reference instances we could possibly use #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall.

Better status then?

Renaming is not just about the current site - it is also about deploying the configuration changes - see #1740378: Implement renames in the import cycle. I agree with @catch and @damien we should consider removing this feature - it introduce complexities for configuration deployment that are seriously complex and might be insurmountable. For example once #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall lands changing a machine namewill trigger updates to all of the entities that depend on it.

Edit: removing duplicate comment

Patch above no longer applies (but it has tests).

Re-roll. Haven't done one in a long time. Hope it's right.

These two below had been deleted from head:
core/modules/node/lib/Drupal/node/Entity/NodeType.php
core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php

patch rerolled.

#2575039: Prevent users from editing a content type's machine name once it has been set was opened as a new critical, but this is the older issue, with more background, and a more comprehensive (albeit stale) patch.

For reference/comparison, here is the latest patch on the other issue.

FWIW, #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted is trying to update the 'target_bundles' field setting on ER fields to account for bundle renames. And the amount of non-trivial code needed is a bit insane, and would theoretically need to be duplicated by any config that holds a list of bundles, which seems like a *very* common pattern - we have lots of things that talk about bundles, or are enabled/disabled by bundle...

We need to account for bundles being removed anyway, but that's simpler, and in some cases having a list with old bundles that don't exist anymore is less problematic)

Just reviewed the patch here, it's not relevant to the current state of this issue at all, was trying to fix renaming, so we should start from #45.

Running #45 past the bot since the fails are useful to see.

Updating the IS to reflect the latest thinking in #2575039: Prevent users from editing a content type's machine name once it has been set.

Preparing a patch combining:

• a form-level protection using a BundleEntityFormBase
• a data-level protection on ConfigEntityBundleBase

as suggested by @dawehner on the other issue. I'll remove the now-unapplicable booktest test regarding content type renamings.

+1 for removing this ability. Typically the reason I would need to rename a bundle is I made a mistake in the name on creation, which I usually can resolve before I've gone though and created lots of integration (Views, pathauto, etc). Would be good to know from contrib that we can rely on these not ever changing.

Since I'm not finishing it tonight, here is the first part: the form-level protection. Some notes about it:

• the existing protection in BlockContentTypeForm::form() and ShortcutSetForm::form() should probably be removed, as redundant with the shared one in BundleEntityFormBase
• although this is unrelated to this issue, it seems the third parameter in the parent::form() call in VocabularyForm::form is useless : none of the parents appears to expect being passed a third parameter. If it actually serves some purpose, it will need to be commented

Setting to CNR to get an idea of the current errors : not expected to pass yet.

And here is a version with the data-level protection. Some other changes:

• BookTest::testBookNodeTypeChange() removed since node type changes are no longer allowed
• protectBundleElement() method renamed to protectBundleIdElement() to be clearer
• removed the existing protections on ShortcutSet and BlockContentType, refactored to the base class
• removed the invalid third parameter in VocabularyForm::build() parent::form() call
• some coding standards fixes to the changed files to reduce the amount of validator complaining

Unassigning for now in case someone wants to work on it during the day.

Sorry, slight error in the previously uploaded patch. Here is the proper version.

Reviewed, and fixed all my remarks myself: all minor stuff.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -102,6 +102,32 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +   * their id be renamed.
   

   I think: s/id/ID/

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -102,6 +102,32 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +      assert($entity_storage instanceof DynamicallyFieldableEntityStorageInterface);
   

   Should be quoted.

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -102,6 +102,32 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +        throw new ConfigNameException(t("The name of this @bundle cannot be changed, because @content content already exists.", [
   

   And here it says "name", not "ID".

   It should actually be "machine name", per the issue title?

   Internally, it is "entity bundle ID", in human-facing text, it is "entity bundle machine name".

4. +++ b/core/lib/Drupal/Core/Entity/BundleEntityFormBase.php
   @@ -0,0 +1,35 @@
   + * Contains BundleEntityFormBase.
   

   Nit: needs FQCN.

5. +++ b/core/lib/Drupal/Core/Entity/BundleEntityFormBase.php
   @@ -0,0 +1,35 @@
   +namespace Drupal\Core\Entity;
   +
   +
   +/**
   

   Nit: 2 blank lines instead of 1.

6. +++ b/core/lib/Drupal/Core/Entity/BundleEntityFormBase.php
   @@ -0,0 +1,35 @@
   + * Class BundleEntityFormBase is a base form for bundle config entities.
   

   Docblock needs to be updated.

7. +++ b/core/lib/Drupal/Core/Entity/BundleEntityFormBase.php
   @@ -0,0 +1,35 @@
   +   * Protect the id element from updates if needed.
   

   "id element" is misleading/vague.

8. +++ b/core/lib/Drupal/Core/Entity/BundleEntityFormBase.php
   @@ -0,0 +1,35 @@
   +  protected function protectBundleIdElement(array $form) {
   

   Incomplete docblock.

A few more in the same vein:

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -105,7 +105,7 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
       * Ensure that config entities which are bundles of other entities cannot have
   

   Just noticed that coding standards say {@inheritdoc} must only be used for unchanged docblocks https://www.drupal.org/coding-standards/docs#inheritdoc so we need a complete docblock

2. Also, it probably needs to include an @throws for the exception.

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -114,10 +114,10 @@ public function preSave(EntityStorageInterface $storage) {
          $entity_storage = \Drupal::entityManager()->getStorage($bundle_of);
   

   We can use $this->entityManager() here. And since we're using it twice in the function, we should refactor its reference to a local $entity_manager.

Shouldn't we then also remove our "bundle has been renamed" notification stack ?
- EntityBundleListenerInterface::onBundleRename() and its implementations (EntityManager, SqlContentEntityStorage)
- hook_entity_bundle_rename() and its implementations (field, field_ui, language)

Yes I think we should remove them, then if we ever add this back and the existing API turns out not to be sufficient, we're adding APIs rather than breaking them at that point. Also removes dead code when we ditch the implementations/invocations that will never run.

Just some drive-by code removal (62/63/65)

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
@@ -113,7 +89,7 @@
-      $entity_storage = \Drupal::entityManager()->getStorage($bundle_of);
+      $entity_storage = $this->entityManager()->getStorage($bundle_of);
       assert('$entity_storage instanceof DynamicallyFieldableEntityStorageInterface');

this assertion is now failing?

I guess that's because in the eval() performed by the quoted assert, the namespace is no longer there, so it now needs the FQCN.

There is a logic error in the data-level check : it prevents creation of a bundle if one already exists, instead of just preventing renaming. Rerolling should remove a good number of fails.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -102,6 +71,43 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +        assert('$entity_storage instanceof \Drupal\Core\Entity\DynamicallyFieldableEntityStorageInterface');
   

   Let's open a follow-up for the assert, we haven't sorted out how we're using them in core properly yet.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -102,6 +71,43 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +        if ($entity_storage->hasData()) {
   

   I don't think we can only prevent this when the bundle has data - you could easily set up views filters, reference fields etc. that depend on the bundle without any actual data being added, and we're removing the mechanism for those things to update themselves in this patch. We should just prevent renames altogether.

@catch : regarding the second point, this is the point I addressed in the #71 patch vs the #69 version : it now checks getOriginalId() vs id(). Maybe your comment only applied to version 69 ?

It's still doing a hasData() check though?

I realized only recently that an issue with hasData() checks is that they can allow some config changes on your dev setup that uses a smaller / fresh db, and then break when deploying the config on prod on the actual full db.

And yeah, Field UI's FieldStorageConfigEditForm is also guilty of that :-/
(when the hasData() check is only done in the UI, the problem becomes : change was approved on dev, and deploys successfully on prod where the UI would have forbidden the change)

Fixes #72 and a few tests.

The rest of the failing tests can just be deleted.

1. +++ b/core/lib/Drupal/Core/Entity/BundleEntityFormBase.php
   @@ -0,0 +1,40 @@
   +    // No need to redo the check if the element is already disabled.
   +    if (empty($element['#disabled'])) {
   +      $element['#disabled'] = !$entity->isNew();
   +    }
   

   That sounds like incredible optimization which sacrifes code simplicity. I don't see a point why this additional functional call would be bad in any way

2. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -187,7 +187,8 @@ public function form(array $form, FormStateInterface $form_state) {
   -    return $form;
   +
   +    return $this->protectBundleIdElement($form);
      }
   

   I'm curious whether the wrapping code: \Drupal\Core\Controller\FormController::getContentResult would additionally call it as well, just to be sure? Contrib/custom code might have not the call in there yet.

+    if (empty($element['#disabled'])) {
+      $element['#disabled'] = !$entity->isNew();
+    }

If the element was disabled, but the entity wasn't new, wouldn't it undo whatever had previously disabled it if we didn't do that additional check? I don't think it's a micro-optimization, it's preventing setting #disabled to FALSE.

@amateescu : this is (mostly) not an optimization. This check is because there is one case where the #disabled property is already set to true (that's on nodetypeform depending on locked status), in which case we do not want (I think) to reset it to false if the entiry is new.

In NodeTypeForm::form():

     // ...
      '#disabled' => $type->isLocked(),
     // ...

@catch : I think that's what you are saying too ?

If the element was disabled, but the entity wasn't new, wouldn't it undo whatever had previously disabled it if we didn't do that additional check? I don't think it's a micro-optimization, it's preventing setting #disabled to FALSE.

Ah gotcha, so this is about enabling to disable it even in other cases.

A bit more reviews.

core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php:20
unused use statement: use Drupal\Core\Entity\DynamicallyFieldableEntityStorageInterface;

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -102,6 +71,35 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +      if (!empty($bundle_of)) {
   +        throw new ConfigNameException(t("The machine name of this @bundle cannot be changed.", [
   +          '@bundle' => $bundle_type->getLabel(),
   +        ]));
   

   IMHO we should simply not use t() for exceptions, its not a message intended to be shown for users.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   index 6bdde07..31a25a1 100644
   --- a/core/lib/Drupal/Core/Entity/entity.api.php
   
   --- a/core/lib/Drupal/Core/Entity/entity.api.php
   +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   

   I'm curious whether we should document somewhere in entity.api.php that you cannot rename bundles?

3. +++ b/core/modules/field_ui/field_ui.module
   diff --git a/core/modules/field_ui/src/Tests/EntityDisplayTest.php b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   index 53a8639..a4059d2 100644
   
   index 53a8639..a4059d2 100644
   --- a/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   
   --- a/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   +++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   
   +++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   +++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   @@ -300,9 +300,9 @@ public function testBaseFieldComponent() {
   

   There is a couple of code in \Drupal\field_ui\Tests\EntityDisplayTest::testDeleteBundle which refer to the node.type.article_rename, which no longer exists.

4. +++ b/core/modules/language/src/Tests/LanguageConfigurationElementTest.php
   @@ -152,39 +152,6 @@ public function testDefaultLangcode() {
   -  public function testNodeTypeUpdate() {
   ...
   -    $edit = array(
   -      'type' => 'article_2'
   -    );
   -    $this->drupalPostForm('admin/structure/types/manage/article', $edit, t('Save content type'));
   -    // Check that we still have the settings for the new node type.
   -    $configuration = ContentLanguageSettings::loadByEntityTypeBundle('node', 'article_2');
   -    $this->assertEqual($configuration->getDefaultLangcode(), 'current_interface', 'The default language configuration has been kept on the new Article content type.');
   -    $this->assertTrue($configuration->isLanguageAlterable(), 'The alterable language configuration has been kept on the new Article content type.');
   -    $this->assertEqual($configuration->uuid(), $uuid, 'The language configuration uuid has been kept on the new Article content type.');
   

   I'm curious whether we want to drop that entire test coverage, given that just the second part is about renaming bundles.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
@@ -102,6 +71,35 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   /**
+   * Acts on an entity before the presave hook is invoked.
+   *
+   * Used before the entity is saved and before invoking the presave hook.
+   *
+   * Ensure that config entities which are bundles of other entities cannot have
+   * their ID changed.
+   *
+   * @param \Drupal\Core\Entity\EntityStorageInterface $storage
+   *   The entity storage object.
+   *
+   * @throws \Drupal\Core\Config\ConfigNameException
+   *   Thrown when attempting to rename a bundle entity.
+   */
+  public function preSave(EntityStorageInterface $storage) {
+    // Only handle renames, not creations.
+    if (!$this->isNew() && $this->getOriginalId() !== $this->id()) {
+      $bundle_type = $this->getEntityType();
+      $bundle_of = $bundle_type->getBundleOf();
+      if (!empty($bundle_of)) {
+        throw new ConfigNameException(t("The machine name of this @bundle cannot be changed.", [
+          '@bundle' => $bundle_type->getLabel(),
+        ]));
+      }
+    }
+
+    parent::preSave($storage);
+  }

So this simply forbids renaming always, which handily avoids the problem @yched describes in #76.

Since @catch suggested this in #72.2

This is very strict, but also very simple to understand, and therefore very predictable. It completely removes a whole area of unpredictability. The negative impact is very limited (how often do people actually need to rename bundles?).

Therefore, once #91 is addressed, I think this is RTBC.

Will address remarks from #91.

Addressed remarks from #91.

1. Fixed.
2. Fixed.
3. Fixed.
4. Added back the tests. Instead of renaming the bundles, I am now updating them, by changing the label title, or the human readable name. It seems like this is less useful than before. Originally it was checking if the language configuration was present and unchanged on what is in effect a new bundle after the rename. Now it checks that the language configuration is unchanged after the bundle is changed. Still somewhat useful :)

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -102,6 +70,33 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +   * Used before the entity is saved and before invoking the presave hook.
   

   So there is not really a use-case for changing the bundle in presave, but should we consider calling parent::presave() at the top of the method instead of the bottom just in case to also protect against that? Or is that not possible for some other reason?

2. +++ b/core/lib/Drupal/Core/Entity/BundleEntityFormBase.php
   @@ -0,0 +1,40 @@
   +    // No need to redo the check if the element is already disabled.
   

   It might be worth changing the comment to clarify it's not an optimization - I'm not surprised dawehner read it like that.

   Or possibly instead:

   if (!$entity->isNew()) {
     $element['#disabled'] = TRUE;
   }
   

Otherwise looks great.

On it!

So this is rtbc now :)

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
@@ -102,6 +70,33 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
+   * Used before the entity is saved and before invoking the presave hook.

OK so for a second I thought this comment was no longer true, but then realised it still is:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

That's not for this patch to fix.

Committed/pushed to 8.0.x, thanks!

YEEEEHAAAAAH ! @all++

Finally... woowwwww

Seriously, !!!!!!!!!!!!!!!!!!!!!!!!!!!!!YES!!!!!!!!!!!!!!!!!!!!!!!!!!

https://www.youtube.com/watch?v=pmGNo8RL5kM !!!

Gratz everyone!

Woo-hoo!

Setting this back to "fixed" because https://www.drupal.org/project/issues/drupal?priorities=400&version=8.x needs to be ALL GREEN.