Updated: Comment #19
Problem/Motivation
The current configuration override system is very powerful, but very complicated. The current system provides far more flexibility and overhead than is required to support locale overrides and arbitrary module-supplied overrides.
We currently fire off two events per config object during a read. Contrast this to D7, where there are no read-time events at all for the variable system.
The configuration system underlies most parts of Drupal, and should be as simple and fast as possible.
Proposed resolution
- bake in locale override support to the config system. this requires setting the language on the config factory - when set, the config system will automatically pick up overrides by looking for 'locale.$langcode.$config_name' objects
- provide a single override event to modules, to allow them to provide arbitrary overrides once per request. this is the equivalent of overriding global $conf in D7
With the current system you can't cache objects together as you have nfi what overrides might've happened. With the new patch, you put the language override in the cache key and you are done: we can cache raw config plus language overrides. That's a massive performance win even if we can't cache $conf / arbitrary overrides -- those will need to have their own caching indeed.
Remaining tasks
Finish the initial patch, get it in.
User interface changes
None.
API changes
- complete removal of all ConfigContext objects and interfaces
- add an override event fired once per-request to allow modules to provide their overrides
- addition of getLanguage() and setLanguage() to to Config and ConfigFactory objects
- changing global settings.php overrides to use the new override event
Related Issues
Original report by beejeebus
Unless I was dreaming, we decided to replace the config context system with baked-in locale support and single-event based overrides.
This issue will track that. We may want a couple of smaller issues.
Comment | File | Size | Author |
---|---|---|---|
#149 | 2098119.149.patch | 163.95 KB | alexpott |
#149 | 145-149-interdiff.txt | 4.57 KB | alexpott |
#145 | 2098119.145.patch | 163.36 KB | alexpott |
#145 | 137-145-interdiff.txt | 978 bytes | alexpott |
#137 | 2098119.137.patch | 163.31 KB | alexpott |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's the last patch i wrote to stop the config context system going in the first place.
https://drupal.org/files/1763640-83.patch
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #3
Dave ReidComment #4
Dave ReidHard to tell what is being proposed or how things will change here for an outsider. Definitely would be nice to run this by agentrickard.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedi'll update the issue summary today, and hopefully provide a first-cut patch.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's a WIP patch that at least installs, and rm -rf's the ConfigContext code.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commented#6: 2098119-first-cut.patch queued for re-testing.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's another go without the durp durp debug.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's another go, that removes config_context_* calls.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedthis one might get us closer to no fatals.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedthis should fix some of those fails.
Comment #18
tstoecklerI'm not sure who agreed on what here, but there are use-cases outside of core for the config context system. environment-specific overrides and domain.module would be the two that come to mind immediately.
Can you elaborate on the approach taken here and the reasoning behind it? Thanks!
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedthis should fix the 'no private stream wrapper' fails. actually restores the global overrides.
Comment #19.0
Anonymous (not verified) CreditAttribution: Anonymous commentedFixing typo
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedadd back locale overrides. fix config import tests.
Comment #23
tstoecklerTo expand on #18 I do not see global overrides handled in this patch.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedyep, because i haven't gotten to all the things yet. but don't worry, the layered cake is coming.
config on disk overridden by config in locale files overridden by config overrides from modules overridden by $settings in settings.php.
Comment #25
catchMarked #1859110: Circular dependencies and hidden bugs with configuration overrides as duplicate. Bumping this to critical.
Comment #26
tstoecklerOK. Would still be great to have some reasoning for this.
Comment #27
catch@tstoeckler one of the main reasons is #1880766: Preload configuration objects. To have efficient caching of configuration objects, we'd need to make the config override event act on multiple configuration objects at once (similar to how hook_node_load() changed in Drupal 7 from single to multiple). This means a big API change for anything using the context system anyway.
With the approach here, domain and any other non-locale use-case still gets to override, but there is one event fired to load all overrides, not per-configuration object, but the common cases are considerably simplified.
Comment #28
Gábor Hojtsy@tstoeckler: see, the config override/context system is sufficiently complex in Drupal 8 much more so than in Drupal 7. People from the config initiative, etc. pointed out that we don't *need* to extend the override/context possibilities, Drupal 7 had domain module, etc. and they could still maintain their hackish ways of working through global overrides. Language would be special cased with first class support much similar (in concept) to fields. So other kinds of overrides would need to hack around the system like in 7 and no official specific support.
This was discussed on the CMI meeting at DrupalCon and we decided to get feedback from @agentrickard and others who already use the context/override system. We managed to summon @webflo who already has a language-domain combination module and he was fine with the changes proposed here. We should still get feedback from @agentrickard. I'll tweet him :)
For me personally and for languages, this is not a step back, so in terms of language support, we should have the same level of features as before this patch. It is a step back from the D8 level to the D7 level for other overrides OTOH simplifying the API a great deal at the same time.
Comment #29
Gábor HojtsyDone: https://twitter.com/gaborhojtsy/status/385383226837336065
Comment #30
Gábor HojtsyLooked at language related stuff. There are obviously lots of dev code in this patch, which I did not call out :)
It would be great to name this something more generic, eg. languageOverrides. While locale manages some of the overrides, these overrides are really about language variants. Fields don't call them locale either :)
Eg. config translation lets you translate configuration even for those elements where locale does not apply (source not English and/or config not in install storage), where locale module has no reach.
So while the event listener was (is?!) in locale module, the overrides are language specific.
It is theoretically possible that a site without locale but with config translation would make sense, so we may want to move the listener to the language module altogether to round this off.
At other places language as the override criteria is nicely identified :) ++
See the problem language vs. locale :) Probably best to rename the file scheme as well to language.config... or language.override... :) Would probably be self explanatory at that point vs. magic :D
Comment #31
agentrickardBusy week for me. The current D8 pattern works nicely for my use-case, because it allows for deployment support for overrides.
I have existing tests that I can refactor against the patch, but there are two things in the current D8 that I'd like to retain:
1) Don't make module authors hack settings.php. That makes code untestable.
2) Allow for export of overrides. That makes deployment easier.
Comment #32
agentrickardI should say, too, that I can probably live with firing _after_ language has, though it is currently possible to override default language per domain.
To make that really work, core's domain-based language configuration needs to support pattern matching (so that multiple domains can be mapped to a single language default.)
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedre #31
1) yep, there will be no need for that.
2) this depends on where you store your overrides :-) but if they are in the active store (as the locale overrides are), then they will be imported/exported like all other config.
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commenteds/locale/language/g.
i've left the config name as 'locale.' ... to keep the patch smaller.
Comment #36
Gábor Hojtsy@beejeebus: thanks, we can do the locale prefix rename later on :) No need to do everything at once. Was just concerned that there is some consistency in the newly introduced stuff :)
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's another go, hopefully this one actually gets through to a test run.
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 3 layers of the 4 layer layer cake in, all config tests pass locally.
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commenteddurp left in debug.
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentedthis should have a few less fails.
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedback out the ConfigFactory event changes. ConfigFactory should implement an identityMap for config objects.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedrestore the hack to not return 'deleted' objects from the static cache. FML.
Comment #49
Jose Reyero CreditAttribution: Jose Reyero commentedReplying mostly to the issue summary:
A truly lovable goal. I just don't understand why if the configuration system is a major mess, we want to axe the part of it that allowed moving some of that complexity into optional modules.
Yes, but that was before the ConfigContext was added. Multiple loading of config objects is nice, I don't see what it has to do with context and locale. And also I don't see how it's going to help performance since most config objects are loaded one at a time...? You should also check which part of the configuration is loaded before language initialization, which part is loaded after...
If you want to add languages into the config system -which won't make it simpler-, it should take care of other events too, like config renaming too (which needs renaming the overrides). Baking everything in the config system will make it faster for sure, but only for sites that needed localization before.
Btw the context system served another purpose too, which was not getting overrides for configuration to be edited in admin pages, just a reminder because I don't see it handled in this patch.
Then about other modules adding config overrides without providing them with a proper "context" to do that ok, I guess that may work (Though I don't see how in the patch) but we are caching that configuration -with overrides?- only per language.
Apparently I've missed the important decision here so nothing else to add and I won't post on this issue anymore. Good luck with simplifying.
Comment #50
Gábor Hojtsy@Jose: thanks for your comments! The discussion that was mentioned took place in Prague, see notes on different meetings at http://bit.ly/prague-notes (CMI meeting at https://docs.google.com/document/d/16xkqnemsGllTrqQoUFs56BvmPgN6L0gcQP36...). Although the notes are not very exact, at some places we lost track of keeping copious notes, we discussed the complexity of caching config if there may be any number of conditions that make the config vary and that if we try to cut down on the conditions at least as it appears to the config system, then caching will be simpler. So a step back basically to the Drupal 7 days for overrides *except* for language, which would be explicitly supported (a step up from the Drupal 7 system in this case still). I represented the multilingual needs, and explicit support for language will serve this need. We asked for @webflo to come in later since he built a locale/domain combination context module and @agentrickard looked at this patch above in the comments. They were existing implementers of the context API.
This patch is indeed making config itself more complex on the expense of removing the more complex context system, so stepping forward less from Drupal 7 that the context system would have done. As for not getting overrides for the admin pages, the context system is not yet properly used for that, eg. we recently resolved #2094797: Config entities upcast from the request always get default language context and still to get #2099541: ConfigEntities should not load the Entity translated on Edit Forms committed. These are backed with tests, so whatever this patch does will need to provide solutions for that and pass those tests :)
You are right the cache will be only per language, and other conditions will not be taken into account. That is kind of the goal of this patch. It will be dumber and not cache by other considerations, so og, domain, etc. will need to be aggressive in clearing caches or will need to inject a more intelligent/specific cache solution or sidestep this cache altogether and be slower I guess.
Comment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedok, i'll stop on this and wait for the discussion to be had again.
Comment #52
Gábor HojtsyOk, so let's see what is the config context system intended to do:
1. Inform overrides whether they should kick in and provide data on how (eg. language: "de", etc.)
2. Inform any caching system that the overrides were computed with the given conditions.
This proposed patch removes knowledge from the config system about the conditions of overrides as well as any way to tell how granularly to cache. So as a result, the caches will not be reliable at all on any site where overrides based on some condition (outside of language) is present. Such as sites using OG or domain. Is that a fair assessment?
The lack of knowledge about override sources was not a problem at all in Drupal 7 and modules just kept putting stuff into $conf to override. However, there was no need to *cache* the runtime value of $conf, since it was still relatively small, at least compared to the extent of where the config system is used in Drupal 8. So the caching requirement came about because it is used for so many things. In this situation, the less we know about the conditions under which the cached data was produced, the less reliable is the cache, and modules like domain or groups will need to clear that cache and start fresh on every page load, since they never know if the data cached was under the condition they are operating in on the current page.
The question then is leaving in the context system in place would actually help? With such massive context values like a user object or language object do we even know what were the *exact* conditions that triggered differences in the config? Was it the user's ID or a value of a profile field (age?), was it the RTL-ness of the language and not the language code per say? Yeah I don't think we would get that level of information with the current system. However, we would still gain at least a way to generate a hash out of all the context objects and key our cache by that. OTOH with the user object granularity that would quickly lead to way too many cache entries when individual user objects are combined with other objects, etc.
Is it totally pointless to fix issues that are not due to the override/context system in a way that would still be useful if we eventually do away with the even/context system? Eg. the two different events can be done away even with the current system as far as I remember the code :) Can we rethink the context system in a way where the values can actually be informing caching in a useful way (which they don't right now) but still inform the override listeners to get the most benefit and not make groups and domain clear caches on all page loads? Is my claim that they would need to clear those caches on all page loads even true? :) @catch, @alexpott, @beejeebus?
The worst thing to be in right now is a stalemate where we don't move anywhere, because major bugs like #1966538: Translation is not updated for configuration, when the config changes cannot be moved on until this is settled :/
Comment #53
catchMy impression was that the new override system happens every request, and it's up to domain or similar to cache their overrides - similar to overriding conf.
That means CMI can reliably cache by language, and any other override system has to do its own caching. If you have a massive number of domain-specific overrides that could get slow, but even that will likely be outweighed by being able to multiple/pre-load CMI objects efficiently anyway, which the current context system makes almost impossible. @beejeebus please correct me if I'm wrong.
Also completely agreed that just having a 'user' context is completely useless for generating a cache key, it makes everything per-user whereas per-role or even less granular than that might be fine. Again that's much, much more damaging for performance than the limitation of overrides having a separate cache and one/two events firing instead of multiple.
Comment #54
Gábor Hojtsy@catch: ok, that is not explained in the issue summary and I did not manage to wrap my head around the patch to understand that. It would be good to get an overall explanation of the proposed new system so such details are clearer :) We know the limitations / issues in the current system, but lack an overview of the new system.
Comment #55
tstoecklerCan someone explain to me why the context system makes pre-loading/multi-loading harder?
If the language override subscriber gets passed an array of config names to load, it can easily figure out the a list of language override files to load.
I don't have a very clear vision of how the config pre-loading system is supposed to work, but I feel like we haven't really explored making the config context system work with it.
Comment #56
chx CreditAttribution: chx commentedWith the current system you can't cache objects together as you have nfi what overrides might've happened. With the new patch, you put the language override in the cache key and you are done: we can cache raw config plus language overrides. That's a massive performance win even if we can't cache $conf / arbitrary overrides -- those will need to have their own caching indeed.
This issue got stalled on not clarifying this, now that we have that please continue.
Comment #56.0
chx CreditAttribution: chx commentedI want a pony. Edit: this comment was not written by chx.
Comment #57
tstoecklerYes, that's in fact very helpful. It still *feels* like we could have some facility to find out which contexts have been applied and sort out the caching in a generic fashion, but since this is a complete re-write anyway, I guess it makes more sense to explore that in a follow-up.
Comment #58
catchAlso having separate cache items isn't necessarily a bad thing.
When caching everything together, if there's an arbitrary override for the request and it's a cache miss, then both the global configuration and the overrides would be a cache miss. (you could get around this with two layers of caching perhaps but that's even more work when everything is a cache miss).
With separate cache items for global config and overrides, the global configuration would be a cache hit assuming any other request to the site has retrieved those already, and only the overrides have to be built from scratch.
Comment #58.0
catchUpdated issue summary.
Comment #59
Gábor HojtsyI emailed with Jose and he expressed that he indeed does not have hard feelings about this issue and most core decisions can now be worked around in contrib using the dependency injection container so I'd say don't feel blocked on that :)
I also started to use a new "Configuration context" tag for issues related to the introduction and the bugs around context as we find them. There are ongoing bugs, eg. you see config entities localised then go edit them they are different (#2136559: Config entity admin lists show configuration with overrides (eg. localized)), or derivative data based on configuration may not be in the original language (#2115025: Content admin views title saved localized to the menu table), or more needs to dynamically jump in and out of language contexts at some places (#2107427: Regression: Language names should display in their native names in the language switcher block). We cannot really put all these in a "wait and see" state, they need to be resolved. The more we resolve of those, the more config context use will expand in core. The less we fix them, the more confusing user experiences remain. I think the set of issues with this tag will be useful: https://drupal.org/project/issues/search?issue_tags=Configuration%20context
Comment #60
Anonymous (not verified) CreditAttribution: Anonymous commentedre. the caching questions in #52 - config context or not, i don't think we should try to build a system that lets modules provide overrides based on runtime php AND try to handle that with a central caching layer.
what i'm trying to do with this patch is move language overrides into the core CMI system. this allows us to also handle language override caching in one place.
overrides that come from arbitrary module code, however, are on their own when it comes to caching.
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's a reroll. i've only tested that it installs.
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commenteddurp durp oh hai bot.
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedmkay, here's another go.
- removed all the config_context_* calls in new files
- actually added the module override functionality
still to do:
- address the config_override_* bits i took out that are not covered by config_factory_set_language($language)
- add tests
- add an API for loading raw config data, and figure out a pattern for config entities to use it
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #67
Grayside CreditAttribution: Grayside commentedOverriding configuration is a key use case for Distributions still. I was hoping not to need any configuration architecture built into the Features 8.x but it sounds like that's not to be. Is it reasonable that Drupal products require contrib modules as part of their architecture?
Comment #68
Anonymous (not verified) CreditAttribution: Anonymous commentedthere is literally nothing in this patch that removes configuration overrides.
this patch changes the implementation, but not the functionality. modules can still override config in arbitrary ways.
Comment #69
Anonymous (not verified) CreditAttribution: Anonymous commentedok, new patch, adds ConfigFactory->disableOverrides() and ConfigFactory->enableOverrides() plus procedural wrappers, and uses that where we used to use the null config context code.
Comment #71
Grayside CreditAttribution: Grayside commented@beejeebus, right, so if a distribution wants to manage or advise managing overrides that are not completely wild, the additional architecture needed will need to come from contrib. The configuration context mechanism looked like it would provide at least half that.
I totally understand not wanting to keep this in core, I'm just trying to understand where the lines are for CMI & distribution use cases so I can know where it is I should be focusing my efforts.
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commented@Grayside - "I totally understand not wanting to keep this in core..." is just totally off the mark.
keep what? this patch does not remove config overrides. at all. we may both be using the word "overrides" but meaning different things.
perhaps ping me in IRC?
Comment #73
Anonymous (not verified) CreditAttribution: Anonymous commentedwho knows, the tests may even run with this patch.
Comment #75
Anonymous (not verified) CreditAttribution: Anonymous commentedok. let's just allow a null language to config_factory_set_language().
Comment #77
Anonymous (not verified) CreditAttribution: Anonymous commentednew patch adds a test module to exercise the 'config.module.overrides' event, should fix some fails.
Comment #79
Anonymous (not verified) CreditAttribution: Anonymous commentednow with 100% less ffs() durp.
Comment #81
Gábor HojtsyContent entities have been recently changed, so they work like this:
- if you load the entity, you get the original version of the entity
- then you have a $entity->getTranslation($langcode) where you can ask for a different version
- anything you access on that is then in that language
If we want to hardcode language so much, maybe we want to follow the same / similar approach, so then the entity would know what language it is in (radical! :D) and could load a different version of itself also if needed. Not sure if that is in scope for this patch, but since you want to hardcode language overrides as a base feature...
Comment #82
alexpottAs this patch will make significant changes to the config API this is beta blocking.
Comment #83
chx CreditAttribution: chx commentedComment #84
chx CreditAttribution: chx commentedDown to 55 fail(s), and 7 exception(s). We shall overcome.
Sneak peek of a 14 fails, 4 exceptions version.
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous commentedok, this might be all greeen.
Comment #88
Anonymous (not verified) CreditAttribution: Anonymous commenteddammit.
Comment #90
jhodgdonYou're not planning to commit a patch with
in it, right? :)
Comment #91
Anonymous (not verified) CreditAttribution: Anonymous commentedok, this one came back green on the patch test issue.
Comment #92
chx CreditAttribution: chx commentedNice job, Justin!
Comment #93
andypost#90 still needs to be cleared.
Is this really needed?
Comment #94
Anonymous (not verified) CreditAttribution: Anonymous commentedyep, #90 is right, i'll remove that in the next roll of the patch. more review welcome!
Comment #95
Anonymous (not verified) CreditAttribution: Anonymous commentedjust a reroll now that #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct is in, hopefully i didn't break stuff.
Comment #97
chx CreditAttribution: chx commentedComment #100
chx CreditAttribution: chx commentedComment #102
chx CreditAttribution: chx commentedComment #103
alexpottLet's not introduce more helper functions to get rid of later - let's just call
Drupal::configFactory()->function()
since we're addingDrupal::configFactory()
in this patch! I think we should also removeconfig_factory_set_language()
that is also added by this patch.No longer needed - has been removed.
The changes here appear to always read configuration regardless of whether we've already read it or not. The current logic here and in loadMultiple() means that we'll cache completely new configuration objects when we're in override free mode but not existing ones which is odd.
Not used this is baked into the loadMultiple() function. I think this actually might be a case of premature optimisation because the configuration directory is going to become a nightmare to manage on multilingual sites so we might want to consider using a sub-directory. The optimisation of baking this into loadMultiple() is going to make this tricky.
Unnecessary.
Removing this negates the point of the test.
This looks pointless now.
Patch attached addresses most of this. It will fail - but the failure I know about we've got to fix - just didn't have time to explore further.
Comment #105
alexpottFixing exceptions
Comment #107
alexpott105: 2098119.105.patch queued for re-testing.
Comment #109
Anonymous (not verified) CreditAttribution: Anonymous commentedthanks for the review.
#103.1 - yep, that looks like a nice improvement.
#103.2 - ack.
#103.3 - nice catch, the intention was to make all the things run through loadMultiple(), but i guess i messed up integrating the different behaviour required for get() (loadMultiple() shouldn't return objects that don't already exist, whereas get() will return new objects in that case).
#103.4 - i'm ok with taking that function out, but i don't agree at all about the 'premature optimisation'. merging storage queries was a big, big win for the configuration system, and i don't want to back that out. please keep the discussion about managing the config dir with subdirs out of this issue - i'm not opposed, but it's nothing to do with this patch.
#103.5 - ack.
#103.6 - can't remember why i took that out, seems to pass now so yay :-)
#103.7 - working on some better tests now. also, the only fail with #105 seems to be a broken head, not this patch.
Comment #110
alexpott105: 2098119.105.patch queued for re-testing.
Comment #111
Gábor HojtsyI was afraid my uber-review would get lost, so only reviewed the first 3rd so far (up until not including config importer). Hopefully some helpful stuff in there :)
Better docs would be to explain that this is for easier global Drupal::configFactory()->enableOverrides() and disableOverrides(). Not just for languages :)
We don't usually capitalize the class name (ie. document the class with itself :). Also, this is not really used to take overrides from or anything like that. It just informs us which language overrides are active, right?
Note: appears in multiple files/places.
There is no $overrides anymore right? There only seems to be languageOverrides and moduleOverrides properties, so I think this is just leftover in the docs.
The "for" got lost resulting in a broken English sentence.
Would be useful to document what this is keyed by. May not be evident.
May be the same problem on setSettingsOverride(), but that was not in the diff scope in the patch.
It is not really true anymore that locale provides these override files. Eg. config translation also provides such files. It is also not true anymore that an event listener in locale module would put these overrides in, this is now working with the native config factory without locale module even enabled AFAIS. So why not name it language.config.LANGCODE.NAME like in the docs. :)
Note that the docs and the code don't match in terms of the filename probably due to this confusion.
Also in the textual description of the priorities the same thing: name these language overrides (which they are) instead of locale (which they are not, especially not anymore).
This doc on Language looks better :) But should be documented as (optional).
Also the language default behavior should be documented. Something like this:
"Each configuration file that may contain human readable text should have a default language (langcode: fr) in the file. If no explicit language overrides are requested or the language override is not available, the default configuration is retrieved."
Not sure it should be documented right here, but somewhere :)
"a given names"?
Would probably be important to document that this may be at any position. Eg. 'page' would match 'frontpage'. I don't think this may be a problem due to the namespacing best practices we suggest, but best to document.
Should fully qualify these class names in docs.
Would this not need to clear the static cache too? Since some of the config caches may contain config overrides from the previously set language?
Same docs/code mismatch in terms of the file names. I vote for the docs to be right and the code to be fixed as per above :)
Comment #112
Gábor HojtsyRemaining part of my review. Please read the whole thing before starting to respond, I got some revelations midway :)
Lacks file level and class level phpdoc.
Same as in previous comment.
Would be useful to document that this is keyed by config name IMHO. Eg. only one override value possible per name :)
Why is this in the patch?! Seems unrelated?
All right, this is a bit alarming for me. This means that the new config get() behavior by default would not return the config value with the language overrides for the page but instead the original values, right?
Since you are not turning overrides off anymore and still not expecting overrides.
I went back and looked at the config factory initialization on whether it initializes with the current page language, but that does not seem to be the case(?) I may be missing something here, since the config translation tests (if nothing else) should ensure that this is still happening and config overrides are applied wholesale by default. What am I missing?
Also, I would suggest that here we should turn off all overrides, eg. this would make config change on settings form based on organic groups or domain (and save back to the original config overwriting the original values).
Whether the global settings.php overrides apply is a different question maybe and your proposal does not really provide a way to make sure those apply but other's don't. In #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects it has been argued that settings.php overrides are "security features" and should not be skipped. Maybe postpone to there but keep skipping overrides here.
The patch introduces Drupal::configFactory() to make this shorter, right? :)
Also, same as above, the presence of setting the default language on the factory is surprising to me. Why would you need to do that explicitly?!
Where is the replacement test which ensures the priorities of module / language, etc. overrides work as expected?
Copy paste incorrect :)
Why remove these? Where is this already tested?
Incorrect docs. The file level docs are surely copy-paste the class level doc seems to be incorrect. This is not really testing anything, it just provides data for the test.
No, this is good. :) We could probably remove the isMultilingual() also, and it would work the same but maybe a little slower. Eg. it would look for English overrides on English only sites at all times. That is probably overkill. You can also remove the Language::TYPE_INTERFACE, since that is the default.
Also this answers some of my questions above about where the site language is set :) And makes me happy our tests cover it well. Sorry, it is very hard to go back now and review all my comments in dreditor :/
Copy-paste first phpdoc line :) Not using the right class name.
Also, missing argument docs.
Comment #113
Anonymous (not verified) CreditAttribution: Anonymous commentedthanks for the review Gabor! this is definitely needs work then :-)
Comment #114
Anonymous (not verified) CreditAttribution: Anonymous commentedok, this patch addresses the functional parts of Gabor's review.
still need more tests, just want to see where this patch is at.
Comment #115
Anonymous (not verified) CreditAttribution: Anonymous commenteddurp durp durpal.
Comment #117
Anonymous (not verified) CreditAttribution: Anonymous commentedhmm, NFI what i did wrong with that last patch.
Comment #120
alexpottThe attached patched:
ConfigModuleOverridesTest
ConfigOverridesPriorityTest
config_override
LocaleAdminPathConfigEntityConverter
works. i.e. it only enters the override free mode to retrieve the config object. Once the config object is retrieve it immediately exits the override free mode.Comment #122
Anonymous (not verified) CreditAttribution: Anonymous commented114: 2098119-114.patch queued for re-testing.
Comment #123
Anonymous (not verified) CreditAttribution: Anonymous commentedsubmitted for restest in because the bot segfaulted.
Comment #125
alexpottFixed up test failures
Comment #126
tstoecklerWow, this is really impressive work. Kudos to everyone involved!
Found a couple things while reading through this.
The starting slash should be retained.
I think there should be a comman after "set", and the last "is" should be an "if".
Can be collapsed into one line.
This could use some docs. I personally don't understand it because loadMultiple() also futzes with
$this->cache
. I'm not saying the code isn't valid but it could do a better job explaining itself.This should be
$module_overrides
.I think this comment could be clearer. If I'm not mistaken what is meant is the following:
I was very confused when I first read this as the intent of this issue is (among others) to enable caching of language overriden config.
Comment #127
Gábor Hojtsy@beejebus / @alexpott: I like all the updates and the only thing I found reviewing the interdiff was the strange combo of camelcase and underscroring that @tstoeckler also noted in point #5 above. So looks good from my point of view and @tstoeckler's review points are also minor, so looks to be *very* close now :)
Comment #128
alexpottSo whilst fixing the minors reported in #126 (thanks @tstoeckler and @Gábor Hojtsy for the reviews) I realised that we have a couple of problems.
Firstly, overrides are not applied to completely new configuration objects. So I've added override code to
ConfigFactory::get()
to make this happen. I think we might need to add more comments to outline why we're baking overriding into bothConfigFactory::get()
andConfigFactory::loadMultiple()
since at first sight this looks simple to refactor but then we'd find it difficult to make some of the new language override optimisations inConfigFactory::loadMultiple()
.Secondly - in one of the comments we write
And this is completely true whilst we're applying language overrides to a config object. For example, when we do
Drupal::config('system.site')
and the language has been set to French or whatever. However it is false if we actually directly access the language config. Say we want to read the language override config and show on a form and edit it. We'd do somethingDrupal::config('language.config.fr.system.site')
and whilst doing ConfigFactory::loadMultiple on this it would get overridden. To fix this I've added a new method ConfigFactory::canOverride() and tested this.The patch also slightly refactors ConfigFactory::loadMultiple() so that the module override event is not unnecessarily fired.
Comment #129
alexpottd.o forgot the patches!
Comment #131
alexpottSo there is some funky expectations around deleted config objects remaining in the static cache in the config factory and there are tests for this in
ConfigCRUDTest
. Adding check for existence of cached object back toConfigFactory::get()
.Comment #132
alexpottI don't think we should be resetting the static cache when we go to override free mode. This will punish any page that presents none overridden config with having to load essential config twice.
Comment #133
alexpottOh and now we can get rid of the preg_match :)
Comment #136
alexpottInteresting the reseting of the static caching in the override switching functions was also responsible for the clearing the cache during any config save!
Comment #137
alexpottWhilst improving the comment in ConfigFactory::onConfigSave() I realised that we could just replace the data on the existing cache entries instead.
Comment #138
Gábor HojtsyI think this now looks as good as this can get. Based on several reviews and multiple leads on the patch, I think this should be good to get in :)
Comment #140
alexpott137: 2098119.137.patch queued for re-testing.
Comment #141
Gábor HojtsyRandom fail.
Comment #142
tstoecklerThanks @alexpott! The latest interdiffs all look great. This would have been above my paygrade to RTBC myself but I wholeheartedly endorse it. :-)
Comment #144
catchComment #145
alexpottSorry for the noise - not so random after all. We need to use
!==
to compare objects, see https://stackoverflow.com/questions/3834791/fatal-error-nesting-level-to.... Also, the getCacheKeys method needed improvement sosystem.theme
andsystem.theme.disabled
didn't conflict.Comment #146
Anonymous (not verified) CreditAttribution: Anonymous commentedoooookay then. i think this is ready to fly. assigning to catch.
Comment #147
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #148
catchWe did this for onConfigSave() already, any reason not to do it directly here too?
Apart from that I don't see anything that should hold this up at all - although didn't do a very in depth review yet. Would be good to get in sooner rather than later so we can go back to #1880766: Preload configuration objects.
Comment #149
alexpottRerolled due to #2042807: Convert search plugins to use a ConfigEntity and a PluginBag and a couple of minor clean ups.
@catch I guess you are referring to
The reason not to do it is because it opens a can of worms with regards to what you then allow code to do with the config object after the delete method has been called. I think this can be explored in a followup.
Comment #150
Gábor HojtsyI agree this is still RTBC :)
Comment #151
catchOK let's keep that as a @todo.
Committed/pushed to 8.x, thanks!
Comment #152
Gábor HojtsyWorking on change notice.
Comment #153
Gábor HojtsyHere is a change notification: https://drupal.org/node/2170437. Please review / correct / provide feedback. Also working on the docs updates, but that will not happen before tomorrow.
Comment #154
Gábor HojtsyAlso entirely rewritten the docs page at https://drupal.org/node/1928898 - reviews also welcome :) Diff at https://drupal.org/node/1928898/revisions/view/6731455/6826437 but its mostly a rewrite, so not very useful to look at the diff.
Noticed two things while doing the docs and the change notice that need a followup to clean up:
1. There is no method to get the override status, so you cannot remember that and go back to where the factory was, like the language pattern of remembering, setting it explicitly and going back. Also the places where disableOverrides()/enableOverrides() was applied, it should probably respect the call stack like the language changes and return the factory to the prior state using this getter. This can be sort of hacked by doing canOverride('dummy') (i.e whatever dummy config name that is not a language override name), but that is ugly :D
2. http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D... was left around, although never invoked anymore since there is no config.init event fired anymore at all (and the global overrides are taken direct in the get() in the factory).
If the change notice and docs looks good, we can move this to fixed AFAIS :)
Comment #155
Gábor HojtsySubmitted a followup for those two issues at #2172561: Config overrides may spill over to undesired places. Please help get that in :)
Comment #156
Gábor HojtsyComment #157
agentrickardI finally got a chance to test this and from my initial read it seems that other modules cannot use the default config storage system.
Can that possibly be right?
I can't call configFactory->get() from inside my override because it creates recursion, and $configFactory->storage is a protected object, so I can't seem to inject it into my loader class.
I may just be missing something. How do I read the contents of a config file?
Essentially, I need to do exactly what the language handler does.
But you can't do that outside the configFactory class, because $storage is protected.
What am I missing here?
Comment #158
Gábor Hojtsy@agentrickard: you can independently take the config storage service as a dependency of your code (eg. in your constructor), no? I don't think the config factory is supposed to offer that out. (But I'm not really an architect in CMI :)
Comment #159
agentrickardFor some reason I didn't find these yesterday.
Will test.
Comment #160
agentrickardInteresting. It seems the setOverride() isn't taking for my code because the Domain negotiation service hasn't loaded yet.
Where does this run in the call stack in relation to the KernelEvent?
Comment #161
agentrickardHere's some voodoo that could use insight. Not sure where to document and I don't understand.
To make this system work, I have two EventSubscribers. The first sets the active domain context of the request. The second the config override.
These values "work" for the two subscribers.
These values do not:
Comment #162
Anonymous (not verified) CreditAttribution: Anonymous commentedagentrickard - can you open a new issue for this?
Comment #163
agentrickardSure. #2175951: EventSubscriber order of operation
Comment #164
Sutharsan CreditAttribution: Sutharsan commentedRelated:
The current issue did not remove the 'config.init' event listener. I created #2177689: Removed deprecated event listeners.