Problem

Follow-up to #1002164: The Book module can be uninstalled with nodes with a book node type still existing: If you locally delete the node.type.book config and push changes to a site with book nodes, it's going to delete the node type and break your book nodes. The problem exists generally for all content based upon an entity bundle that is defined in config.

Proposed resolution

Write a config import validator that checks for content before deleting an entity bundle.

Remaining tasks

  • Review
  • Commit

User interface changes

-

API changes

-

Files: 
CommentFileSizeAuthor
#23 2392351-2.23.patch8.51 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,839 pass(es). View
#23 19-23-interdiff.txt1.64 KBalexpott
#19 interdiff.txt2.24 KBswentel
#19 2392351-19.patch8.53 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,574 pass(es). View
#18 interdiff.txt4.13 KBswentel
#18 2392351-18.patch8.65 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,494 pass(es). View
#13 2392351.13.patch6.8 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,377 pass(es). View
#13 10-13-interdiff.txt3.03 KBalexpott
#10 2392351.10.patch6.8 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,375 pass(es). View
#10 6-10-interdiff.txt1.24 KBalexpott
#7 2392351.6.patch6.8 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,397 pass(es), 1 fail(s), and 0 exception(s). View
#7 4-6-interdiff.txt2.37 KBalexpott
#4 2392351.4.patch6.8 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,370 pass(es). View
#4 2-4-interdiff.txt3.83 KBalexpott
#2 2392351.3.patch5.74 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,200 pass(es). View

Comments

alexpott’s picture

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
5.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,200 pass(es). View

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.

yched’s picture

  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 ?

alexpott’s picture

FileSize
3.83 KB
6.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,370 pass(es). View

Re #3

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

Patch looks great, only found some nitpicks:

  1. +++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
    @@ -0,0 +1,79 @@
    +          if (!empty($entities)) {
    

    Minor but it are $ids, not $entities.

  2. +++ b/core/modules/config/src/Tests/ConfigImportUITest.php
    @@ -393,4 +393,31 @@ function testImportErrorLog() {
    +   * Tests that bundles cannot be deleted by the field importer when they used.
    

    they are used?

yched’s picture

#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 :-)

alexpott’s picture

FileSize
2.37 KB
6.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,397 pass(es), 1 fail(s), and 0 exception(s). View

@fago thanks for the review

  1. Fixed
  2. Fixed
fago’s picture

+++ b/core/lib/Drupal/Core/Entity/Event/BundleConfigImportValidate.php
@@ -63,13 +63,13 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
-            $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())));
+            $event->getConfigImporter()->logError($this->t('There are entity_ids 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())));

Looks like entities was wrongly replaced in the message with entity_ids.

Status: Needs review » Needs work

The last submitted patch, 7: 2392351.6.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
6.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,375 pass(es). View

alexpott--

yched’s picture

Typo in #6 still needs adressing :-)

Other than that, looks RTBC ?

swentel’s picture

+++ 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.

alexpott’s picture

FileSize
3.03 KB
6.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,377 pass(es). View

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

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Looks great.

Wim Leers’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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.

swentel’s picture

I'm quickly updating the test

swentel’s picture

Status: Needs work » Needs review
FileSize
8.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,494 pass(es). View
4.13 KB

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

swentel’s picture

FileSize
8.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,574 pass(es). View
2.24 KB

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.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs review
  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?

yched’s picture

@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.

alexpott’s picture

FileSize
1.64 KB
8.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,839 pass(es). View

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.
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: 2392351-2.23.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
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.

alexpott queued 23: 2392351-2.23.patch for re-testing.

  • catch committed 007eaf3 on 8.0.x
    Issue #2392351 by alexpott, swentel: When an entity bundle config gets...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.