When an entity bundle config gets deleted, entities of that bundle break

Patch attached adds a event subscriber to check any delete of config entities that are bundles. If they are in use it prevent config import.

1. +++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
   @@ -0,0 +1,80 @@
   +          $entity_query = $this->entityManager->getStorage($bundle_of)->getQuery();
   +          $entities = $entity_query->condition($bundle_of_entity_type->getKey('bundle'), $id)
   +            ->accessCheck(FALSE)
   +            ->range(0, 1)
   +            ->execute();
   +          if (!empty($entities)) {
   +            $entity = $this->entityManager->getStorage($entity_type_id)->load($id);
   +            $event->getConfigImporter()->logError($this->t('There are entities of type %entity_type and %bundle_label %bundle existing. These need to be deleted before importing.', array('%entity_type' => $bundle_of_entity_type->getLabel(), '%bundle_label' => $bundle_of_entity_type->getBundleLabel(), '%bundle' => $entity->label())));
   +          }
   

   Don't we have a hadData() method for that already ?

2. +++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
   @@ -0,0 +1,80 @@
   +  public function onConfigImporterValidate(ConfigImporterEvent $event) {
   +    $deletes = $event->getConfigImporter()->getStorageComparer()->getChangelist('delete');
   

   Side note : that is a bit verbose :-)

   Accessing the list of changes is likely to be the primary task for any event class reacting to "config import". Could ConfigImporterEvent provide a helper ->getChangeList($op) method to simplify that ?
   Makes sense for a "config import" event to provide direct access to the list of changes in the import ?

Re #3

1. I don't think that is per bundle
2. I was thinking the same thing :)

#3.1 - ah, true, the existing one is not by bundle :-/
That could be easily added without an API break, though ?

#3.2 - awesome :-)

+++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
@@ -46,30 +46,29 @@ public function __construct(ConfigManagerInterface $config_manager, EntityManage
-        // What is this entity type a bundle of.
+        // Is this entity type define a bundle of another entity type.

Yay, I hesitated to suggest that change earlier on.
s/Is/Does, though :-)

@fago thanks for the review

1. Fixed
2. Fixed

alexpott--

Typo in #6 still needs adressing :-)

Other than that, looks RTBC ?

+++ b/core/modules/config/src/Tests/ConfigImportUITest.php
@@ -393,4 +393,31 @@ function testImportErrorLog() {
+    $validation_message = t('There are entities of type %entity_type and %bundle_label %bundle existing. These need to be deleted before importing.', array('%entity_type' => $node->getEntityType()->getLabel(), '%bundle_label' => $node->getEntityType()->getBundleLabel(), '%bundle' => $node_type->label()));

Maybe 'There are existing entities of type .. ' instead of existing at the end ? Sounds better in my mind.

Addressed #12 and #6. Thanks for all the reviews.

Looks great.

Beautiful patch: beautiful code & beautiful test.

Two nits that could be fixed on commit:

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporterEvent.php
   @@ -37,4 +37,23 @@ public function getConfigImporter() {
   +   * Gets the list of differences that will be imported.
   ...
   +  public function getChangelist($op = NULL, $collection = StorageInterface::DEFAULT_COLLECTION) {
   

   Nit: "list of differences" vs. "changelist".

2. +++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
   @@ -0,0 +1,79 @@
   +   * @param ConfigImporterEvent $event
   

   Nit: Needs fully qualified classname.

Actually, I think I'd love to see that test hardened a little bit, if possible. It was actually not obvious except from having swentel walk me through it why the presence or absence of the string "There are no configuration changes to import." meant things were working. And from looking at other tests in that file, most of them both assertText on the UI but also assertFoo to make sure things are working properly under the hood as well.

In other words:

+  public function testEntityBundleDelete() {
+    \Drupal::service('module_installer')->install(array('node'));
+    $this->copyConfig($this->container->get('config.storage'), $this->container->get('config.storage.staging'));
+
+    $node_type = $this->drupalCreateContentType();
+    $node = $this->drupalCreateNode(array('type' => $node_type->id()));
+    $this->drupalGet('admin/config/development/configuration');
+    // There will be one delete of the newly created node type.
# This doesn't actually assert that. It only asserts that there is something that needs importing. It seems like we should assert for a specific key/value, like in testImport().
+    $this->assertNoText(t('There are no configuration changes to import.'));
+
+    // Attempt to import configuration and verify that an error message appears.
+    $this->drupalPostForm(NULL, array(), t('Import all'));
+    $validation_message = t('Entities exist of type %entity_type and %bundle_label %bundle. These entities need to be deleted before importing.', array('%entity_type' => $node->getEntityType()->getLabel(), '%bundle_label' => $node->getEntityType()->getBundleLabel(), '%bundle' => $node_type->label()));
+    $this->assertRaw($validation_message);
# Here, should probably also assert that the content type continues to exist, since that's the actual bug we're fixing, no? 
+
+    // Delete the node and try to import again.
+    $node->delete();
+    $this->drupalPostForm(NULL, array(), t('Import all'));
+    $this->assertNoRaw($validation_message);
+    $this->assertText(t('There are no configuration changes to import.'));
# And here, that the content type no longer exists.
+  }

Apart from that, and the things Wim mentioned, this looks good to go.

I'm quickly updating the test

More asserts. Also address the first from Wim's review. Couldn't come up with something for 2 though ..

Gave the assert on '5 removed' another thought and removed it. It's kind of silly to be looking for this number, especially if at some point we change something in the generation of fields, displays etc. Having to change the number is not worth asserting for.

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporterEvent.php
   @@ -37,4 +37,23 @@ public function getConfigImporter() {
   +   * Gets the list of differences that will be imported.
   

   Extreme nitpick but why 'differences' and not just 'changes'?

2. +++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
   @@ -0,0 +1,79 @@
   +      if ($entity_type_id && ($entity_type = $this->entityManager->getDefinition($entity_type_id))) {
   

   Sorry don't love the assignment here - can we just move that to the line above too?

3. +++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
   @@ -0,0 +1,79 @@
   +          // Work out if there are entities with this bundle.
   

   This seems like something we'd probably want to re-use elsewhere too?

@catch:

+++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
@@ -0,0 +1,79 @@
+ // Work out if there are entities with this bundle.
This seems like something we'd probably want to re-use elsewhere too?

Made that same remark in #3 / #6. ContentEntityStorage::hasData() should have an optional $bundle param.

But IMO it would be better to do as part of or after #2391829: ContentUninstallValidator relies on the not required ContentEntityStorage::hasData() method (left a note in comment #12 over there), keeping the manual implementation here for now.

re #21

1. Fixed
2. Fixed
3. There is also NodeTypeDeleteConfirm which looking at closely looks wrong I think we should not be doing access checking there. Also the whole count thing seems superfluous.

Setting to rtbc since the changes in #23 are so minor

Drupal\taxonomy\Tests\TermTest	346	1	0
Message	Group	Filename	Line	Function	Status
Autocomplete returns term iVq4p9sl after typing the first 3 letters.	Other	TermTest.php	281	Drupal\taxonomy\Tests\TermTest->testNodeTermCreationAndDeletion()

random unrelated fail.

Avoiding the duplication of code in the other issue seems fine.

Committed/pushed to 8.0.x, thanks!