Problem

  • Each call to config() instantiates a new DrupalConfig object, and entirely new storage controllers, even though the storage controllers do not change and the behavioral logic of DrupalConfig does not change either; only the configuration object/data is different, but the object/data is not cleanly separated.
  • The current DrupalConfig object is a mixture of functional "storage arbitrator/manager" logic and an attempt to represent the configuration object/data itself at the same time.
  • The StorageInterface and DatabaseStorage are currently implementing methods, which enforce concurrent write operations to the database and file system. This logic does not belong into a storage controller; each controller should only care for its own storage.
  • The current, parallel writes to the file system fundamentally break the intention of being able to import/export/synchronize configuration between the active store and file system.
  • The code contains further synchronization methods, which are not used anywhere, and which won't be used by the actual synchronization implementation either.

Goal

  • Cleanly separate the actual configuration object/data from the DrupalConfig storage manager as well as storage controllers.
  • Create a ConfigObject class, which maps 1:1 to a configuration object. Return that from config() and load each of those only once per request.
  • Instantiate DrupalConfig and storage controllers only once per request.
  • Clean up the StorageInterface to only contain actual storage controller methods.
  • Remove the automated concurrent/parallel writes to multiple storages, since they would have to be re-implemented differently, but they ultimately conflict with the actually envisioned import/export/sync operation in the first place.
  • Remove the premature synchronization methods.
  • Implement a StorageManager class which arbitrates which storage controllers should act on each operation.

Existing Architecture

New Architecture


Original summary by @beejeebus:

currently, we create a new DrupalConfig object for each call to config().

this is slow and memory eating. we should:

  • only load DrupalConfig once per request, and make DrupalConfig a site-scope object
  • create a ConfigObject class, which maps one-to-one per config file, and only load each of those once per request

Follow up issues

CommentFileSizeAuthor
#89 config.object.89.patch69.16 KBsun
#89 interdiff.txt2.43 KBsun
#83 config.object.83.patch64.57 KBsun
#83 interdiff.txt9.89 KBsun
#77 interdiff.47-67.txt60.94 KBsun
#71 1605324-71.patch63.63 KBJose Reyero
#69 1605324-70.patch63.62 KBJose Reyero
#67 config.object.67.patch68.88 KBsun
#58 1605324-58.patch47.94 KBJose Reyero
#58 interdiff.txt17.4 KBJose Reyero
#55 1605324-config_reachitecting_simplified.patch8.37 KBJose Reyero
#47 1605324_47.patch45.41 KBchx
#47 interdiff.txt1.85 KBchx
#45 1605324_45.patch45.41 KBchx
#43 bench_functional.php_.txt2.21 KBsun
#41 interdiff.txt7.3 KBsun
#41 config.object.41.patch42.6 KBsun
#39 interdiff.txt3.69 KBsun
#39 config.object.39.patch40.09 KBsun
#38 config.object.38.patch39.15 KBsun
#38 interdiff.txt1.15 KBsun
#37 config.object.36.patch38.97 KBsun
#37 interdiff.txt6.59 KBsun
#36 1605324_36_configobject.patch39.77 KBalexpott
#36 34-36-interdiff.txt601 bytesalexpott
#35 interdiff.27-34.DrupalConfig-rename-reverted.txt7.98 KBsun
#34 1605324_34_configobject.patch39.78 KBalexpott
#34 29-30-interdiff.txt909 bytesalexpott
#33 config_memory_test.php_.txt1.42 KBalexpott
#30 config.object.30.patch40.89 KBRobLoach
#30 config.object.30-innerdiff.patch2.36 KBRobLoach
#29 1605324.patch39.96 KBchx
#27 config.object.27.patch38.32 KBsun
#25 config.object.25.patch38.32 KBsun
#21 config.object.21.patch35.39 KBsun
#18 config.object.18.patch35.2 KBsun
#10 config.object.10.patch34.26 KBsun
#10 interdiff.txt6.81 KBsun
#8 config.object.8.patch28.78 KBsun
#4 1605324.patch14.28 KBRobLoach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

PoC code from https://gist.github.com/2822341:


function config($name, $config_class = NULL) {
  static $config_instances = array(), $storage_instances = array();

  // Set defaults.
  if (!isset($config_class)) {
    $config_class = 'Drupal\Core\Config\DrupalConfig';
  }
  if (!isset($storage_class)) {
    $storage_class = 'Drupal\Core\Config\DatabaseStorage';
  }

  // Instantiate objects, if required.
  if (!isset($config_instances[$config_class])) {
    $config_instances[$config_class] = new $config_class;
  }
  if (!isset($storage_instances[$storage_class])) {
    $storage_instances[$storage_class] = new $storage_class;
  }

  return $config_instances[$config_class]->getConfigObject($name, $storage_instances[$storage_class]);
}

sun’s picture

Priority: Normal » Major
Issue tags: +Configuration system

gist in #1 is a 404 for me, but

https://gist.github.com/2822857

still seems to be the latest version of @beejeebus and my collaborative edits.

Also, testability of the config system is in no way affected by these changes.

RobLoach’s picture

Status: Active » Needs review
FileSize
14.28 KB

This moves the set of Drupal configs to the Container in a ConfigFactory. The API stays the same, but this allows us to keep track of all configs, as well as use one storage for all different config objects. The Container definition looks like this:

    // Register the Drupal Config service. The parameters, which are
    // "drupal.config.factory", "drupal.config.class" and
    // "drupal.config.storage" can be changed to alter the way the Drupal Config
    // system operates.
    $this->setParameter('drupal.config.factory', 'Drupal\\Core\\Config\\ConfigFactory');
    $this->setParameter('drupal.config.class', 'Drupal\\Core\\Config\\DrupalConfig');
    $this->setParameter('drupal.config.storage', 'Drupal\\Core\\Config\\DatabaseStorage');
    $this->register('drupal.config', '%drupal.config.factory%')
      ->addArgument('%drupal.config.class%')
      ->addArgument('%drupal.config.storage%');

Given this, modules could, in theory, use the following to swap out classes for the config system:

  drupal_container()->setParameter('drupal.config.class', 'Drupal\\mymodule\\MyConfigSystem');

I believe it's still giving some exceptions when trying to save private wrappers, but the initial idea is there. The key is making sure each of the classes is decoupled and given what information it is needed. In this case, the majority of it was removing $name from the Storage system so that we could share a single Storage system across multiple Config objects.

Status: Needs review » Needs work

The last submitted patch, 1605324.patch, failed testing.

sun’s picture

Status: Needs work » Active

Hrm. @Rob Loach: That's something completely different compared to the scope of this issue. :-/ Can you create a separate issue for implanting config into DI?

RobLoach’s picture

This only instantiates the Config classes once per name, and once for the DatabaseStorage. So it does accomplish what the goal of this issue is. The alternate solution is using statics, which is what we want to move away from.

sun’s picture

Assigned: Unassigned » sun
Status: Active » Needs review
FileSize
28.78 KB

Here we go.

Fully working implementation of https://gist.github.com/2822857

Alas, this resolves a shitload of issues in one fell swoop.

This code lives in the config-object-1605324-sun branch.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
34.26 KB

Sorry, forgot to update tests accordingly.

cosmicdreams’s picture

Just a few questions for my education:

+++ b/core/includes/config.incundefined
@@ -80,9 +78,30 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ * @todo Replace with DI container.

Can you be more specific / explicit with this comment? What will be replaced by the DI container? the $class parameter?

+++ b/core/includes/config.incundefined
@@ -80,9 +78,30 @@ function config_get_storage_names_with_prefix($prefix = '') {
+  global $config_info;
+  static $instances = array();

Hopefully when this code is changed to use the DI container we can also reduce or eliminate the use go global / static variables

+++ b/core/lib/Drupal/Core/Config/FileStorage.phpundefined
@@ -91,10 +64,10 @@ class FileStorage {
+      throw new FileStorageException('Failed to write configuration file: ' . $this->getFilePath($name));

Does revealing the file path (with name?) here cause an security concerns?

+++ b/core/includes/config.incundefined
@@ -80,9 +78,30 @@ function config_get_storage_names_with_prefix($prefix = '') {
+ * @todo Replace with DI container.

Could you be more explicit with this comment? Replace what with the DI container? The $class argument?

Status: Needs review » Needs work

The last submitted patch, 1605324.patch, failed testing.

Issue tags: -Configuration system

The last submitted patch, 1605324.patch, failed testing.

sun’s picture

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

This should pass now.

re: #11/@cosmicdreams:

  1. Moving from a singleton to the global dependency injection container is basically what @Rob Loach attempted to do, and what I'd also like to investigate, but in a separate follow-up issue.

    The reason for that is relatively obvious — namely, the question of what comes first? Will the DI container itself require configuration, and can it even be instantiated before the config system exists? IMO, that's a non-trivial design question that needs to be properly discussed and figured out first. However, I'd like to detach that discussion from this issue, since this issue effectively blocks #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation and #1552396: Convert vocabularies into configuration currently.

  2. Basically same as 1. The static would go with a DI container, but the global variable essentially is "configuration for the configuration system"; i.e., the very lowest config that can exist.
  3. That's an interesting question, but that code is only moved and not introduced here. Can you copy your concern over into #1444620: Remove file signing from configuration system, please? :) I think it's worth to discuss.

Status: Needs review » Needs work

The last submitted patch, config.object.18.patch, failed testing.

cosmicdreams’s picture

@sun, I've forwarded the security question to #1444620: Remove file signing from configuration system as requested.

sun’s picture

Status: Needs work » Needs review
FileSize
35.39 KB

Now for real.

sun’s picture

The summary is sparse, but actually states what we've discussed through various issues. And the patch in #21 implements exactly that. Nevertheless, I'll try to update it, yes.

I won't repeat myself, so with regard to a potential usage of DI and the fundamental architectural chicken-and-egg situation, see #18. This is totally worth to discuss, but a completely separate issue than what we're aiming for here.

The goal here is to untangle DrupalConfig and storage controllers from the actual/individual config objects, and only instantiate them once. This requires architectural changes, since the entire code hard-codes the 1:2:1 binding. The new architecture is 1 DrupalConfig, 1-M storage controllers, and N detached config objects.

sun’s picture

FileSize
38.32 KB

Resolved the added @todo and added high-level docs for DrupalConfig.

Status: Needs review » Needs work

The last submitted patch, config.object.25.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
38.32 KB

whoopsie, variable name mismatch.

chx’s picture

When trying to understand what's happening here , I found it very hard to understand what happens with classesnames like "DrupalConfig" and "ConfigObject". I suggested renaming "DrupalConfig" to "StorageManager". I am told it's a "followup". I no longer grok even the core process , I can't understand how the very source of the confusion with a patch is brushed aside as a followup but so many things are new in Drupal 8 so it is possible that this is another new thing I am unaware of so I won't even CNW the issue on this. I would adjust the one line summary as well because "Defines the default configuration manager." is almost meaningless. How does it "manage" configuration? Then it continues "A high-level storage manager that determines which storage out of multiple is configured and allowed to handle a particular configuration object, depending on the read/write operation being performed." obviously missing something like "This class is a high-level storage manager..." but then the issue summary should be "The default configuration storage manager" -- that makes more sense especially with the now fluently continuing issue summary.

"The information about available storage controllers and their configuration options is passed once into the constructor and normally should not change within a single request or context." and how do tests handle changed config

"The configuration manager instantiates a new configuration object for each configuration object that is accessed, and which is returned to callers of the load/save/delete methods instead of the configuration manager, since most consuming code wants to act on the configuration object directly." -- this is totally non sequitur. So far we talked about storage. Now we have a mile long sentence which I only understand because I read the code. It also makes me wonder -- is this the right way to do this? Shouldnt be the architecture like this: storage classes , storage arbitrator (that's what you call a manager), storage arbitrator injected into the configuration factory? Why does the storage arbitrator also act as a config factory?

chx’s picture

FileSize
39.96 KB

This is my version with separated ConfigFactory and a simplified StorageArbiter and also simplified ConfigObject. Edit: note that most of the work is still sun's! It's a lot and awesome work. I just moved a few pieces around.

RobLoach’s picture

I was a bit too late to get this in before chx uploaded his patch. But, here's sun's patch using the Container instead of statics. The instantiation logic of config() is still there, and all the classes are still swappable via the Container parameters.

Status: Needs review » Needs work

The last submitted patch, config.object.30-innerdiff.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

Please review #29 in the light that #30 makes it hopeful that it will be easy to port to DIC and get rid of statics.

alexpott’s picture

FileSize
1.42 KB

Ran some tests against current 8.x and the patch in #29 (see php attached).

Test code just loads every config object we have:

$config = config('system.performance');
$config1 = config('system.cron');
$config2 = config('image.style.large');
$config3 = config('image.style.thumbnail');
$config4 = config('image.style.medium');
$config5 = config('system.rss-publishingi');

With the patch:
25015.35KB peak usage before test
45.25KB additional memory used after loading config
25047.04KB peak usage after test

Without the patch:
25016.4KB peak usage before test
23.71KB additional memory used after loading config
25030.63KB peak usage after test

Interpretation:
So it seems that with this patch ConfigObject is larger than DrupalConfig. There are some slight memory savings with the patch in just bootstrapping drupal to DRUPAL_BOOTSTRAP_CODE. The patch is using more memory to load all of the currently available config objects.

alexpott’s picture

Patch in #29 removed DrupalConfig class but left references to it in a couple of places. Patch attached cleans this up.

sun’s picture

First of all, attached is an interdiff between #27 and #34.

To make that interdiff more meaningful, I have reverted the changes that rename of DrupalConfig.

In general, I still think that rename could and actually should happen in a separate issue, because it would make sense to discuss the expected behavior and functionality of that manager/arbitrator class a lot more — which potentially also includes to create a second implementation that introduces comparison/synchronization methods for actually managing the stores, which sorta was the original purpose of all the writeToActive(), isOutOfSync(), etc methods on (some of) the storage controllers (but which were not actually used anywhere, and also, the built-in multi-storage writing behavior of the DatabaseStorage was/is a huge conflict in terms of the actual envisioned design and desire of being able to diff configuration changes between active and file storage, so any automated write to the file storage inherently means to break that ability and vision).

For the scope of this patch, the rename is not necessary and the very basic storage selection algorithm is sufficient to make the system work with multiple available stores.

Thus, created the follow-up issue: #1624580: Research and implement a proper design for StorageDispatcher

The purpose of the interdiff is to clarify the architectural differences between the two patches, which wasn't really easy to distill before.

I will now go ahead and review these changes in some more depth and likely also perform some clean-ups.

alexpott’s picture

An issue with ConfigFactory class that meant it never used the static array of arbiters. With the attached patch the results are:

25358.89KB peak usage before test
20.63KB additional memory used after loading config
25371.32KB peak usage after test

sun’s picture

FileSize
6.59 KB
38.97 KB
  1. Removing the load/save/delete methods from the storage manager and only keeping them on ConfigObject definitely seems to be cleaner.
  2. The original underlying idea for making the storage manager instantiate ConfigObjects was that we most likely ultimately have to implement a caching layer and mechanism comparable to the variables bootstrap cache in D7. So there would have to be one place/controller/manager, which tracks what config objects are being used within a request. The ConfigFactory does not really seem optimal for that. Perhaps thinking about this is premature though, and we might want to discuss it in either DI follow-up or the StorageManager follow-up issue instead.
  3. Embedding the storage controller default configuration into the storage manager constructor seems wrong to me. A better place would probably be the ConfigFactory, but the amount of arguments to that are getting a bit hairy already.
  4. Now. What bugs me most:

Attached is a slightly cleaned up patch; most notably including more docs.

I will now try to make the above mentioned last point possible again.

sun’s picture

FileSize
1.15 KB
39.15 KB

Incorporated #36, but correctly.

sun’s picture

FileSize
40.09 KB
3.69 KB

So, basically, what I want is this.

You don't need to tell me that this looks suboptimal. The purpose is to clarify the design goal I outlined in #37.4.

The question is, can we make this possible?

Status: Needs review » Needs work

The last submitted patch, config.object.39.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
42.6 KB
7.3 KB

I'm running against walls with pretty much everything I'm saying. The debates we're having are clearly showing huge gaps between theoretical naysaying and practical problems in actually attempting to make this shit work.

Attached patch reverts #39, and adds a dedicated test to verify that CRUD operations on a config object are working as expected.

Also changed config() to construct the ConfigFactory, to prevent it leaking into tests, since it seems we're going to do the DI change in a separate issue.

RobLoach’s picture

Issue summary: View changes

more better summary

RobLoach’s picture

sun’s picture

FileSize
2.21 KB

Important additional note:

This patch (unintentionally) makes config() and the config system (including active store) work at the DRUPAL_BOOTSTRAP_CONFIGURATION bootstrap level already. This is because of the replacements of procedural db_*() functions with Database component methods in DatabaseStorage.

Also, to redo the benchmarks that @alexpott did before:

HEAD:

$ php bench_functional.php
ref: refs/heads/config-object-base
Peak memory before test: 1,183.09 KB

nothing: 0.0001 seconds
function no_op(): 0.0002 seconds
config(): 0.7282 seconds

Peak memory after test: 3,999.20 KB
Memory difference: +2,816.10 KB

HEAD with patch:

$ php bench_functional.php
ref: refs/heads/config-object-1605324-sun
Peak memory before test: 1,184.52 KB

nothing: 0.0001 seconds
function no_op(): 0.0002 seconds
config(): 0.7209 seconds

Peak memory after test: 3,986.45 KB
Memory difference: +2,801.93 KB

Bench script attached.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Updated the summary.

chx’s picture

FileSize
45.41 KB

Renamed DrupalConfig to StorageManager as it was already called so in the patch several places just not the actual class. I think I stated it crystal clear that while I will not contend anything else but this is a non-starter. Merlin on IRC: " To be fair, a class named DrupalConfig might as well be named Leprechaun for all the name conveys about what it does."

sun’s picture

Thanks :)

chx’s picture

FileSize
1.85 KB
45.41 KB

StorageManager is storageManager when property. Sigh.

chx’s picture

It must be noted that I find the process this patch went through extremely disturbing. sun declared that "we can rename once it's RTBC" and completely ignored my insisting to rename beforehands. RobLoach RTBC'd this despite me and merlinofchaos both saying that this class name is a non-starter. This is not how our process works or should work.

gdd’s picture

Status: Reviewed & tested by the community » Needs review

I still haven't had a chance to review this in detail and I would like to before it goes in.

gdd’s picture

Assigned: sun » gdd
gdd’s picture

Title: Instantiate DrupalConfig class and storage controllers only once » Configuration system cleanup and rearchitecture

Retitling to be more appropriate, as discussed in Barcelona

gdd’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

Having taken some time to review this patch and after a long discussion at Drupal Dev Days in Barcelona, I'm pretty well on board with the changes in #47 I'm waiting to RTBC to see if sun wants to update this to his latest work or not.

Jose Reyero’s picture

The patch looks good to me and quite an improvement . I'd just suggest some simplifications:

- Why don't we pass the StorageManager object as a parameter to ConfigFactory? This would allow using different ConfigFactories for other purposes.
- Same for StorageManager, could'nt it be better a built StorageInstance object which would work for our 90% of the cases which is just reading/storing configuration from/to database? Some example of this (and really simplified storage classes) is here, https://drupal.org/node/1624580#comment-6153342 (For this example it is called ImportStorage instead of StorageManager and not exactly the same but way more simple).

Jose Reyero’s picture

Also, as a side note I think I won't understand the purpose of passing around the ConfigObject class till I can see some example. What's the difference between these two options besides making the whole config system more complex?:

return new WhateverModuleOjbect(config('my.config.name'));

and

return config('my.config.name', 'WhateverModuleObject');

Btw on the patch if we use config() two times with different class names, the static caching in the config factory won't work.

Jose Reyero’s picture

Updated the patch with (own) suggestions in #53. It is at the very least much simpler.

Status: Needs review » Needs work

The last submitted patch, 1605324-config_reachitecting_simplified.patch, failed testing.

aspilicious’s picture

Patch is incomplete and when you reupload can you provide an interdif? Thnx!

Jose Reyero’s picture

FileSize
17.4 KB
47.94 KB

Patch #55 with missing files, fixed tests, and interdiff.

Jose Reyero’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1605324-58.patch, failed testing.

aspilicious’s picture

Looking at the interdif, why are you removing the static functions?

sun’s picture

Assigned: gdd » sun

@Jose Reyero: Let's keep the discussion on StorageManager/StorageDispatcher in #1624580: Research and implement a proper design for StorageDispatcher. For this issue/patch, we should perform the basic refactoring only, and retain the current, implicit multi-storage handling. I'll comment on your manager/dispatcher changes over there.

@all: My plan of attack is to get the remaining @todos and things we've discussed in Barcelona into the code over in #1626584: Combine configuration system changes to verify they are compatible and will extract the minimum required changes from that into this issue immediately after.

Jose Reyero’s picture

@aspilicious,
Removing the static functions because as they depend on a class property ('directory' for files to get paths) they cannot really be static. There are other weird usages of static functions there, like for getting the file extension we invoke a static method on the class itself, talking about FileStorage::getAllNamesWithPrefix(), where we invoke FileStorage::getFileExtension(). This doesn't look right to me because if we ever extend that class and change getFileExtension() then we'll need to override getAllNamesWithPrefix() too.

Other usages are similar so since I had to fix one of them for the patch, fixed them all.

@sun,
Well, since this patch introduces StorageManager it looks like the right place to improve it before we make it too complex. My point is we don't need all the complexity that was being introduced here and we can have more possibilities with a simpler patch.

Also, this patch adds some subtle but 'big' feature which is the possibility of building multiple ConfigFactories because if you look at it it doesn't rely on class names anymore passed around but on actual objects which gives us better encapsulation, more flexibility, and the possibility of building some kind of 'context' into them later.

sun’s picture

1) ::getNamesWithPrefix() has already been adjusted in #1626584: Combine configuration system changes to verify they are compatible. It cannot be a static method, because the target/directory to operate on depends on the storage controller options/configuration.

In general, please note that the most recent patch here is very outdated and does not account for a couple of required changes that were discovered in #1626584: Combine configuration system changes to verify they are compatible.

2) It still doesn't make sense for me to just simply hack on StorageDispatcher changes without having concrete requirements and expectations first. In case you didn't see it, I followed up on #1624580-6: Research and implement a proper design for StorageDispatcher with some possible and very reasonable expectations on the storage dispatcher, which would fundamentally change its design. It is only part of and introduced this issue, because we need to untangle it. I personally do not really care about how exactly it is introduced here, as long as the functionality is untangled from the Config object and we basically retain the original architectural intention. Since it is very likely that we're going to design and revamp it entirely in aforementioned issue, I'd really like to defer any further changes to that issue.

Jose Reyero’s picture

Ok, I really can't follow up on so many patches spread all around.

> ) It still doesn't make sense for me to just simply hack on StorageDispatcher

Agree. I just posted a simplified version because I was seeing this getting way too complex without reason (that I can see)

About StorageManager, StorageDispatcher or whatever I'll follow up on the other issue.

But please let's keep this simple and up to the point. (Was it ConfigFactory + names into config objects instead of storage?. Then I guess there's no reason for StorageManager being here).

Short: please commit anything so we can move forward with other issues.

sun’s picture

please commit anything so we can move forward with other issues.

Yep ;) I'm currently writing unit tests for the actual storage controller operations (i.e., without Config or StorageDispatcher being involved at all), which were totally missing in the entire refactoring thus far, and writing them massively helped to sort out expectations on their behavior.

One primary and essential expectation on their behavior is the handling of soft errors, hard errors, and edge-cases. The combined patch revealed a couple of problems on that already. Writing actual unit tests for storage controller expectations revealed some more.

sun’s picture

Status: Needs work » Needs review
FileSize
68.88 KB

Here we go. Clean extract from #1626584: Combine configuration system changes to verify they are compatible

As the file size suggests, the main refactoring (including proper tests) is ~70% of the combined patch. I still don't really understand why we want to split things out, but that has been the decision in Barcelona, so I won't argue against that. As long as this big chunk lands ASAP, I don't really have an issue with that approach.

Note: If anyone sees any need for changing anything in this patch, for the sake of love and world peace, please branch off from config-object-1605324-sun into your own branch and push your suggested changes into the CMI sandbox. The purpose of doing so is to properly retain and maintain a "mainline" for the combined issue/patch in parallel, in order to validate any architectural changes against the bigger picture — which has proven itself to be a critical verification of pretty much every single suggested change in detail thus far. If you don't want to do that, then just review and tell us/me what needs to be changed, please.

Jose Reyero’s picture

Well, sorry about my last comment what I think was not helping too much in any direction.

What I think we should do is keep the patch smaller and see the few things everybody seems to agree on about this, which are:

- We need some kind of ConfigFactory which produces and caches config objects and can be later used with dependency injection.
- We need to keep the name into the Config object so it can read and write its data to storage when needed.
- Simpler Storage classes which just read and write from / to somewhere when given a config name / data

Since this seems to be in everybody's patches. Why don't we stick to that few points, forget about StorageManager or StorageDispatcher or whatever you want to call it until we see first whether we actually need it and what we need it for, and try to get that few things in? That should make every other patch I've seen so far way smaller.

Jose Reyero’s picture

FileSize
63.62 KB

@sun,
And this is your patch (#67) without StorageDispatcher which as you said before and I agree should be kept aside for further discussion (whether we need it or not in the first place)
And without renaming the config object yet just to keep the patch smaller and keep focused on what matters.

Status: Needs review » Needs work

The last submitted patch, 1605324-70.patch, failed testing.

Jose Reyero’s picture

FileSize
63.63 KB

Oops, fixed patch.

@sun,
About branching from config-object-1605324-sun, I am sorry but I've already did before and spent a lot of hours on it just to find out later it contanis a lot of changes that as we can see are still under discussion.

aspilicious’s picture

Status: Needs work » Needs review
aspilicious’s picture

Yeah can we please just commit the stuff *everyone* agrees on.

Sun you're probably 100% convinced we need the StorageDispatcher and chances are we probably need it but could you keep that for a different issue. (so we can speed things up) Else this ping pong game will continue for a while and in the end everyone is so upset that it's even harder to reach consensus.

Please?

alexpott’s picture

After reading #67 and #71 I think we might have an issue with module uninstalls. Which (unless I'm mistaken) will now leave behind config files in the /sites/default/files/confg_somerandomstring directory. This is going to lead to some interesting situations when a module is re-installed. Config provided by the module will be overwritten eg. image.style.large but if you've created some config eg. image.style.mycustomstyle this will be available for import.

Is this the expected behaviour? I think that when I module is uninstalled all config should be removed from the database and the public files directory.

aspilicious’s picture

If that is true and we want it to be removed we should fix that and write a test for it. (if that is possible)

gdd’s picture

This functionality was committed in #1444766: Determine method for automatically cleaning up configuration after a module is uninstalled, if it is broken now that is a regression.

sun’s picture

FileSize
60.94 KB
  1. #67 contains exactly those changes that we collectively agreed on.
  2. It also contains a StorageDispatcher. Everyone agreed that there is another business logic mixed into the current DrupalConfig, and that it is a good idea to untangle and separate that code into a StorageDispatcher (formerly named StorageManager), so we can properly discuss what that logic is and needs to be, in a separate issue.
  3. #67 obviously grew in size compared to the last RTBC patch in #47, since the combination of issues/patches revealed many bugs and conflicting changes, which in turn led to the addition of code for unit/functional tests.

I did not attach an interdiff for #47 and #67 previously, because a quick interdiff appeared to be bogus. I created a branch-based interdiff now.

Summary of changes to #47:

  • config.inc must not be unconditionally loaded in the global scope of bootstrap.inc. It is only able to work after the DRUPAL_BOOTSTRAP_CONFIGURATION bootstrap level has been reached and the class autoloader is functional.
  • config_get_config_directory() has to be in bootstrap.inc, since Drush calls into t() very [too] early, which in turn calls into drupal_container().
  • config_install_default_config() and other module system and install/update system functions inappropriately wrote to the FileStorage previously. By default, the config system must only write to the active store.

    Enabling/allowing dual-write operations to both the active store and the file storage is the topic of #1624580: Research and implement a proper design for StorageDispatcher

  • StorageInterface::getNamesWithPrefix() was renamed to StorageInterface::listAll(). In turn, each storage controller implements Create, Read, Update, Delete, and List.
  • StorageInterface::listAll() is no longer a static method, because it needs to take the storage controller's configuration options into account; e.g., when FileStorage is instantiated with a custom directory, then ::listAll() has to list files from that directory.
  • ConfigFactory is registered as service on the DI container. Essentially the change that @Rob Loach asked for earlier in this issue. For simplicity, however, the service registration does not use separate parameters for the individual class names to instantiate. Replacing the config system services (not the storage controllers) is a 0.01% use-case, so whoever wants to do that can, but needs to replace the service definition entirely.
  • drupal_uninstall_modules() and other functions and tests now also treat the first part of a configuration object name up until to the first dot/period as the namespace/owner of the config object. This concept was introduced with #1589174: Configuration upgrade path is broken already, but not fully implemented in module system and installation functions yet. This namespace/owner concept is also what enables us to do #1447686: Allow importing and synchronizing configuration when files are updated in a clean fashion. This was part of the discussions in Barcelona and we agreed on that.
  • StorageManager was renamed to StorageDispatcher, because "manager" did not really describe what it does. As mentioned before, untangling this code from the former DrupalConfig is a temporary measure only, and only happens for technical clarity.

    This code and logic was never really discussed in the past, because it always was mixed and baked into the DrupalConfig class. There are no technical requirements defined for it, nor do we have a defined set of expectations. It is not clear whether it will stay or not, and figuring that out is the purpose of #1624580: Research and implement a proper design for StorageDispatcher — what we know is that the current code contains logic for that functionality, which in turn means that there have been architectural intentions to support such logic, even though it wasn't cleanly specified. This is a too large topic on its own, for which not even the combined issue/patch was able find any clues.

    Consequently, this refactoring here only separates the existing code into an own class, so that it is clearly visible and can be discussed properly.

  • The static/property caching of config objects in ConfigFactory has been removed and replaced with @todo comments, which explain the situation. Further work on this needs to happen, but not as part of this issue/patch. We need to convert some more variables into config and do proper performance benchmarks and profiling in #1605124: Configuration system performance afterwards.
  • Storage controllers were fixed in terms of what exceptions and errors they throw, and under which circumstances. To actually prove this and put expectations into code, dedicated / isolated tests for storage controller CRUD operations were added. The storage controller tests are written in a way that allows us to verify that each storage controller behaves exactly in the same way. As usual, these tests revealed many bugs on their own. However, note that the tests are not fully complete; a few @todos remain, but those can be handled in a separate follow-up issue.
  • Admittedly, this patch also contains the fix for #1608912: (Re-)adding keys to configuration breaks the intended goal of being able to diff files in the Config::sortByKey() function, because I was too lazy to remove it. However, if that is a problem, then please state so, and I will remove it + the test for it.
  • A large amount of documentation fixes and clean-ups.
sun’s picture

re: #74 - #76: Nope, there's nothing broken. Other Drupal code and subsystems have no business at all with the FileStorage. No code is supposed to perform explicit configuration exports on its own. All code is interacting with the configuration system, and by default, the configuration system works with the active store only (whatever it is).

The configuration system reads from a single storage only, the active store. Thus, it doesn't matter whether there are any exported configuration objects in the file system or not. The import/export functionality is and has to be separated from the config system's runtime operation.

The separate discussion we need to have on the StorageDispatcher might (re-)introduce a way to perform dual-writes to multiple storages (i.e., also writing to FileStorage on every write) - but perhaps not. We need to understand that (always) writing to the FileStorage breaks the entire idea, concept, and original vision of being able to record and revision an exact state of configuration through the file system. As soon as files are overwritten, there's nothing to import. Consequently, there's also nothing to export. The dual-writes performed by default in the DrupalConfig class in HEAD are only valid and suitable for the use-case of a local developer site, on which you are sure that all configuration changes you make will be committed into your site's VCS repository, and pushed to a staging/production environment afterwards, on which an explicit configuration import is performed.

But again, that discussion belongs into #1624580: Research and implement a proper design for StorageDispatcher

gdd’s picture

@sun Given the fact that even you admit in various places (#77 above, #1624580-8: Research and implement a proper design for StorageDispatcher) that we may not end up with a dispatcher at all, and since there seems to be some conflict over its design, it does seem to make sense to remove that entirely from this patch and move that discussion entirely over to #1624580: Research and implement a proper design for StorageDispatcher.

@everyone This issue is becoming completely impossible to track because it covers so much ground, so naturally there is lots of stuff for people to argue over. I can barely even keep up with whats going on here since Barcelona and that was only a week ago. I really appreciate everyone's passion and attention and time, but lets do what we can to get this in and move on, even if it means pushing some fundamental disagreements to followups. Everyone seems basically on board with this and it is blocking a ton of work on other issues. Thanks all.

Jose Reyero’s picture

Basically, my only objection is to the StorageDispatcher/Manager/Whatever being here and as demonstrated in some of the patches there's no actual reason why we need it here.

For the rest, any of the patches here is a huge improvement over what we've got, fixes a ton of other side issues, and we could move on from that point way more easily.

(Which means I'll be happy with whatever version of the patch is committed, even if we've got a StorageManager we need to fix later...)

So, in order to move on, I'd suggest someone, (@sun?) does a final version of the patch and and if we've got no major objections we push together for it to be committed.

sun’s picture

The problem I have with #71 is that it does not return a Config object from config() (and ConfigFactory).

Instead, it returns a DrupalConfig object, which duplicates the Config object's properties and methods, which is exactly what this refactoring tries to get rid of - it lacks a proper separation of concerns and retains a DrupalConfig class that does not know what it is and bundles way too much functionality and business logic into a single object.

Thus, returning something else than a Config object from config() does not fly at all for me.

I could live with an alternative architectural implementation that would introduce a DatabaseFileStorage or similar (reading from Database, writing to Database and File storage) that resembles the behavior that is currently in HEAD, and which would replace the StorageDispatcher that is passed into the Config class constructor. However, that would also be a larger architectural change, of the same size and effect of the StorageDispatcher.

My point still is that StorageDispatcher is only introduced here for the sole purpose of clarifying that the current code has a storage dispatching logic built-in. For the refactoring to make sense, that current code needs to go somewhere else. I'm uncomfortable with just removing or replacing it with something completely different that removes the dispatching behavior entirely, because that means that we're removing functionality that was originally foreseen to happen within the config system. Even though the functionality was never cleanly specified, I'd like to retain something that "at least" clarifies that "something like this" was originally foreseen and that we have to discuss whether it is actually required and needed, and if so, how its architecture and implementation has to look like.

As also mentioned before, not even the combined issue/patch led to any better clues on that functionality. The only hard requirement that got revealed was that no code (except of tests) should contain any hard-coded references to any particular storage controller class, since that breaks the design of pluggable storage controllers in the first place. However, implementing something that would remove these hard-wired class names very quickly conflicts with a StorageDispatcher, if there is any.

In the end, I spent a lot of time on that question already and attempted different approaches in code, but none of them looked clean or solid enough. From my perspective, the root cause for that is that we don't have any technical requirements and expectations for that functionality written up anywhere, which means that a dozen of different implementations are possible, but all of them are too easily missing crucial functionality that needs to be supported.

Hence, I'd feel much more comfortable with introducing the StorageDispatcher as-is here. Even though the implementation does not actually support what it is supposed to do, it at least gives us some concrete code to look at and argue for or against in #1624580: Research and implement a proper design for StorageDispatcher. I.e., we're changing the rest of architecture to be what it should be, except for the previously baked-in dispatching logic, for which we're retaining at least the conceptual idea that there is something that is capable of writing to multiple storages.

I'm aware that this has a "lame touch" to it. But at this point, I think the critical priority is to get the actual refactoring into core, and I'd really like to see us stop hacking on StorageDispatcher vs. alternative approaches without a clear and precise plan for what functionality is actually required or isn't required. Until then, the (basic) concept exists in HEAD and should remain to exist, so it can be filled out with a proper implementation, replaced with a better, or perhaps even removed entirely — which is all possible and absolutely depends on the actual plan we come up with.

Jose Reyero’s picture

@sun,
> The problem I have with #71 is that it does not return a Config object from config() (and ConfigFactory).

I don't really understand this since it is the same as your patch in #67 just without renaming DrupalConfig to Config. I mean the only thing that changes about that is the class name.

Patch #71 is #67 without StorageDispatcher, and if I've changed anything else it's been by mistake. (About class names I couldn't care less, the only purpose of keeping the old name was making the patch smaller, since the new one didn't make more sense to me than the old one and renaming added a good number of lines to the patch...)

About the rest (StorageDispatcher) if I get it right you mean you are possitive about needing a StorageDispatcher, still not knowhing "what functionality is actually required or isn't required" for it...?

Whatever, I'm fine with that. Since I agreee with he other 90% of the patch, which is *critical* to move on with config system, and D8MI follow up patches, I'll just +1 your patch. Really I don't want to discuss more about it.

sun’s picture

FileSize
9.89 KB
64.57 KB

Hrm. I see. The rename of DrupalConfig into Config is an essential part of the re-architecture/clean-up though. Reverting that rename was totally unexpected and caused quite some confusion on my side.

So what you actually did is:

1de5f2a Removed StorageDispatcher concept entirely.

Attached patch includes just that suggested change; the interdiff is against #67.

Providing this patch does not mean that I really support this change. As I tried to explain earlier, this removes a conceptual feature that is currently in HEAD, without replacement; i.e., an intentional feature regression. With this change, the configuration system only knows about exactly one storage, the active store; i.e., DatabaseStorage (by default). Any notion and original intention/idea of having a mechanism for handling multiple storages (and thus also dual-writes to multiple storages) built-in is lost. While this definitely simplifies things, the consequence of doing so is that an attempt to re-introduce that capability means to add a new feature to Drupal core. It is that consequence which made me introduce the StorageDispatcher. Even though the implementation in #67 does not actually do what it promises, retaining the code means to retain the conceptual and architectural feature.

That said, I'd also be fine with committing this patch instead of #67 in order to get this issue finally off the table ASAP.

Therefore, I'm deferring to @heyrocker and/or @catch/@webchick/@Dries to make a decision between #67 and this patch, as I believe that sufficient explanation and details have been provided on the potential StorageDispatcher feature regression.


That said, checking the interdiff once more after attaching it, this change lacks the $options parameter definition for the storage controller, so DatabaseStorage always defaults to the default database connection and its default target. Which also implies that there's no way to specify storage controller options when using a different implementation/active store. However, I'd be fine with fixing that in a follow-up issue, and alas, this (again) actually touches the StorageDispatcher and pluggable storage discussion... so actually belongs into that issue.

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

The last submitted patch, config.object.83.patch, failed testing.

sun’s picture

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

#83: config.object.83.patch queued for re-testing.

Test client #654:

Fatal error: FieldInfoTest.php	138	Drupal\field\Tests\FieldInfoTest->testFieldPrepare()
Fatal error: FormTest.php	48	Drupal\field\Tests\FormTest->testFieldFormSingle()

Created issue for that:
#1660888: Fatal errors in D8 Drupal\field\Tests\FieldInfoTest and Drupal\field\Tests\FormTest

cosmicdreams’s picture

Just one question:

+++ b/core/includes/bootstrap.inc
@@ -480,6 +478,25 @@ function find_conf_path($http_host, $script_name, $require_settings = TRUE) {
+ * Returns the path of the configuration directory.
+ *
+ * @return string
+ *   The configuration directory path.
+ */
+function config_get_config_directory() {
+  global $config_directory_name;
+
+  if ($test_prefix = drupal_valid_test_ua()) {
+    // @see Drupal\simpletest\WebTestBase::setUp()
+    $path = conf_path() . '/files/simpletest/' . substr($test_prefix, 10) . '/config';
+  }
+  else {
+    $path = conf_path() . '/files/' . $config_directory_name;
+  }
+  return $path;
+}
+
+/**

Should we inject the path to the config here?

Otherwise it looks good.

keeping this as needs review as this question doesn't need work, just feedback.

sun’s picture

Status: Needs review » Reviewed & tested by the community

You probably meant to turn conf_path() into an argument of the function, instead of calling it from within.

I'd rather not want to do that - at least not in this patch. The function is just moved from config.inc into bootstrap.inc here, to make it available earlier. But that said, it's an excellent suggestion, which basically boils down to "service-fying" conf_path() and config_get_config_directory(). I'm not sure whether that would ultimately make sense, but we can explore it in a separate issue.

Also. This patch (whichever of both) is RTBC. See #83 for final call.

gdd’s picture

After discussions in Barcelona I do believe we will need the StorageDispatcher in some form, and my desire to commit without it above was more an attempt to move forward than anything. But if everyone just wants to see something happen then I'd like to see #67 go in.

sun’s picture

FileSize
2.43 KB
69.16 KB

Thanks for making the call, @heyrocker.

Re-attaching #67, but with additional simplification and added docs from #83:

1d45c92 Revert "Removed StorageDispatcher concept entirely."
7fcb44a Simplified ConfigFactory service definition; added docs.

EDIT: interdiff is against #67, of course.

webchick’s picture

Assigned: sun » Dries

Dries would like to take a look at this before commit. This should be able to happen on Friday.

webchick’s picture

Issue summary: View changes

Updated issue summary.

gdd’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Followup: #1665022: Rename StorageDispatcher to StorageArbiter I do not want to hold up this issue with another debate, so definitely followup.

Dries’s picture

I'll be reviewing this patch today.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I looked at this patch and it looks good. I personally don't think we need the StorageDispatcher. It would be straight-forward, and maybe even necessary, to make a custom Storage class; say a hybrid Database+Filestorage class. Let's figure that out in a follow-up issue; maybe we can actually come up with a good use case for the StorageDispatcher.

In the interest of making progress, I've committed this patch to 8.x.

sun’s picture

Priority: Major » Critical
Status: Fixed » Reviewed & tested by the community

And again, all new files are missing from the commit.

Easiest course of action would be @Dries to fix the mess, but he doesn't seem to be online anymore.

Thus, second best thing to do is to revert the commit and re-commit the patch properly.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oopsie. :)

Ran:

git revert 8df23c8a8717b637
git commit
git apply --index config.object.89.patch
git commit
git push

We should be good now!

Jose Reyero’s picture

Wow! Finally! Opening a beer here :-)

(Or maybe two, because I couldn't agree more with @Dries #93 "I personally don't think we need the StorageDispatcher....")

See you on the follow-up issues ;-)

sun’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.