over in #1697256-141: Create a UI for importing new configuration, sun said:

+++ b/core/includes/config.inc
@@ -40,6 +56,28 @@ function config_install_default_config($type, $name) {
+function config_uninstall_default_config($type, $name) {
...
+  // If this module defines any ConfigEntity types, then delete the manifest
+  // file for each of them.
+  foreach (config_get_module_config_entities($name) as $entity_type) {
+    config('manifest.' . $entity_info['config_prefix'])->delete();
+  }

Uninstallation happens after disabling a module.

You cannot invoke any APIs in disabled modules. The entity info you're trying to retrieve no longer exists.

i think this is a valid bug? lets discuss and fix here.

Files: 
CommentFileSizeAuthor
#15 1831776-12-config-manifest-uninstall.patch15.49 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 49,282 pass(es). View
#12 10-12-interdiff.txt1.26 KBalexpott
#12 1831776-12-config-manifest-uninstall.patch15.49 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 2 fail(s), and 3 exception(s). View
#10 1831776-10-config-manifest-uninstall.patch14.09 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 1 fail(s), and 0 exception(s). View
#9 1831776-9-config-manifest-uninstall.patch14.09 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 49,360 pass(es). View
#8 6-8-interdiff.txt1.15 KBalexpott
#8 1831776-8-config-manifest-uninstall.patch12.75 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 49,285 pass(es). View
#6 4-6-interdiff.txt1.08 KBalexpott
#6 1831776-6-config-manifest-uninstall.patch12.14 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 47,857 pass(es). View
#4 1831776-4-config-manifest-uninstall.patch12.02 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 47,885 pass(es), 1 fail(s), and 0 exception(s). View
#2 1831776-2-config-manifest-uninstall.patch20.19 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 47,912 pass(es). View
#2 1831776-2-config-manifest-uninstall.test-only.patch3.28 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 47,910 pass(es), 2 fail(s), and 0 exception(s). View

Comments

beejeebus’s picture

Issue tags: +Configuration system

tag

alexpott’s picture

FileSize
3.28 KB
FAILED: [[SimpleTest]]: [MySQL] 47,910 pass(es), 2 fail(s), and 0 exception(s). View
20.19 KB
PASSED: [[SimpleTest]]: [MySQL] 47,912 pass(es). View

This patch changes the filename format of manifest files from manifest.config_prefix to module.manifest.config_prefix.

This has several happy outcomes:

  • The current config uninstall code will remove manifest files - no need for special manifest code
  • It's way easier to deploy config for one module as now all the relevant files start with module name
  • It ensures that all config entity files are prefixed with the module name
  • The Manifest project can use config :)

An aside: I'm definitely coming round to the idea that each module ought to have its own directory in the config directory this patch makes that easier.

Also attaching a patch with only the test to prove we have a problem. Unfortunately has to change the test from a DrupalUnitTestBase to WebTestBase as module_uninstall does not work in a DrupalUnitTestBase environment.

EDIT: Spelling mistake.

sun’s picture

Status: Active » Needs work
+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Plugin/Core/Entity/BreakpointGroup.php
@@ -21,7 +21,7 @@
- *   config_prefix = "breakpoint.breakpoint_group",
+ *   config_prefix = "breakpoint_group",

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Core/Entity/ConfigTest.php
@@ -24,7 +24,7 @@
- *   config_prefix = "config_test.dynamic",
+ *   config_prefix = "dynamic",

+++ b/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Category.php
@@ -24,7 +24,7 @@
- *   config_prefix = "contact.category",
+ *   config_prefix = "category",

+++ b/core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.php
@@ -20,7 +20,7 @@
- *   config_prefix = "image.style",
+ *   config_prefix = "style",

+++ b/core/modules/picture/lib/Drupal/picture/Plugin/Core/Entity/PictureMapping.php
@@ -27,7 +27,7 @@
- *   config_prefix = "picture.mappings",
+ *   config_prefix = "mappings",

+++ b/core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.php
@@ -28,7 +28,7 @@
- *   config_prefix = "views.view",
+ *   config_prefix = "view",

The config prefix should be fully qualified. Removing the owner part makes introduces unnecessary complexity down the line.

alexpott’s picture

Status: Needs work » Needs review
FileSize
12.02 KB
FAILED: [[SimpleTest]]: [MySQL] 47,885 pass(es), 1 fail(s), and 0 exception(s). View

I'm not particularly fussed but the change makes the manifest file names have quite a bit of redundancy. With the patch in #2 the config_test manifest files were called config_test.manifest.dynamic.yml now they called config_test.manifest.config_test.dynamic.yml.

@sun What complexities do you envisage down the line?

sun’s picture

This is the wrong issue to perform that change.

I consider the change as wrong, because the entity system is a separate system. The entity info/metadata contains a 'module' key already, and apparently, the proposed change tried to rely on that, but that is not a safe assumption to make.

Also, no one has addressed the fundamental flaws of the current manifest files yet that have been outlined in #1697256-141: Create a UI for importing new configuration

Until that happened, a manifest file should be a simple (as in: trivial) index that lists config object names.

+++ b/core/includes/config.inc
@@ -32,9 +32,9 @@ function config_install_default_config($type, $name) {
+      foreach ($source_storage->listAll($entity_info['module'] . '.' . $entity_info['config_prefix']) as $config_name) {

Stray 'module' concatenation.

alexpott’s picture

FileSize
12.14 KB
PASSED: [[SimpleTest]]: [MySQL] 47,857 pass(es). View
1.08 KB

Good catch... wonder if a test will catch that...

Improved the php doc of the config_get_module_config_entities() too

heyrocker’s picture

+++ b/core/includes/config.incundefined
@@ -56,12 +56,12 @@ function config_install_default_config($type, $name) {
+ *   The name of the module or theme to install configuration for.

Should be uninstall. Also sun pointed out in the UI issue that 'uninstall_default' doesn't really make sense, which is probably true (since you might be uninstalling config that wasn't supplied as a default). We could make that change here if we wanted.

+++ b/core/includes/config.incundefined
@@ -125,12 +119,21 @@ function config_sync_get_changes(StorageInterface $source_storage, StorageInterf
-  foreach ($source_storage->listAll('manifest') as $name) {
-    if ($source_manifest_data = $source_storage->read($name)) {
+  foreach (config_get_module_config_entities() as $entity_info) {
+    $manifest_name = $entity_info['module'] . '.manifest.' . $entity_info['config_prefix'];
+    $source_manifest_data = $source_storage->read($manifest_name);
+    // Empty arrays need to be merged as this would indicate that all config
+    // entities of this type are to be removed.
+    if ($source_manifest_data !== FALSE) {
       $source_config_data = array_merge($source_config_data, $source_manifest_data);

So I originally went with 'manifest.' for the filenames to make listing of all the manifest files easier. This is more work, but I do see the benefits as outlined by Alex above. If we don't have a more general use case for listing just manifest files, then I don't really see a problem with the rename. The repeated module name in the filenames is vaguely annoying but I'm hard pressed to get up in arms about file naming to be honest.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -255,7 +255,7 @@ public function delete(array $entities) {
+      config($this->entityInfo['module'] . '.manifest.' . $this->entityInfo['config_prefix'])

In some ways it seems stupid to have a helper function to generate these manifest file names, but on the other hand we do repeat this in quite a few places and who knows if we're done renaming them. Could go either way on it.

alexpott’s picture

FileSize
12.75 KB
PASSED: [[SimpleTest]]: [MySQL] 49,285 pass(es). View
1.15 KB

Updated patch with suggestions from @heyrocker

I don't think we should have a helper function for manifest file names.

alexpott’s picture

FileSize
14.09 KB
PASSED: [[SimpleTest]]: [MySQL] 49,360 pass(es). View

Changed my mind since we going to have to write manifest files in hook_update_N functions - this patch adds two helper functions config_manifest_name() and config_manifest()

alexpott’s picture

FileSize
14.09 KB
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 1 fail(s), and 0 exception(s). View

Reroll...

Status: Needs review » Needs work

The last submitted patch, 1831776-10-config-manifest-uninstall.patch, failed testing.

alexpott’s picture

FileSize
15.49 KB
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 2 fail(s), and 3 exception(s). View
1.26 KB

Fixing the config import test around name validation to work with the changes.

alexpott’s picture

Status: Needs work » Needs review

Go bot go!

Status: Needs review » Needs work

The last submitted patch, 1831776-12-config-manifest-uninstall.patch, failed testing.

alexpott’s picture

FileSize
15.49 KB
PASSED: [[SimpleTest]]: [MySQL] 49,282 pass(es). View

Reuploading #12 to keep the random upgrade failure around... for #1893800: Something is very, very wrong with update.php / upgrade tests... demons suspected

alexpott’s picture

Status: Needs work » Needs review
sun’s picture

+++ b/core/includes/config.inc
@@ -150,8 +182,9 @@ function config_sync_get_changes(StorageInterface $source_storage, StorageInterf
+    // Ignore manifest files which have the pattern
+    // module.manifest.config_entity.
+    if (!preg_match('/^[^.]+\.manifest\./', $name)) {

I'm still not happy with the manifest files, but for the time being, I wondered whether we should perhaps prefix them with a dot:

.manifest.module.type

That would list them first and make them "hidden". They're not part of a module's config, but instead, exclusively owned + maintained by the config system itself.

alexpott’s picture

The issue with using the . is that they'll be hidden for no reason when you issue an ls command on the config directory and we still won't remove them when a module is uninstalled - we'll have to do extra magic to discovered a disabled module's config entities.

beejeebus’s picture

pretty sure a leading '.' works against the whole reason for the manifest files existing.

they are there to make it easy to 'just publish my damn view', so they need to be easy for site-builders to see and copy around.

beejeebus’s picture

Status: Needs review » Closed (fixed)

manifests are dead, so this issue is too.