This patch (which includes most of #1671198: Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor) implements:

- Configuration Events and Listeners that are pluggable objects that can alter the runtime values for configuration or respond to read/write operations:
- Storage realms ('language=xx') without changing at all the Storage classes. Using now name prefixes like 'locale.config.es.system.site.yml' that will keep the locale's module overrides for a configuration object.
- A Locale Config Listener that can localize runtime values for configuration objects.

Examples of these Configuration Listeners provided in the patch (to demonstrate how they can be reused) are:
- LocaleConfigListener, which overrides configuration with localized values.
- ConfigGlobalOverrideListener, which overrides with global $conf values.

Updated write up according to #9

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, config-next-plugins-01.patch, failed testing.

alexpott’s picture

Issue tags: +Configuration system

Tagging issue to make it easy to find

gdd’s picture

There is some discussion in #1186034: Length of configuration object name can be too small that impacts this issue in terms of file naming.

Jose Reyero’s picture

Status: Needs work » Postponed

@heyrocker,
Thanks for the heads up.

Anyway this should be postponed till the rework gets in, #1605324: Configuration system cleanup and rearchitecture

Jose Reyero’s picture

Title: Implement plugins and storage realms for configuration » Implement ConfigHelpers and storage realms for localized configuration
Status: Postponed » Needs review
FileSize
15.7 KB

Updated the patch for latest config system changes and simplified it a lot, also removing example config files.
- Rebasing the patch to latest 8.x HEAD, includes also most of #1671198: Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor,
- Renamed ConfigPlugin to ConfigHelper since calling them plug-ins was just a name, nothing to do with the big Plugin Architecture that seems to be coming.
- Removed realm concept from db layer, now using prefixed config names, example locale.config.es.system.site.yml (Whether we need realm backs later we'll see, maybe for performance reasons but for now we can build the feature without them).
- Removed example localized configuration files since they don't belong to core, but you can get them (some localized spanish config like site name) installing this module http://drupal.org/sandbox/reyero/1635230
- To demonstrate how ConfigHelpers can simplify config system added a GlobalConfigHelper that will take care of global $conf overrides. Also provided in the patch a WatchdogHelper that will log all config operations to watchdog.

If you want to see this in action, just need to enable Spanish language and locale module, download that module linked above and see how switching language to Spanish the site name changes to 'Spanish Drupal'.

neclimdul’s picture

These feel a lot like Symfony Events/Subscribers. I'm going to have to dig in and get a feel for how they're being used and the interacting systems I think.

Jose Reyero’s picture

@neclimdul,
Yes, that makes sense, I'm still learning the basics about that Events/Suscribers.

Jose Reyero’s picture

Status: Needs review » Needs work

Changing to needs work to think about Susbscribers or consider a cleaner implementation.
Also the patch would get much smaller if we get this before #1671198: Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
15.41 KB

Yeah! Reimplemented the whole thing using Symfony Event Listeners and the container's event dispatcher.

I think the whole thing looks much cleaner now. As part of the patch, there are two Listeners working:
- LocaleConfigListener, which overrides configuration with localized values.
- ConfigGlobalOverrideListener, which overrides with global $conf values.

Events are triggered through the ConfigFactory with just a few wrapper functions. This is how we add a ConfigListener now:

function locale_language_init() {
  // Add locale helper to configuration listeners.
  drupal_container()->get('dispatcher')->addSubscriber(new LocaleConfigListener());
}

Quite straight forward, isn't it? I think this looks much better now, almost ready to go (still waiting for the other patch this one includes) but changing to needs review for the testing bot.

Jose Reyero’s picture

Issue summary: View changes

Updated issue summary.

Jose Reyero’s picture

Title: Implement ConfigHelpers and storage realms for localized configuration » Implement Config Events and Listeners, and storage realms for localized configuration
Issue tags: +D8MI

Updating title and tagging

neclimdul’s picture

I haven't really looked at what all is going on but the code does look a lot cleaner. Glad events lined up with what you where designing as well as they did!

Looking at the example, drupal_container()->get('dispatcher') has me wondering if we're introducing a core concept we should discuss though.

neclimdul’s picture

never mind, its already being discussed here #1599108: Allow modules to register services and subscriber services (events) we should just be aware :)

chx’s picture

So what is the point of having a StorageDispatcher if we add an external system to have overrides? I thought that we will add storages to the dispatcher and that will choose between the storages as appropriate.

chx’s picture

FileSize
9.87 KB

So here is another approach. This makes the storageDispatcher a real dispatcher, every operation is sent to every storage engine it is aware of. In order to allow the locale storage engine to be switched on or off, information about the storage engines are stored in the storage engines themselves: when we initalize a storage engine we read the "config" CMI object and the storages key in there can store information about other storage engines. So we start with the conf storage engine, find the active storage stored therein, the active storage might contain several others like locale (or domain or whatnot).

While the 'realms' concept is not implemented in #9 nor here, it's something to be addressed: what needs to be written where needs some sort of indication.

Edit: and yes, this does change how $conf behaves runtime.

Status: Needs review » Needs work

The last submitted patch, 1646580_14.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
9.81 KB

Rerolled against HEAD.

Status: Needs review » Needs work

The last submitted patch, 1646580_16.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review

Though it's good to see the storage dispatcher actually dispatching anything :-), please note that this patch was trying to address a more generic issue, which was allowing modules to react on config operations (events) and actually alter configuration objects. Also it included runtime localization *working*, which I don't see in the new patch (Though I guess it could be implemented somehow on top of it, but still it is missing the part about modules being able to do anything once the configuration system has been initialized.)

So I have to say I like this 'new storage dispatcher' patch more than what we have atm (that is a useless thing called StorageDispatcher) but also I think it belongs to a different issue, possibly this one: #1624580: Research and implement a proper design for StorageDispatcher

Also I agree it could open up more options for the localization issue but please, keep it as a different issue at least until it really addresses the 'problem' and it is a full replacement for the feature in the original patch of this issue.

Thanks

chx’s picture

Please note that http://drupal.org/node/1609760#comment-6099074 and on an agreement was reached after a long, long debate that we do not wish hooks to fire on $config->save(), I can only presume that this extends to other similar events (delete etc) too. So I am not sure why suddenly we want that to happen.

Now, to get to internalization I can add a LocaleStorage easily of course -- however what is not clear to me is just -- how and what do we want to write with that one? Seems to me that your patch stores every localization override in a single config object? Won't be that a little bit too big?

Jose Reyero’s picture

...we do not wish hooks to fire on $config->save(), I can only presume that this extends to other similar events (delete etc) too. So I am not sure why suddenly we want that to happen.

Well, that may be a bit outdated, I mean on later discussions it seemed clear to me that we are needing some kind of hooks/events/plugins https://drupal.org/node/1616594#comment-6141522

Anyway, whether we finally can manage to have everybody on board for such architectural changes, well, we'll see, this patch just tries to make the point of how it can be implemented with event listeners without 'inventing' anything else... still a hard sell I know. So I'd say thanks for the heads up, let me keep trying a bit more...

However if we end up with global $conf as the only way to override configuration in D8, I think that would be so sad...

Now, to get to internalization I can add a LocaleStorage easily of course -- however what is not clear to me is just -- how and what do we want to write with that one? Seems to me that your patch stores every localization override in a single config object? Won't be that a little bit too big?

Actually it's the opposite, it stores one override per config object and per language which I think is the only way to make the thing scale for many languages.
About LocaleStorage yes, I can see it and that's why I like your idea for putting StorageDispatcher to some use. What is not clear to me yet is how locale module would plug such storage in (and if modules cannot plug their storages, then I guess something is missing...) But really, I'd like to discuss that on a different issue.

chx’s picture

    $locale_name = $this->getLocaleConfigName($config->getName(), $language);
    $storage = $factory->getStorageDispatcher()->selectStorage('read', $config->getName());
    if ($override = $storage->read($locale_name)) {

To me this reads like you are doing one read per locale_name not per config name. Is this just a major typo or am I completely misunderstanding what $storage->read($locale_name) is?

Jose Reyero’s picture

We load the locale overrides for each config object and current language. So if that's what you mean, yes, we are loading one aditional 'config data' array, for each config object.

In the code above, if config name = 'mymodule.myname', and language code is 'es' locale name would be 'locale.config.es.mymodule.myname'.

?

chx’s picture

Oh then it's a little bit misleading variable name! sorry. I thought $locale_name was just 'es'. Makes a lot more sense now.

chx’s picture

We had a long phone call and we will go ahead with Jose's patch and eventually rip out the StorageDispatcher.

Jose Reyero’s picture

Assigned: Unassigned » Jose Reyero
Status: Needs review » Needs work

Ok, so this needs to wait for two other patches and then I will do a re-roll of #9 that hopefully will be simpler and cleaner. Waiting for:
#1671198: Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor
#1624580: Research and implement a proper design for StorageDispatcher

In the meanwhile we still want more feedback, not on the patch itself, that needs to be upgraded, but on the idea of using Events and Listeners for this.

chx’s picture

It absolutely does not need to be postponed on the storagedispatcher work. This is independent of it, once the config object simplification is in, this can get in too.

chx’s picture

Status: Needs work » Needs review
FileSize
13.07 KB

This is a reroll and a small refactor of #9. I know the Configuration tests pass so I hope everything else will.

Important changes include:

  1. In the minimal DIC we build in drupal_container() we create an event dispatcher too because the one in the core bundle is too late.
  2. The dispatch happens from within the config object, not the config factory. I doubt there was a need for that.

There's no interdiff as the reroll was more a reconstruction of the original intent with re-using the new files provided than a reroll, with the storage dispatcher gone and part of this already committed.

Status: Needs review » Needs work

The last submitted patch, 1646580_27.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
15.26 KB

It seems I forgot to re-add some new files (I had a catastrophe mid work where I did a git apply --index > 1646580_27.patch so that's how that happened). I checked git status this time.

Status: Needs review » Needs work

The last submitted patch, 1646580_29.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
547 bytes
15.25 KB

Oh, language(). Edit: wrong interdiff, this is the real one:

-    $language = drupal_container()->get(LANGUAGE_TYPE_INTERFACE);
+    $language = language(LANGUAGE_TYPE_INTERFACE);
chx’s picture

FileSize
2.17 KB
15.33 KB

I talked to Crell (because he is the OOP structure maven) and he said that creating an init method to fire the initial config event is "OK for now until we can figure out something better." This allowed me to make notify protected which makes me happy. There's no real functional change, though.

Gábor Hojtsy’s picture

Issue tags: +sprint

First and foremost should be on the D8MI sprint.

Gábor Hojtsy’s picture

On a quick review, I really like how this introduces a generic solution for overrides to config and moving $conf handling to this system. I'm definitely not satisfied with this level of control for language (hardwiring interface language), but this can be made more flexible in followups. We *don't* need to solve all problems at once or we risk it not getting done in any case. This is enough of a rework of the CMI stuff to support our overrides, that we should go with this level of implementation IMHO and then get more flexible language "contexts" in later.

Gábor Hojtsy’s picture

Priority: Normal » Major

Also giving some justice to the level of importance (if not critical :).

sun’s picture

Priority: Major » Normal
Status: Needs review » Needs work

Thanks for moving this forward.

It's hard to get some focused minutes for reviewing more complex patches here in Munich, but I already looked over this patch on my way to here. Overall, I think the proposed changes make sense. I didn't have sufficient time to look at the exact override logic being involved though.

What I've seen is that Locale module's override are using special config filenames. I think the last time we discussed this, we agreed on using subdirectories for context/override realm names, which contain the identical config file names as in the main/original bin.

+++ b/core/includes/bootstrap.inc
@@ -2458,9 +2458,13 @@ function drupal_container(Container $reset = NULL) {
+    $container->register('config.globaloverridesubscriber', 'Drupal\Core\EventSubscriber\ConfigGlobalOverrideSubscriber');
+    $container->register('dispatcher', 'Symfony\Component\EventDispatcher\EventDispatcher')
+      ->addMethodCall('addSubscriber', array(new Reference('config.globaloverridesubscriber')));

1) I'm still uncertain what the naming pattern/practice for registered services/subscribers is going to be (since that was left out of the modules/events/subscribers patch), but I think it would make sense to use a name of config.subscriber.[name].

2) It wasn't entirely clear to me at first sight what "globaloverridesubscriber" is for/doing. How about renaming it to config.subscriber.globalconf?

+++ b/core/includes/config.inc
@@ -65,7 +65,7 @@ function config_get_storage_names_with_prefix($prefix = '') {
-  return drupal_container()->get('config.factory')->get($name)->load();
+  return drupal_container()->get('config.factory')->get($name);

@@ -195,7 +195,6 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
         $old_config = new Config($name, $target_storage);
-        $old_config->load();

Unless I'm mistaken, the auto-load behavior was also originally part of #1671198: Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor, but got removed. I'm still against introducing that behavior, because it decreases/reduces encapsulation and testability of the Config object itself. I can imagine a couple of situations in which developers may want to instantiate a config object without loading data.

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -64,10 +72,21 @@ class Config {
+    $this->eventDispatcher = $event_dispatcher ? $event_dispatcher : drupal_container()->get('dispatcher');

The fallback on the global event dispatcher adds a unnecessary dependency on the procedural drupal_container() function. The fallback behavior actually doesn't seem to be used anywhere in this patch, so I think we should just remove it and change ::notify() to do nothing if no dispatcher has been provided.

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -89,6 +108,7 @@ class Config {
+    $this->build();

@@ -146,8 +164,7 @@ class Config {
+    return $this->rebuild();

These build() and rebuild() methods were originally part of #1671198: Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor, but have been replaced with the setOverriddenData() and resetOverridddenData() methods. So I think their re-addition is a unintended artifact of re-rolling this patch, which originally resulted in the separate spin-off issue.

sun’s picture

Priority: Normal » Major
chx’s picture

Assigned: Jose Reyero » chx

I will do a reroll soon. Thanks for providing context; I wasn't sure whether those were left out deliberately or simply left for this patch. The fallback is used from import so I can't remove that.

Edit: $old_config = new Config($name, $target_storage); this right here uses the fallback.

chx’s picture

Status: Needs work » Needs review
FileSize
5.66 KB
11.09 KB

That results in quite a smaller, cleaner patch. Thanks again.

Gábor Hojtsy’s picture

Issue tags: +Needs tests

Reviewed the update from @chx. It is indeed a leaner version of the patch, it does not contain things we don't need here. What seems to be pointedly missing in test coverage. Eg. setting $conf stuff to something and then verifying it working and similar for language varied values.

chx’s picture

- Configuration overrides (Drupal\config\Tests\ConfigOverrideTest)

I can't write the language test. Please do.

chx’s picture

Component: configuration system » overlay.module
FileSize
1.27 KB
12.73 KB

This adds a test. You might scoff at it but it's a valid test -- we do not need to test the language system itself.

chx’s picture

Component: overlay.module » configuration system

Erm.

Anonymous’s picture

setOverride() doesn't look right.

seems it could legitimately be called by both init events and locale events, with different values to override in each case. but we clobber $this->overrides, which doesn't seem to be what we want? maybe i'm just missing something, but perhaps we need something like:

  public function setOverride(array $data) {
    $this->overrides = NestedArray::mergeDeepArray(array($this->overrides, $data));
    $this->resetOverriddenData();
    return $this;
  }

just a note that using special filenames where we eat about 20 chars could bite us because of our filename length constraints.

gdd’s picture

Status: Needs review » Needs work

Initially I was a little wary of the Subscriber model for this because it seemed more complicated, but its actually pretty elegant seeing it in action. Overall I have to say I like this implementation.

@beejeebus' comment makes sense to me.

A somewhat related question I had, which is when two objects want to override the same data, how do we specify which subscriber goes last? What I found out was that in the following code:

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.phpundefined
@@ -0,0 +1,53 @@
+    $events['config.load'][] = array('configLoad', 20);

The second component of the array (20) is a priority, and subscribers execute in order of priority. It states that 'higher numbers are more important' which I assume means they execute later? Regardless, this is something we're going to want to make sure we pay attention to. Right now it has $conf with a higher priority than Locale which seems appropriate.

Barring any followup reviews I don't really have any problems.

Anonymous’s picture

working on adding #44 to the patch.

Anonymous’s picture

re. #45 and the priority of events - i don't think it's that simple in this case, because we can have multiple listeners across multiple events.

this patch adds code that runs on both the 'config.init' event (global conf override) and the 'config.load' event (locale override). as 'config.init' will always fire first, overrides added by 'config.load' will always win.

we have to consider both the priority of listeners within an event, and the order of different events across an object. so, as the patch stands, locale overrides override global conf overrides. perhaps we should collapse both of these subsystems to use the same event, something like 'config.override' ?

Anonymous’s picture

Status: Needs work » Needs review
FileSize
15.01 KB

ok, here's a patch that addresses #44.

- added a GlobalConfConfigOverride test
- changed setOverride() as per #44

i haven't attempted to address #47, i think that needs more discussion, and could possibly be a follow up.

Status: Needs review » Needs work

The last submitted patch, 1646580-48-config-event-listeners.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
13.22 KB

oh dear, lots of PEBKAC in #48's patch. new fixed patch:

- removed my changes from LocaleConfigOverride test
- remove redundant GlobalConfConfigOverride test

chx’s picture

FileSize
655 bytes

Note from IRC: "GaborHojtsy:#drupal-contribute> chx_afk: good test :)". I attached an interdiff between #42 and #50. Basically, it is only possible to add overrides, not nuke existing ones.

gdd’s picture

Status: Needs review » Reviewed & tested by the community

I think we should go forward with this patch as is, and I've created a followup to discuss prioritization of events and listeners at #1741984: Figure out priorities for config event listeners.

moshe weitzman’s picture

I don't see any test or docs that describe how to use ConfigGlobalOverrideSubscriber

chx’s picture

See core/modules/config/lib/Drupal/config/Tests/ConfigOverrideTest.php for a config override test.

moshe weitzman’s picture

@chx - thats not in latest patch AFAICT

chx’s picture

No, it's in HEAD

moshe weitzman’s picture

chx informs me that this test is already present in core, so of course it is not in this patch. still rtbc.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

So this is normally one of those patches that I'd defer to Dries or catch on, but chx and Gábor actually patiently explained this to me today on the floor outside the Core Conversations room for a half hour. :) Essentially, this patch allows us to fire off hooks before we can fire off hooks (e.g. before system.module is loaded) using the Symfony Event Dispatcher system. This is really important for the basics of multilingual configuration, so....!

Committed and pushed to 8.x. YAY!

I saw a couple of very minor things but nothing that should hold up commit, but if someone could roll a follow-up that'd be great.

+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigGlobalOverrideSubscriber.phpundefined
@@ -0,0 +1,39 @@
+ * Definition of Drupal\Core\EventSubscriber\ConfigGlobalOverridesubscriber.
...
+class ConfigGlobalOverridesubscriber implements EventSubscriberInterface {

ConfigGlobalOverrideSubscriber (capital S)

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.phpundefined
@@ -0,0 +1,53 @@
+ * Definition of Drupal\locale\LocaleConfigsubscriber.
...
+class LocaleConfigsubscriber implements EventSubscriberInterface {

LocaleConfigSubscriber (capital S)

I thought there was something else too but it's late and I can't remember. :P

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.36 KB

there you go webchick.

chx’s picture

Status: Needs review » Needs work

At least the @file needs changing too. Does the subscriber addition need fixing too?

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.77 KB

woops, fixed that too.

chx’s picture

Status: Needs review » Reviewed & tested by the community

That's OK. I checked and all registering does use ConfigGlobalOverrideSubscriber and LocaleConfigsubscriber.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thank you!

Committed and pushed that small follow-up to 8.x. Sorry I didn't just do that myself but I wasn't sure if renaming them might break anything else and wasn't in a position @ DrupalCon to blindly commit something. :)

I don't think this needs a change notice, since these are all internal changes? but if I'm wrong, please feel free to mark back to "active" and "critical task"

catch’s picture

Gábor Hojtsy’s picture

Do we need a change notice or changelog.txt entry for this? I don't think change notice is applicable because this is extension of entirely new functionality. How best to document those new capabilities? A changelog.txt entry in a very general sense like "Added the ability to override configuration values with language variants runtime." would possibly be good. Agreed, not agreed?

sun’s picture

I think that some high-level info should go into the existing primary Config system change notice, but ultimately, this entire thing will need a fully fledged handbook chapter (comparable to the one of DBTNG).

In other words, it doesn't make sense to me to litter the information about how this system works into a dozen of change notices. Instead, we need a single one that (quickly) informs about the upgrade path from variables to config and state, and ultimately points to the handbook for in-depth explanations.

I've added this issue to the existing change notice: http://drupal.org/node/1500260

(And btw, that change notice is heavily outdated... ;))

Gábor Hojtsy’s picture

Priority: Major » Normal
Status: Fixed » Reviewed & tested by the community
FileSize
703 bytes

Ok, here is a slightly reworded (from my above proposal) changelog entry for this functionality extending the CMI section not the language section :) Should be good once this committed to move off of D8MI sprint then.

jhodgdon’s picture

Assigned: chx » jhodgdon

I can get that committed sometime soon. :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Changelog is done... back to fixed!

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: +language-config

Add missing language-config tag.

Gábor Hojtsy’s picture

#1763640: Introduce config context to make original config and different overrides accessible made some significant changes as to how this is integrated in the system (retaining its functionality). I've added one more change notice to this too at http://drupal.org/node/1928922 and wrote extensive docs on how does the config override and context system work (together). See http://drupal.org/node/1928898

Gábor Hojtsy’s picture

Issue summary: View changes

Updated according to #9

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: +Configuration context