Problem/Motivation

#1863512: Move default views into their respective module config directories wants to move views that are owned by other modules to their respective extension directories. For example, views.view.taxonomy_term.yml should be provided the taxonomy module and not views. Also if the taxonomy module is disabled then this view should be removed. At the moment views does this in the view storage controller. This is because the listAll() method on Config's FileStorage still returns disabled module's config. This means that disabled modules configuration is bleeding in the running site!

This issue somewhat related to: #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed

Proposed resolution

At DrupalCampLondon @swentel, @damiankloip and I discussed this issue at length. The solution we came up with is:

  • We need to introduce the concept of configuration owner into the configuration system
  • We can then use this to create directories within the config directory for each enabled owner
  • If we inject the module list into the config storage controller that we can then ignore directories of disabled modules.

This will also solve:

Remaining tasks

  • Inject the module list into the config storage controller that we can ignore directories of disabled modules.
  • This patch also exposes an existing issue with manifest synchronisation on module disable. However this is an existing issue.

API changes

@todo

Files: 
CommentFileSizeAuthor
#29 1932600.configuration-owner.29.patch93.65 KBjackbravo
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#28 1932600.configuration-owner.28.patch90.98 KBjackbravo
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#27 1932600.configuration-owner.27.patch90.18 KBjackbravo
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#22 19-22-interdiff.txt992 bytesalexpott
#22 1932600.configuration-owner.22.patch104.46 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1932600.configuration-owner.22.patch. Unable to apply patch. See the log in the details link for more information. View
#19 17-19-interdiff.txt11.43 KBalexpott
#19 1932600.configuration-owner.19.patch103.36 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 53,094 pass(es), 1 fail(s), and 1 exception(s). View
#17 1932600.configuration-owner.17.patch90.6 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 52,185 pass(es), 722 fail(s), and 348 exception(s). View
#11 Screen Shot 2013-03-04 at 18.49.00.png71.99 KBswentel
#1 1932600.configuration-owner.1.patch79.21 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
79.21 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Current patch

Status: Needs review » Needs work

The last submitted patch, 1932600.configuration-owner.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Configurables

Tagging

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1932600.configuration-owner.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

patch in #1 is failing due to [[SimpleTest]]: [MySQL] Drupal installation failed.

Install works just fine locally!

alexpott’s picture

#1: 1932600.configuration-owner.1.patch queued for re-testing.

alexpott’s picture

Title: Configuration names do not always begin with the extension » Configuration names do not always begin with the extension that actually owns the configuration

Improving title

Status: Needs review » Needs work

The last submitted patch, 1932600.configuration-owner.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

The failures are probably due to php version... thanks @berdir

swentel’s picture

Gave this a spin, I actually like the directory structure, it feels more sane to the active store directory (cf screenshot further down).

Two things I've noticed:

  • if you copy system.site.yml to your staging directory, the configuration screen will detect it but will not import it. I guess this can qualify as a bug.
  • if you move that file to a subdirectory 'system' (as it probably should), importing works fine. The file is gone, but the system directory remains. Not sure whether we want to remove this or not.

Hello directories :)

Screen Shot 2013-03-04 at 18.49.00.png

Some code comments.

+++ b/core/includes/config.incundefined
@@ -93,10 +91,11 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ *   This either be a string or an instance of \Drupal\Core\Config\ConfigName.

This 'can' either ..

+++ b/core/includes/config.incundefined
@@ -349,7 +366,7 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
+      if ($entity_type = config_get_entity_type_by_name((string) $name)) {

Just wondering why you cast to string here, I'm seeing this in a lot of places, can $name really be something else ?

+++ b/core/lib/Drupal/Core/Config/ConfigDirectoryFilterIterator.phpundefined
@@ -0,0 +1,47 @@
+ * Definition of Drupal\Core\Config\ConfigDirectoryFilterIterator.

We use 'Contains' nowadays right ?

+++ b/core/lib/Drupal/Core/Config/ConfigName.phpundefined
@@ -0,0 +1,86 @@
+ * not define. Also it allows the active file storage to be organised into

It 'also' allows ?

+++ b/core/lib/Drupal/Core/Config/EnableStorage.phpundefined
@@ -0,0 +1,110 @@
+ * Storage controller used by the Drupal when enabling extensions.

redundant 'the'

+++ b/core/lib/Drupal/Core/Config/StorageInterface.phpundefined
@@ -141,4 +142,14 @@ public function listAll($prefix = '');
+   * Gets the a directory if implemented by the storage engine.

redundant 'a' or 'the'.

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.phpundefined
@@ -39,15 +39,19 @@ public function __construct(StorageInterface $configStorage, StorageInterface $s
+   *   Configuration object name. Either a string on an instance of

'or' instead of 'on'

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.phpundefined
@@ -39,15 +39,19 @@ public function __construct(StorageInterface $configStorage, StorageInterface $s
+    if (!is_object($name)) {
+      $name = new ConfigName($name);

I'm generally not a fan being able to pass on mixed params. However, I haven't looked in depth whether it's solvable or not.

heyrocker’s picture

It seems to me that this issue is just a duplicate of the very very long and involved discussion that has already happened at #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API? A lot of conclusions have been reached there and I hope everyone has read and ingested all that.

Also, I'm against making any changes for the purposes of disabled modules until #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed gets resolved.

Finally, by moving config into subdirectories we remove the ability to sort all config by date changed, which is a very easy way to tell what config has changed recently or not. I'm not a big fan of that.

sun’s picture

IMHO all subdirectories should be (or must be) reserved for config context override files.

I originally asked that the Locale module overrides should be moved into and contained in respective ./locale.whatever-foo/ or ./language-foo subdirectories. Last time I looked, Locale still used config filename modifiers instead of subdirectories. We definitely need to move the overrides into subdirectories — otherwise, the whole thing becomes unmaintainable.

alexpott’s picture

#13 funnily enough this patch actually achieves (imo) a sensible approach to the locale issue.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.phpundefined
@@ -104,8 +105,8 @@ public function onKernelRequestSetDefaultConfigContextLocale(GetResponseEvent $e
+  public function getLocaleConfigName(ConfigName $name, Language $language) {
+    return new ConfigName('locale.config.' . $language->langcode . '.' . $name, $name->getOwner());

This means that locale files are in the same as the module that provides the config it is translating. Which means you translations are removed when you uninstall the module that provides the translated config.

xjm’s picture

This seems like it would simplify a number of other problems. It's both robust and simple. If we restrict it to a single subdirectory per module, I think it also is an acceptable increase in complexity usability-wise.

See also: #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API

xjm’s picture

Also, I'm against making any changes for the purposes of disabled modules until #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed gets resolved.

I'm not sure we can afford to wait on that.

alexpott’s picture

FileSize
90.6 KB
FAILED: [[SimpleTest]]: [MySQL] 52,185 pass(es), 722 fail(s), and 348 exception(s). View

Rerolled, fixed up the views suite of tests and made new config diff functionality work...

Now hopefully we've move 5.3.10 will run on the bots.

Status: Needs review » Needs work

The last submitted patch, 1932600.configuration-owner.17.patch, failed testing.

alexpott’s picture

FileSize
103.36 KB
FAILED: [[SimpleTest]]: [MySQL] 53,094 pass(es), 1 fail(s), and 1 exception(s). View
11.43 KB

Might be green...

alexpott’s picture

Status: Needs work » Needs review

Well.... it won't be green if I don'set to needs review...

Status: Needs review » Needs work

The last submitted patch, 1932600.configuration-owner.19.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
104.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1932600.configuration-owner.22.patch. Unable to apply patch. See the log in the details link for more information. View
992 bytes

Fixes the failing views test that is occurring now that \Drupal\views\Tests\ModuleTest has an additional dependency on the node module because it is using the views.view.archive view.

kerasai’s picture

Issue tags: -Configuration system

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1932600.configuration-owner.22.patch, failed testing.

dbcollies’s picture

Assigned: Unassigned » dbcollies
dbcollies’s picture

Assigned: dbcollies » Unassigned
jackbravo’s picture

Status: Needs work » Needs review
FileSize
90.18 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Hi guys, this is a re-roll of the patch. Tests still fail, but I thought it would be better to post it now than later (specially since I'm probably done for today). I can install drupal, config files are in their own directory and in general seems like things are going well, except on the tests =P.

This is an example test fail from running "sudo -u www-data php ./core/scripts/run-tests.sh Configuration":

exception 'Exception' with message 'The configuration directory type <em class="placeholder">staging</em> does not exist.' in /home/jackbravo/work/drupal-sites/git-drupal2/core/includes/bootstrap.inc:443
Stack trace:
#0 /home/jackbravo/work/drupal-sites/git-drupal2/core/modules/system/system.install(351): config_get_config_directory('staging')
#1 /home/jackbravo/work/drupal-sites/git-drupal2/core/includes/install.inc(1000): system_requirements('install')
#2 /home/jackbravo/work/drupal-sites/git-drupal2/core/includes/install.core.inc(2143): drupal_check_profile('testing', Array)
#3 /home/jackbravo/work/drupal-sites/git-drupal2/core/includes/install.core.inc(941): install_check_requirements(Array)
#4 /home/jackbravo/work/drupal-sites/git-drupal2/core/includes/install.core.inc(657): install_verify_requirements(Array)
#5 /home/jackbravo/work/drupal-sites/git-drupal2/core/includes/install.core.inc(517): install_run_task(Array, Array)
#6 /home/jackbravo/work/drupal-sites/git-drupal2/core/includes/install.core.inc(93): install_run_tasks(Array)
#7 /home/jackbravo/work/drupal-sites/git-drupal2/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php(782): install_drupal(Array)
#8 /home/jackbravo/work/drupal-sites/git-drupal2/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(741): Drupal\simpletest\WebTestBase->setUp()
#9 /home/jackbravo/work/drupal-sites/git-drupal2/core/scripts/run-tests.sh(484): Drupal\simpletest\TestBase->run()
#10 /home/jackbravo/work/drupal-sites/git-drupal2/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('30', 'Drupal\config\T...')
#11 {main}FATAL Drupal\config\Tests\ConfigEntityListTest: test runner returned a non-zero error code (1).
- Found database prefix 'simpletest349281' for test ID 30.

Sorry for not providing an interdiff, but I could not apply the last patch to go from there.

jackbravo’s picture

FileSize
90.98 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

I modified also core/lib/Drupal.php which like config.inc has a function config($name). So just a minor edit.

jackbravo’s picture

FileSize
93.65 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Ok. I had a permissions problem, and that's where most exception came from. The patch did have some errors, most related with some tests not being up to date with the code. I updated this tests and now I'm down to two fatal errors when running sudo -u www-data php ./core/scripts/run-tests.sh Configuration. And I don't think this errors are related to this patch :P.

So here is the new patch.

Status: Needs review » Needs work

The last submitted patch, 1932600.configuration-owner.29.patch, failed testing.

Cottser’s picture

Issue tags: +Needs reroll

Thanks @jackbravo!

Unfortunately this needs another reroll.

I think the diffstat might be a bit too different for the reroll in #29 as well. It might make sense to do another reroll from #22 - this may be a difficult reroll since it's a relatively large patch touching many files.

Patch in #22:
46 files changed, 791 insertions, 297 deletions.

Patch in #29:
47 files changed, 683 insertions, 214 deletions.

Gaelan’s picture

Assigned: Unassigned » Gaelan

Rerolling.

Gaelan’s picture

Assigned: Gaelan » Unassigned

I can't quite figure this one out either. :(

beejeebus’s picture

EDIT - removed unhelpful post.

xjm’s picture

See also: #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API

This might not be necessary if we resolve the above issue by using subdirectories or name parsing in the default config directories above, but I'm not 100% convinced of either of those solutions.

alexpott’s picture

Status: Needs work » Closed (won't fix)
alexpott’s picture

Issue summary: View changes

Fix up text