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
Comment | File | Size | Author |
---|---|---|---|
#67 | cmi-language-changelog.patch | 703 bytes | Gábor Hojtsy |
#61 | 1646580-61.patch | 1.77 KB | Anonymous (not verified) |
#59 | loclulz.patch | 1.36 KB | Anonymous (not verified) |
#51 | interdiff.txt | 655 bytes | chx |
#50 | 1646580-50-config-event-listeners.patch | 13.22 KB | Anonymous (not verified) |
Comments
Comment #2
alexpottTagging issue to make it easy to find
Comment #3
gddThere is some discussion in #1186034: Length of configuration object name can be too small that impacts this issue in terms of file naming.
Comment #4
Jose Reyero CreditAttribution: Jose Reyero commented@heyrocker,
Thanks for the heads up.
Anyway this should be postponed till the rework gets in, #1605324: Configuration system cleanup and rearchitecture
Comment #5
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated 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'.
Comment #6
neclimdulThese 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.
Comment #7
Jose Reyero CreditAttribution: Jose Reyero commented@neclimdul,
Yes, that makes sense, I'm still learning the basics about that Events/Suscribers.
Comment #8
Jose Reyero CreditAttribution: Jose Reyero commentedChanging 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
Comment #9
Jose Reyero CreditAttribution: Jose Reyero commentedYeah! 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:
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.
Comment #9.0
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated issue summary.
Comment #10
Jose Reyero CreditAttribution: Jose Reyero commentedUpdating title and tagging
Comment #11
neclimdulI 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.Comment #12
neclimdulnever mind, its already being discussed here #1599108: Allow modules to register services and subscriber services (events) we should just be aware :)
Comment #13
chx CreditAttribution: chx commentedSo 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.
Comment #14
chx CreditAttribution: chx commentedSo 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.
Comment #16
chx CreditAttribution: chx commentedRerolled against HEAD.
Comment #18
Jose Reyero CreditAttribution: Jose Reyero commentedThough 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
Comment #19
chx CreditAttribution: chx commentedPlease 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?
Comment #20
Jose Reyero CreditAttribution: Jose Reyero commentedWell, 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...
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.
Comment #21
chx CreditAttribution: chx commentedTo 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?
Comment #22
Jose Reyero CreditAttribution: Jose Reyero commentedWe 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'.
?
Comment #23
chx CreditAttribution: chx commentedOh then it's a little bit misleading variable name! sorry. I thought $locale_name was just 'es'. Makes a lot more sense now.
Comment #24
chx CreditAttribution: chx commentedWe had a long phone call and we will go ahead with Jose's patch and eventually rip out the StorageDispatcher.
Comment #25
Jose Reyero CreditAttribution: Jose Reyero commentedOk, 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.
Comment #26
chx CreditAttribution: chx commentedIt 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.
Comment #27
chx CreditAttribution: chx commentedThis is a reroll and a small refactor of #9. I know the Configuration tests pass so I hope everything else will.
Important changes include:
drupal_container()
we create an event dispatcher too because the one in the core bundle is too late.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.
Comment #29
chx CreditAttribution: chx commentedIt 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.
Comment #31
chx CreditAttribution: chx commentedOh, language(). Edit: wrong interdiff, this is the real one:
Comment #32
chx CreditAttribution: chx commentedI 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.
Comment #33
Gábor HojtsyFirst and foremost should be on the D8MI sprint.
Comment #34
Gábor HojtsyOn 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.
Comment #35
Gábor HojtsyAlso giving some justice to the level of importance (if not critical :).
Comment #36
sunThanks 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.
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
?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.
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.
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.
Comment #37
sunComment #38
chx CreditAttribution: chx commentedI 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.Comment #39
chx CreditAttribution: chx commentedThat results in quite a smaller, cleaner patch. Thanks again.
Comment #40
Gábor HojtsyReviewed 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.
Comment #41
chx CreditAttribution: chx commented- Configuration overrides (Drupal\config\Tests\ConfigOverrideTest)
I can't write the language test. Please do.
Comment #42
chx CreditAttribution: chx commentedThis adds a test. You might scoff at it but it's a valid test -- we do not need to test the language system itself.
Comment #43
chx CreditAttribution: chx commentedErm.
Comment #44
Anonymous (not verified) CreditAttribution: Anonymous commentedsetOverride() 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:
just a note that using special filenames where we eat about 20 chars could bite us because of our filename length constraints.
Comment #45
gddInitially 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:
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.
Comment #46
Anonymous (not verified) CreditAttribution: Anonymous commentedworking on adding #44 to the patch.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedre. #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' ?
Comment #48
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 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.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedoh dear, lots of PEBKAC in #48's patch. new fixed patch:
- removed my changes from LocaleConfigOverride test
- remove redundant GlobalConfConfigOverride test
Comment #51
chx CreditAttribution: chx commentedNote 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.
Comment #52
gddI 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.
Comment #53
moshe weitzman CreditAttribution: moshe weitzman commentedI don't see any test or docs that describe how to use ConfigGlobalOverrideSubscriber
Comment #54
chx CreditAttribution: chx commentedSee core/modules/config/lib/Drupal/config/Tests/ConfigOverrideTest.php for a config override test.
Comment #55
moshe weitzman CreditAttribution: moshe weitzman commented@chx - thats not in latest patch AFAICT
Comment #56
chx CreditAttribution: chx commentedNo, it's in HEAD
Comment #57
moshe weitzman CreditAttribution: moshe weitzman commentedchx informs me that this test is already present in core, so of course it is not in this patch. still rtbc.
Comment #58
webchickSo 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.
ConfigGlobalOverrideSubscriber (capital S)
LocaleConfigSubscriber (capital S)
I thought there was something else too but it's late and I can't remember. :P
Comment #59
Anonymous (not verified) CreditAttribution: Anonymous commentedthere you go webchick.
Comment #60
chx CreditAttribution: chx commentedAt least the @file needs changing too. Does the subscriber addition need fixing too?
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commentedwoops, fixed that too.
Comment #62
chx CreditAttribution: chx commentedThat's OK. I checked and all registering does use ConfigGlobalOverrideSubscriber and LocaleConfigsubscriber.
Comment #63
webchickAwesome, 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"
Comment #64
catchDid anyone profile this? See berdir's last post on #1187726: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects).
Comment #65
Gábor HojtsyDo 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?
Comment #66
sunI 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... ;))
Comment #67
Gábor HojtsyOk, 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.
Comment #68
jhodgdonI can get that committed sometime soon. :)
Comment #69
jhodgdonChangelog is done... back to fixed!
Comment #70
Gábor HojtsyThanks! Removing off the sprint. Thanks all! Now turn energies on #1648930: Introduce configuration schema and use for translation and #1763640: Introduce config context to make original config and different overrides accessible.
Comment #72
Gábor HojtsyAdd missing language-config tag.
Comment #73
Gábor Hojtsy#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
Comment #73.0
Gábor HojtsyUpdated according to #9
Comment #74
Gábor Hojtsy