Problem

  • Despite huge efforts and spending entire weeks with working almost full time on the configuration system, the Drupal core process fails, and we're not making any substantial process at all.
  • We've tried to distill architectural changes into separate and isolated issues previously. But the isolation is inherently missing the big picture. People not only had a hard time to make sense of the isolated changes, but the individual change proposals itself were partially conflicting with each other.
  • At the current pace of individual issues, we would be done with the required re-architecture work at the time of feature freeze. But there's a huge stack of other efforts and issues that are blocked by the re-architecture work.

Goal

  • Replace the configuration system with a working implementation.

Notes

  • This change combines the essential roadmap changes into a single.
  • No random patches. Entire development happens exclusively in the CMI sandbox. The main branch is config-next-1626584-sun.

Combined issues

DONE: #1605324: Configuration system cleanup and rearchitecture
DONE: #1626398: Move the Configuration system to Dependency Injection
DONE: #1605236: Move synchronization methods from StorageInterface to DrupalConfig
#1624580: Research and implement a proper design for StorageDispatcher
DONE: #1447686: Allow importing and synchronizing configuration when files are updated
DONE: #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation
DONE: #1609418: Configuration objects are not unique
DONE: #1605338: Implement pluggable storage for configuration system
DONE: #1605124: Configuration system performance
DONE: #1505090: Clean up all (non-default) configuration (files) when a module is uninstalled
DONE: #1608912: (Re-)adding keys to configuration breaks the intended goal of being able to diff files

@todos

  1. Rename ConfigObject into just Config
  2. Rename StorageManager to StorageDispatcher
  3. #30: Remove hook_config_import_validate() and _error().
  4. FileStorage throws exceptions, while DatabaseStorage does not. Exceptions are not caught anywhere.
  5. Config objects need a better method of identifying whether any configuration data exists initially.
  6. Modules need a way to access the active store, whatever it is, without explicitly referencing DatabaseStorage.

@todos originally planned here, but deferred

  1. Configurable module thingies: Find a proper name. ;)
  2. Configurable module thingies: At minimum, introduce a consistent interface.
  3. Configurable module thingies: Consider to introduce a ConfigObjectBase that can be extended by all.
  4. Implement Drupal\image\Style object, make Image module API use it. Migrate image style rename/replacement style behavior.

Issues that won't be included here

#1624580: Research and implement a proper design for StorageDispatcher (#17 + #24 + #28)
#1605460: Implement dependencies and defer process for importing configuration
#1324618: Implement automated config saving for simple settings forms
#1552396: Convert vocabularies into configuration

CommentFileSizeAuthor
#130 config.next_.130.patch14.64 KBsun
#129 config.next_.129.patch64.66 KBsun
#128 config.next_.128.patch65.16 KBsun
#128 interdiff.txt502 bytessun
#123 config.next_.123.patch65.32 KBsun
#123 interdiff.txt27.51 KBsun
#121 config.next_.121.patch62.01 KBsun
#119 config.next_.119.patch67.15 KBsun
#117 config.next_.117.patch79.72 KBsun
#117 interdiff.txt5.5 KBsun
#115 config.next_.115.patch75.77 KBsun
#115 interdiff.txt24.3 KBsun
#113 config.next_.113.patch62.59 KBsun
#112 config.next_.112.patch64.6 KBsun
#112 interdiff.txt720 bytessun
#109 config.next_.109.patch63.9 KBsun
#109 interdiff.txt8.1 KBsun
#107 config.next_.107.patch55.99 KBsun
#107 interdiff.txt14.43 KBsun
#105 config.next_.105.patch41.88 KBsun
#105 interdiff.txt23.32 KBsun
#103 config.next_.103.patch36.71 KBsun
#103 interdiff.txt31.66 KBsun
#101 config.next_.101.patch74.32 KBsun
#101 interdiff.txt986 bytessun
#99 config.next_.99.patch73.36 KBsun
#99 interdiff.txt5.76 KBsun
#97 config.next_.97.patch72.38 KBsun
#94 config.next_.94.patch72.85 KBsun
#94 interdiff.txt3.99 KBsun
#92 config.next_.92.patch69.8 KBsun
#92 interdiff.txt4.47 KBsun
#86 config.next_.86.patch68.72 KBsun
#86 interdiff.txt1.99 KBsun
#84 config.next_.84.patch68.65 KBsun
#84 interdiff.txt16.29 KBsun
#83 config.next_.83.patch61.3 KBsun
#83 interdiff.txt4.13 KBsun
#82 config.next_.82.patch58.33 KBsun
#81 config.next_.81.patch47.73 KBsun
#80 config.next_.80.patch46.99 KBsun
#80 interdiff.txt3.56 KBsun
#79 config.next_.79.patch46.45 KBsun
#79 interdiff.txt1.17 KBsun
#78 config.next_.78.patch45.77 KBsun
#78 interdiff.txt9.46 KBsun
#77 config.next_.77.patch38.72 KBsun
#76 config.next_.76.patch39.34 KBsun
#76 interdiff.txt4.67 KBsun
#74 config.next_.74.patch40.54 KBsun
#72 config.next_.72.patch65.84 KBsun
#72 interdiff.txt11.89 KBsun
#71 config.next_.71.patch62.03 KBsun
#71 interdiff.txt29.3 KBsun
#70 config.next_.70.patch50.36 KBsun
#68 config.next_.68.patch50.06 KBsun
#68 interdiff.txt11.49 KBsun
#66 config.next_.66.patch44.57 KBsun
#66 interdiff.txt13.88 KBsun
#63 config.next_.63.patch37.93 KBsun
#63 interdiff.txt12.19 KBsun
#62 config.next_.62.patch33.66 KBsun
#60 config.next_.60.patch102.27 KBsun
#60 interdiff.txt1.76 KBsun
#59 config.next_.59.patch102.39 KBsun
#59 interdiff.txt6.64 KBsun
#57 config.next_.57.patch101.68 KBsun
#57 interdiff.txt27.96 KBsun
#55 config.next_.55.patch89.08 KBsun
#55 interdiff.txt10.6 KBsun
#53 config_next-51.patch5.13 KBJose Reyero
#52 config_next-50.patch10.81 KBJose Reyero
#49 config.next_.49.patch87.79 KBsun
#48 config.next_.48.patch87.91 KBsun
#45 config.next_.45.patch88.65 KBsun
#45 interdiff.txt6.43 KBsun
#43 config.next_.43.patch88.62 KBsun
#41 config.next_.41.patch88.22 KBsun
#41 interdiff.txt2.54 KBsun
#38 config.next_.38.patch96.39 KBalexpott
#38 35-38-interdiff.txt1.3 KBalexpott
#36 34-35-interdiff.patch2 KBalexpott
#36 config.next_.35.patch96.23 KBalexpott
#34 config.next_.34.patch88.36 KBsun
#34 interdiff.txt2.23 KBsun
#33 config.next_.33.patch86.16 KBsun
#33 interdiff.txt5.15 KBsun
#32 config.next_.32.patch85.06 KBsun
#32 interdiff.txt693 bytessun
#31 config.next_.31.patch84.39 KBsun
#31 interdiff.txt9.64 KBsun
#29 config.next_.29.patch90.28 KBsun
#29 interdiff.txt7.95 KBsun
#23 config.next_.23.patch88.72 KBsun
#23 interdiff.txt4.59 KBsun
#21 config.next_.21.patch86.89 KBsun
#21 interdiff.txt2.67 KBsun
#19 config.next_.19.patch86.59 KBsun
#19 interdiff.txt4.99 KBsun
#16 config.next_.16.patch83.93 KBsun
#16 interdiff.txt9.97 KBsun
#13 config.next_.12.patch75.04 KBsun
#13 interdiff.txt11.43 KBsun
#10 config.next_.10.patch72.37 KBsun
#10 interdiff.txt2.66 KBsun
#8 config.next_.8.patch70.25 KBsun
#8 interdiff.txt21.32 KBsun
#6 config.next_.6.patch68.01 KBsun
#6 interdiff.txt4.21 KBsun
#4 config.next_.4.patch65.85 KBsun
#4 interdiff.txt10.13 KBsun
#2 config.next_.2.patch59.22 KBsun
#1 config.next_.1.patch35.44 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Active » Needs review
FileSize
35.44 KB

#1605324: Configuration system cleanup and rearchitecture including DI container.

Obviously, this code lives in a CMI sandbox branch and will only developed in there. Will update the issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

FileSize
59.22 KB

Adding in #1447686: Allow importing and synchronizing configuration when files are updated in order to supply the first actual use-cases.

Introduces 1 test failure.

Not sure how to resolve the following — @Rob Loach to the rescue?

function config($name) {
  // @todo config needs to be a factory per name. Definition seems to support
  //   this somehow, but grepping the net didn't yield usable results.
  return drupal_container()->get('config')->setName($name)->load();
}

Status: Needs review » Needs work

The last submitted patch, config.next_.2.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system
FileSize
10.13 KB
65.85 KB

Incorporating #1505090: Clean up all (non-default) configuration (files) when a module is uninstalled

Includes fixes for the ill-informed test assumption that anything would write to files.

sun’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, config.next_.4.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.21 KB
68.01 KB

Cleaned up exceptions.

Status: Needs review » Needs work

The last submitted patch, config.next_.6.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
21.32 KB
70.25 KB

Added export operation, the natural counterpart to any import operation.

That is, to fix the import tests.

sun’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, config.next_.8.patch, failed testing.

sun’s picture

sun’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, config.next_.10.patch, failed testing.

gdd’s picture

Status: Needs work » Closed (won't fix)

There are several of the roadmap changes that are controversial, and the roadmap as a whole has no widespread support as far as I can tell. These issues need to be discussed individually and patches done incrementally. I'm sorry you're frustrated but its not going to work this way.

sun’s picture

Status: Closed (won't fix) » Needs review
FileSize
11.43 KB
75.04 KB

getNamesWithPrefix() has to be aware of the storage controller configuration.

sun’s picture

People can continue to discuss the individual changes in isolation.

There's little point in doing so, since the isolation is missing the big picture. Which, in turn, is the exact cause of the controversy in the first place.

This work already revealed a range of misconceptions stemming from individual issues. It will continue to do so.

Status: Needs review » Needs work

The last submitted patch, config.next_.12.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
9.97 KB
83.93 KB

Now getting to the actual core of architectural and systematic problems:

Added config_test module implementation of a "configurable thingie".

Bare essential module code only. Pretty much based on @merlinofchaos' comments elsewhere on how he'd like to use configuration objects/data for configurable thingies.

sun’s picture

Q: Why don't we remove the StorageManager and multiple storage madness entirely from the normal runtime code?
A: I'd love to. But dunno how. The StorageManager (ex DrupalConfig) + multiple storages behavior is only needed when performing import/export/sync operations. Regular runtime only needs an "active store". Resolving that sounds simple on the surface, but ConfigObject gets the StorageManager injected, and the ConfigObject's load/save/delete methods are actively calling into the manager to choose an appropriate storage engine.

Status: Needs review » Needs work

The last submitted patch, config.next_.16.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.99 KB
86.59 KB

Added basic functional tests for a configurable thingie, including essential basics for renaming a configurable thingie.

chx’s picture

<sarcasm>I am glad you read #1560060-18: [meta] Configuration system roadmap and architecture clean-up and acted accordingly.</sarcasm>.

sun’s picture

FileSize
2.67 KB
86.89 KB

Fixed combined expectations on delete behavior.

Typical issue that would take entire weeks to resolve through separate/isolated issues.

Status: Needs review » Needs work

The last submitted patch, config.next_.21.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.59 KB
88.72 KB

Implemented hook_config_import() for configurable test thingies.

Tests are incomplete. Got distracted by researching why only the 'delete' import operation fails. Still suspect a typo somewhere.

chx’s picture

#17 what about injecting the active storage (as it happens now) and use the FileStorage directly to import/export/sync. What stops us from doing this? We would not need $this->setParameter('config.storage.info' which lacks the information about what is active storage and what is file storage, we could just do $this->setParameter('config.storage.active and $this->setParameter('config.storage.file?

Status: Needs review » Needs work

The last submitted patch, config.next_.23.patch, failed testing.

webchick’s picture

What the what? I really don't understand why this was done, at all, let alone deliberately against Greg's request/recommendation. We need a real, actual issue summary here. Tagging.

webchick’s picture

Also, let's not forget we're a core development team here. Not an army of one. Especially an army of one who tears down others' work in the process of improving it. "Come for the software, stay for the community" isn't just something we say to feel good about ourselves. It's the basic underlying fundamentals of our community. http://drupal.org/dcoc

webchick’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Needs work » Needs review

@chx: That sounds like a good idea to me. At least worth to explore. I'm a bit worried about how the import/export code would look as a result of doing that, but alas, that's the exact cause and reason for combining the changes. ;) It also means we'd lock the architecture into two stores — effectively turning the $conf override into a permanent "hack" that isn't considered as a storage.

Since that is a relatively huge change in terms of architectural design, I will fork into a config-next-nomanager-1626584-[sun] branch, unless someone beats me to it.

sun’s picture

FileSize
7.95 KB
90.28 KB

But first:

- Fixed instantiation of ConfigObjects via new ConfigFactory.

- Fixed import does not catch storage exceptions.

sun’s picture

Q: Both the code and docs for hook_config_import_validate() and _error() is looking premature. Can we remove them?
A: I'd love to. Had serious troubles to even think of code, docs, and use-cases in the first place. In general, they rather map to #1605460: Implement dependencies and defer process for importing configuration and should probably be discussed in-depth in that issue. Unless someone disagrees, I'll remove them later.
sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

FileSize
9.64 KB
84.39 KB

Went ahead and implemented #30. This change can be reverted, if there's disagreement.

Also updated the issue summary with @todos extracted from the code and issue comments.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

FileSize
693 bytes
85.06 KB

Fixed straw unconditional loading of config.inc in bootstrap.inc.

sun’s picture

FileSize
5.15 KB
86.16 KB

- Completed ConfigImportTest for dynamic configuration.

- Fixed bogus $op conditions in hook_config_import() implementations.

sun’s picture

FileSize
2.23 KB
88.36 KB

Added ConfigObject "cache" to ConfigFactory, including elaborative notes for further research and performance profiling.

(The benchmark script that was used can be found here.)

Status: Needs review » Needs work

The last submitted patch, config.next_.34.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
96.23 KB
2 KB

I'm not sure that we need to cache every config object for each name. This patch suggests an alternative approach that only caches one configObject that might be more memory efficient.

My work is tracked in config-next-1626584-alexpott

Status: Needs review » Needs work

The last submitted patch, config.next_.35.patch, failed testing.

alexpott’s picture

The caching of the config object introduces issues for config_test_load(). Fixed by cloning the config object the loader retrieves.

alexpott’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

sun:

Can you please update the summary with reasons why you think the current architecture needs to be replaced, with an emphasis on how your revision corrects those problems?

I think I've read in other issues an explanation of your reasoning, but don't want to put words in your mouth.

sun’s picture

FileSize
2.54 KB
88.22 KB

#38 skips the DI container a bit too aggressively and the clone is basically equally "ugly" as the previous clone that was directly in config() since #16 and following. I've therefore reverted the premature caching of ConfigObjects in ConfigFactory entirely, but retained the comments and @todos. We still want and need to figure this out in order to fix the performance problem of the configuration system, but it makes more sense to re-think the architecture first. If the removal of StorageManager will work out, then it's possible that we might not need the ConfigFactory in the first place anymore. So let's defer that to later.

Status: Needs review » Needs work

The last submitted patch, config.next_.41.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
88.62 KB

Too many changes in HEAD, so had to merge (and move config-next-base).

RobLoach’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.phpundefined
@@ -27,5 +29,31 @@ class ContainerBuilder extends BaseContainerBuilder {
+      ->addArgument(new Reference('service_container'));
+    // @todo Obsolete?
+    $this->register('config', '%config.object%')
+      ->addArgument(new Reference('config.manager'));

Yeah, it looks like we're not using the "config" service anywhere.

Hmmm, ConfigFactory is the only place that uses 'config.object', which is just a wrapper around the Container, which seems interesting. Instead of passing the Container into the ConfigFactory, the ConfigFactory's constructor should take the ConfigObject class name and the ConfigManager instance. We want to send objects their dependencies to reduce our dependency on refering to the container all the time.

public function __construct($config_object_class, $config_manager_class) {
  $this->configObjectClass = $config_object_class;
  $this->configManagerClass = $config_manager_class;
}

Then we don't have to refer to the container from ->get() as we already have all the dependencies we need. Should I post a patch in one of your sandboxes? To pass them into the constructor during instantiation, you'd refer to the parameters during the ->register() call.

sun’s picture

Status: Needs work » Needs review
FileSize
6.43 KB
88.65 KB

b175f35 Removed ContainerBuilder dependency from ConfigFactory.
183e987 Renamed ConfigObject into ConfigContainer.

Status: Needs review » Needs work

The last submitted patch, config.next_.45.patch, failed testing.

alexpott’s picture

The patch no longer applies because #1589174: Configuration upgrade path is broken has landed... and this mega patch also contains it :)

sun’s picture

Status: Needs work » Needs review
FileSize
87.91 KB

Merged in HEAD.

sun’s picture

FileSize
87.79 KB

Renamed ConfigContainer into Config due to way too many confusion and mistaking it as DI container within a single day of discussions.

Status: Needs review » Needs work

The last submitted patch, config.next_.49.patch, failed testing.

Jose Reyero’s picture

Related patch, that adds Plugins and Storage realms on top of this, #1646580: Implement Config Events and Listeners, and storage realms for localized configuration

Jose Reyero’s picture

FileSize
10.81 KB

The patch in #49 has some issue with the file name that contains the class, renamed DrupalConfig.php to Config.php

Jose Reyero’s picture

FileSize
5.13 KB

This is a patch that tries to simplify the previous one and also changes StorageManager so it can be really used for import / export, which also allows us to move some of these related functions into the class.

Some bits of the patch:

- Really simplified dependency injection:

    $this->register('config.storage', 'Drupal\Core\Config\DatabaseStorage');
    $this->register('config.factory', 'Drupal\Core\Config\ConfigFactory')
      ->addArgument(new Reference('config.storage'));

- Reworked constructors so there's more encapsulation and we save lines of code. Example of export operation:

config_export_manager()->synchronize();

- Simplified config_import() too, though I'd really get rid of that hooks for this patch and let them for later...

- Simplified Config::read() and Config::write(), we really don't need complex logic to get hold of the storage interface.

gdd’s picture

Status: Needs work » Closed (won't fix)

At Drupal Dev Days in Barcelona, it was discussed that while we need separate issues and patches in order to isolate changes into reviewable chunks, there is also value in maintaining a single branch / ongoing patch that contains the big picture of how the various rearchitecture efforts are interacting. However, we already have a place for that, the original roadmap issue at #1560060: [meta] Configuration system roadmap and architecture clean-up. Therefore this issue is being closed, and further overarching patches will go into the roadmap issue.

gdd’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Title: Replace the configuration system » [test] Combine configuration system changes to verify they are compatible
Status: Closed (won't fix) » Needs review
FileSize
10.6 KB
89.08 KB

Hm. Actually, I think it would be beneficial to keep the meta issue about high-level discussions. If we start to throw patches in there in the same way as here, then that will pretty much break and hi-jack the discussion thread.

Slowly but certainly preparing for extracting the basic architectural changes into #1605324: Configuration system cleanup and rearchitecture... In this patch:

ae96279 Simplified service registration in ContainerBuilder.
d139080 Fixed fatal error in early Drush bootstrap.
866235b Renamed StorageManager into StorageDispatcher.

sun’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, config.next_.55.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
27.96 KB
101.68 KB

Now, after cleaning up docs and stuff, the next best major @todo in the issue summary, the consistent handling of errors, exceptions, and edge-cases in all storage controllers, was anything but trivial, and required to (finally) add dedicated/isolated tests for storage controller CRUD operations.

This @todo was originally triggered by the import and export operation changes that are combined in this patch, since those are accessing the storage controllers in unusual ways.

Adding these tests also revealed that getNamesWithPrefix() is a very odd method name on the storage controllers. The $prefix argument is optional, and one of the primary use-cases of that method is to actually list all config object names in the storage (bin). Limiting the list to a certain prefix only is a special case, not the common case. I've therefore renamed the method to just listAll(). [list() is not possible, since it's a reserved keyword/language construct in PHP.]

In turn, that rename results in storage controller method functionality that actually resembles Create, Read, Update, Delete, and List.

So, in this patch:

ec3296e Fixed missing and improper phpDoc and unnecessary imports.
a7c91f6 Relocated read/write/delete methods in FileStorage to match interface.
cde4e87 Fixed stale StorageDispatcher service name in config_import_invoke_sync_hooks().
124cfe1 Fixed bogus thrown exceptions and documented storage controller return values and which exceptions and errors are actually thrown.
1e61723 Renamed getNamesWithPrefix() into listAll().
65723d1 Added unit tests for storage controller CRUD operations.

Note that both storage controllers should actually throw (various) exceptions on different edge-cases, but I wasn't able to trigger those exceptions in the added tests (yet). Although this is technically incomplete, I do not plan to complete/fix that test coverage right now, as it is very important to get the basic refactoring out of the way.

Status: Needs review » Needs work

The last submitted patch, config.next_.57.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
6.64 KB
102.39 KB

Totally forgot to mention that the new tests assert expectations, which are not met by the storage controllers yet. ;)

eba99fb Fixed functional code for expectations on invalid storage controller options.

sun’s picture

FileSize
1.76 KB
102.27 KB

172f09d Fixed stale references to "container" and unnecessary use statements.

(Discovered during extraction for #1605324: Configuration system cleanup and rearchitecture)

sun’s picture

Plain re-roll for #1605324: Configuration system cleanup and rearchitecture

As visible, most of the remaining changes are about #1447686: Allow importing and synchronizing configuration when files are updated.

Before moving ahead with that import mechanism though, we first need to find a common denominator for #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation

sun’s picture

FileSize
33.66 KB

d'oh.

sun’s picture

FileSize
12.19 KB
37.93 KB

e4a2c10 Fixed module API hooks for thingies are not invoked on module installation.
(#1609760: hook_image_style_*() is not invoked for image styles upon Image module installation)

Status: Needs review » Needs work

The last submitted patch, config.next_.63.patch, failed testing.

sun’s picture

LOL! Finally figured out what the problem of #64 is ;)

"Obviously" the import mechanism involves a full synchronization, so for each installed module, a full diff is performed, adding the new config of the installed module, but deleting any and all config that existed before :-D Nice! :)


Also, FWIW, the very first debug() within default config install function shows a "change" for system.cron, which looks highly suspicious to me:

array (
  'create' => 
  array (
    1 => 'system.performance',
    2 => 'system.rss-publishing',
  ),
  'change' => 
  array (
    0 => 'system.cron',
  ),
  'delete' => 
  array (
  ),
)
sun’s picture

Status: Needs work » Needs review
FileSize
13.88 KB
44.57 KB

7790497 Clarified namespace/owner callback architecture by renaming functions and cleaning up docs.

9f06a38 Introducing Config::isNew() and NullStorage.

Status: Needs review » Needs work

The last submitted patch, config.next_.66.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.49 KB
50.06 KB

6ef6442 Implemented Config::isNew() in module APIs.
4b660b8 Fixed improper handling of FALSE return value of storage controllers. Added test coverage for isNew().

Status: Needs review » Needs work

The last submitted patch, config.next_.68.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
50.36 KB

301e5ea Fixed fatal error in image_style_load().
565124f Fixed stale references to DrupalConfig.
a0bc28e Moved config_test module into tests subdirectory.

sun’s picture

FileSize
29.3 KB
62.03 KB

e20b52b Copied ConfigTest into Drupal\Core\ConfigThingie\ConfigThingie.
7f20970 Added generic ConfigThingie with abstract base class and interface.
6b92dac Renamed ConfigThingie::load() into ::setConfig(). Added type-hinting, docs, and cleaned up implementation.

sun’s picture

FileSize
11.89 KB
65.84 KB

4aca5c4 Removed $op from MODULE_config_import() callback.
24b3b35 Added docs for NullStorage.

Status: Needs review » Needs work

The last submitted patch, config.next_.72.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
40.54 KB

Merged latest 8.x into config-next.

Status: Needs review » Needs work

The last submitted patch, config.next_.74.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
39.34 KB

9436165 Removed obsolete sync hook invocation test.
e3c1dda Removed base class specific methods from ConfigThingieInterface.

sun’s picture

FileSize
9.46 KB
45.77 KB

756dd4d Added tests for import/export UI.

sun’s picture

FileSize
1.17 KB
46.45 KB

9dabce4 Fixed config import screen deletes all configuration initially (stop-gap UX fix).

sun’s picture

FileSize
3.56 KB
46.99 KB

89dd92d Updated ConfigImportUITest::testImportLock() for new empty source storage behavior.
2c80423 Added assertions for new empty source storage UI behavior.

sun’s picture

FileSize
47.73 KB

Re-rolled against HEAD.

sun’s picture

sun’s picture

Priority: Normal » Critical
FileSize
4.13 KB
61.3 KB

I'm afraid, but I see no other way to get back to this issue and make it real again. There are conflicting change proposals in other issues.


We've decided to remove the StorageDispatcher. And replace its functionality with a new MultiStorage controller that simply wraps multiple other storage controllers to perform CRUD operations in multiple storages instead of one.

The concrete use-case for the MultiStorage controller is the idea of (re-)enabling "concurrent" (dispatched) writes/deletes to both the DatabaseStorage and FileStorage. Developers should be able to enable/configure that behavior; e.g., when working on local development sites on which all potential configuration changes will be exported anyway in the end, so they can be staged "upstream".

As such, the MultiStorage controller is suitable as the default active store on a development site. But not on a site that is supposed to import configuration. That is, because writing to both DatabaseStorage and FileStorage inherently conflicts with the idea of having differences between current and exported configuration. The moment you write, you eliminate any possible differences.

Furthermore, all hell breaks lose if you write to the FileStorage during an import.

Nevertheless, we want the active storage to be pluggable. This means that it is defined as a service on the Container and all code only ever refers to the service definition, never to e.g. DatabaseStorage directly. The service definition essentially enables developers to use the MultiStorage controller instead of the DatabaseStorage controller.

Making that service definition swappable is going to get a bit tricky, since it essentially is the lowest of low level configuration; i.e., configuration for the configuration system itself. We usually use global variables in settings.php for that kind of stuff, but I can already hear people screaming... ;) A potential alternative to that would be a single settings.yml configuration file, which would require us to always instantiate the FileStorage controller in order to load it on every request. That, with the potential goal of making these lowest low level settings configurable from the UI.

Anyway, the critical part is that we most likely need to hard-code a special condition for the MultiStorage controller into the config import mechanism, so as to deny execution, if it is configured as the active store.

The compatibility of all these changes has to be verified first. This investigation essentially blocks all the other issues, because not even I would have any idea where we're architecturally heading with the config system otherwise. Hence, bumping to critical.

Thus, kick-starting:

619f236 Added MultiStorage controller to enable "concurrent" (dispatched) writes to DatabaseStorage and FileStorage.

sun’s picture

Title: [test] Combine configuration system changes to verify they are compatible » Combine configuration system changes to verify they are compatible
FileSize
16.29 KB
68.65 KB

Configurable thingies are still vaporware at this point, so we don't have interdependencies yet. It is going to get hairy to write a test which essentially confirms that the MultiStorage does break the config import mechanism. I will try to come up with something very hacky, but focused on detecting that problem.

Otherwise, this seems to orchestrate better than I had imagined - even though the overall changes are quite invasive so far already;

1d4c486 Hardened ConfigImportTest to take MultiStorage into account.
ffdf235 Moved ::exists() into StorageInterface.
3048ca1 Replaced all direct references to DatabaseStorage with config.storage service.
9bd8c71 Fixed module API hooks for config objects are not invoked on module uninstallation.

Didn't run all tests. Let's see what the testbot thinks.

Status: Needs review » Needs work

The last submitted patch, config.next_.84.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
68.72 KB

58b0cb2 Fixed EnableDisableTest failures.
6ee9bfe Fixed bogus config storage service retrieval in config UI callbacks.

catch’s picture

We usually use global variables in settings.php for that kind of stuff, but I can already hear people screaming... ;)

Not me. If it's good enough for the database and cache, it's good enough for CMI. If we eventually find a better way to do things that performs well that's fine but that can be its own issue.

gdd’s picture

I completely agree with catch on that one.

webchick’s picture

Ditto.

sun’s picture

Until we've figured this out, there are some smaller issues that really could use your attention in the meantime:

#1701014: Validate config object names
#1671198: Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor

sun’s picture

Priority: Critical » Normal
sun’s picture

FileSize
4.47 KB
69.8 KB

27b5566 Added CachedFileStorage controller; using files as canonical storage and another "active store" as cache.

Status: Needs review » Needs work

The last submitted patch, config.next_.92.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
3.99 KB
72.85 KB

Just an investigation based on wild ideas and discussions with @beejeebus ;)

3eb34fb Added CacheStorage to leverage existing Cache subsystem for file storage cache.

Status: Needs review » Needs work

The last submitted patch, config.next_.94.patch, failed testing.

alexpott’s picture

I added a cache storage layer in #1605124: Configuration system performance (really old version of config alert!). It worked great and one of the obvious benefits it that config will then be able to use memcache / redis etc... with no extra work.

The one issue of the approach are the chicken/egg situations in early bootstrap, early install, and update d7 to d8.

sun’s picture

Status: Needs work » Needs review
FileSize
72.38 KB

Merged HEAD.

Status: Needs review » Needs work

The last submitted patch, config.next_.97.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
5.76 KB
73.36 KB

Now... an essential change to verify and enable my resolution proposal for #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs

e415d18 Removed obsolete todos.
c4e9013 Introducing config.state to enable CachedFileStorage and configuration staging to work as intended.

Status: Needs review » Needs work

The last submitted patch, config.next_.99.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
986 bytes
74.32 KB

003226d Fixed SearchExcerptTest cannot be a unit test.

Status: Needs review » Needs work

The last submitted patch, config.next_.101.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
31.66 KB
36.71 KB

cd64db5 Removed all Configurable thingie/entity changes.

Architectural design and implementation for Configurables will continue in
#1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc)

However, thus far, those changes did not appear to be relevant for this combined patch (as well as spin-off branches + extracted patches), which apparently is in line with that issue's summary, which already states that Configurables are not part of the configuration system. Nevertheless, it was good to have them in the mainline until now.

Status: Needs review » Needs work

The last submitted patch, config.next_.103.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
23.32 KB
41.88 KB

Alrighty. Some massive changes to incorporate latest directions from #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs

:)

Added separate config/active and config/import directories.
Removed obsolete MultiStorage.
Fixed Container of parent site executing a test is leaking into tests.
Fixed ConfigImportTest hard-codes bogus configuration storage controllers.
Clarified variables and documentation for CachedFileStorage and CacheStorage.
Fixed bogus variable names in ConfigImportTest.
Added proper 'config' cache bin.
Removed obsolete 'config' database table.
Updated ConfigImportUITest for latest core changes.
Fixed CacheStorage can return bogus config object data.
Removed orphan/obsolete config_test.delete default config object.

Status: Needs review » Needs work

The last submitted patch, config.next_.105.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
14.43 KB
55.99 KB

Ahem... so this turned out to be quite the challenge... ;)

Configuration is used and required all over the place in the installer already.

The fact that we're currently able to install Drupal at all is apparently caused by a full stack of "coincidences" and special cases that have been implanted into various subsystems in order to be compatible with the incomplete installer environment. Most of them kicked in, because they assume that the installer wants to talk to a non-functional database. So when switching to files, none of them apply anymore.

Fixed Cache\DatabaseBackend depends on procedural functions in includes/database.inc; and ::checksumTags() throws fatal error if database is not available.
Fixed Drupal installer cannot be fully tested in a separate sub/multi-site.
Fixed theme_task_list() does not use installer-compatible get_t().
Added InstallStorage to resolve fatal errors and misbehaviors during installation due to missing config directory.

Don't worry... I'll create separate issues for all the non-config stuff that had to be fixed here. (I'll edit this comment to inject the #issues later)

Status: Needs review » Needs work

The last submitted patch, config.next_.107.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.1 KB
63.9 KB

Fixed Drupal\config\Tests\Storage\DatabaseStorageTest (technically obsolete though).
Fixed Drupal\config\Tests\ConfigFileContentTest hard-codes DatabaseStorage.
Fixed UpgradePathTestBase::performUpgrade() does not always load all new modules after upgrade.
Fixed {cache_config} table is not created when upgrading from D7.

Status: Needs review » Needs work

The last submitted patch, config.next_.109.patch, failed testing.

sun’s picture

FileSize
720 bytes
64.6 KB

Fixed ConfigCRUDTest hard-codes DatabaseStorage.

sun’s picture

Status: Needs review » Needs work

The last submitted patch, config.next_.113.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
24.3 KB
75.77 KB

Properly merged in HEAD and latest changes from #1741804: Implement a config import/staging directory, which caused quite some "havoc"...

Merged config-directories branch.
Fixed bogus t() in theme_task_list().
Added new StorageInterface::rename() methods to new config storage controllers.
Fixed watchdog() triggers fatal error in Drupal installer.
Updated for CACHE_PERMANENT constant moved into CacheBackendInterface.
Fixed merge conflicts.

Status: Needs review » Needs work

The last submitted patch, config.next_.115.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
5.5 KB
79.72 KB

Fixed public StorableBase::$isCurrentRevision property leaks into config objects.
Fixed ConfigImportTest and ConfigImportUITest for default properties of dynamic ConfigTest entities.

Status: Needs review » Needs work

The last submitted patch, config.next_.117.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
67.15 KB

Merged in latest HEAD.

Still contains "config.state" as a service name. I hope we can figure out a proper/final name in #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs ASAP.

Status: Needs review » Needs work

The last submitted patch, config.next_.119.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
62.01 KB

Merged in HEAD.

Will try to finalize + extract this patch today.

Status: Needs review » Needs work

The last submitted patch, config.next_.121.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
27.51 KB
65.32 KB

Fixed HookBootExitTest fails due to added module_load_all() check in watchdog().
Renamed config.storage to config.storage.active, and config.state into config.storage.staging.
Fixed phpDoc, variable names, and code comments.

Status: Needs review » Needs work

The last submitted patch, config.next_.123.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

Hm. I'm not able to reproduce that Drupal installation error (tested installation in a subdirectory).

sun’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Configuration system

The last submitted patch, config.next_.123.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
502 bytes
65.16 KB

Fixed PHP warning "The use statement with non-compound name 'Exception' has no effect."

sun’s picture

FileSize
64.66 KB

Essential changes have been extracted into #1702080: Introduce canonical FileStorage + (regular) cache

However, test failures over there. If those are true, then this merge/re-roll should fail as well.

sun’s picture

FileSize
14.64 KB

#1702080: Introduce canonical FileStorage + (regular) cache landed, which means that the main architectural changes are done.

Attached patch shows the remnants of the config-next branch.

sun’s picture

Status: Needs review » Fixed
Issue tags: -Needs issue summary update

The remaining changes will be dealt with in:

1) phpDoc fix for FileStorage: #1702080: Introduce canonical FileStorage + (regular) cache

2) Consider using config import mechanism for uninstalling module config: #1765936: Invoke ConfigStorageController::delete on module uninstall and add tests

3) Configuration module UI: #1697256: Create a UI for importing new configuration

4) And also: #1760786: Move entity system "back" into a Drupal\Core component

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.