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

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.

CommentFileSizeAuthor
#149 2098119.149.patch163.95 KBalexpott
#149 145-149-interdiff.txt4.57 KBalexpott
#145 2098119.145.patch163.36 KBalexpott
#145 137-145-interdiff.txt978 bytesalexpott
#137 2098119.137.patch163.31 KBalexpott
#137 136-137-interdiff.txt2.55 KBalexpott
#136 2098119.136.patch162.49 KBalexpott
#136 133-136-interdiff.txt2.36 KBalexpott
#133 2098119.133.patch161.8 KBalexpott
#133 132-133-interdiff.txt547 bytesalexpott
#132 2098119.132.patch161.75 KBalexpott
#132 131-132-interdiff.txt1.3 KBalexpott
#131 2098119.131.patch161.71 KBalexpott
#131 128-131-interdiff.txt3.02 KBalexpott
#129 2098119.128.patch161.3 KBalexpott
#129 125-128-interdiff.txt11.23 KBalexpott
#125 2098119.125.patch156.9 KBalexpott
#125 120-125-interdiff.txt3.52 KBalexpott
#120 2098119.120.patch154.18 KBalexpott
#120 116-120-interdiff.txt46.5 KBalexpott
#117 2098119-116.patch122.83 KBAnonymous (not verified)
#114 2098119-114.patch141.79 KBAnonymous (not verified)
#105 103-105-interdiff.txt698 bytesalexpott
#105 2098119.105.patch122.64 KBalexpott
#103 2098119.103.patch122.59 KBalexpott
#103 102-103-interdiff.txt20.54 KBalexpott
#102 2098119_102.patch120.35 KBchx
2098119_102.patch120.35 KBchx
#100 2098119_98.patch120.42 KBchx
#97 2098119_97.patch120.42 KBchx
#95 2098119-95.patch121.23 KBAnonymous (not verified)
#91 2098119-90.patch121.1 KBAnonymous (not verified)
#88 2098119-88.patch121.11 KBAnonymous (not verified)
#86 2098119-86.patch121.01 KBAnonymous (not verified)
#84 2098119_83.patch114.97 KBchx
#79 2098119-79.patch115.71 KBAnonymous (not verified)
#77 2098119-77.patch115.77 KBAnonymous (not verified)
#75 2098119-75.patch112.24 KBAnonymous (not verified)
#73 2098119-73.patch111.81 KBAnonymous (not verified)
#69 2098119-69.patch111.81 KBAnonymous (not verified)
#64 2098119-64.patch110.18 KBAnonymous (not verified)
#61 2098119-61-cmi-overrides.patch99.67 KBAnonymous (not verified)
#47 2098119-47.patch95.71 KBAnonymous (not verified)
#45 2098119-45.patch95.76 KBAnonymous (not verified)
#43 2098119-43.patch96.23 KBAnonymous (not verified)
#41 2098119-41.patch94.48 KBAnonymous (not verified)
#39 2098119-39.patch95.01 KBAnonymous (not verified)
#37 2098119-37.patch86.79 KBAnonymous (not verified)
#34 2098119-34.patch86.15 KBAnonymous (not verified)
#21 2098119-21.patch87.51 KBAnonymous (not verified)
#19 2098119-19.patch80.55 KBAnonymous (not verified)
#16 2098119-16.patch77.46 KBAnonymous (not verified)
#13 2098119-13.patch51.12 KBAnonymous (not verified)
#11 2098119-11.patch47.51 KBAnonymous (not verified)
#9 2098119-9.patch37.76 KBAnonymous (not verified)
#6 2098119-first-cut.patch38.28 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

here's the last patch i wrote to stop the config context system going in the first place.

https://drupal.org/files/1763640-83.patch

Anonymous’s picture

Issue tags: +Configuration system
Dave Reid’s picture

Title: Replace config context system with baked-in locale support and single-even based overrides » Replace config context system with baked-in locale support and single-event based overrides
Dave Reid’s picture

Hard to tell what is being proposed or how things will change here for an outsider. Definitely would be nice to run this by agentrickard.

Anonymous’s picture

i'll update the issue summary today, and hopefully provide a first-cut patch.

Anonymous’s picture

Status: Active » Needs review
FileSize
38.28 KB

here's a WIP patch that at least installs, and rm -rf's the ConfigContext code.

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

The last submitted patch, 2098119-first-cut.patch, failed testing.

Anonymous’s picture

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

#6: 2098119-first-cut.patch queued for re-testing.

Anonymous’s picture

FileSize
37.76 KB

here's another go without the durp durp debug.

Status: Needs review » Needs work

The last submitted patch, 2098119-9.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
47.51 KB

here's another go, that removes config_context_* calls.

Status: Needs review » Needs work

The last submitted patch, 2098119-11.patch, failed testing.

Anonymous’s picture

FileSize
51.12 KB

this one might get us closer to no fatals.

Anonymous’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2098119-13.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
77.46 KB

this should fix some of those fails.

Status: Needs review » Needs work

The last submitted patch, 2098119-16.patch, failed testing.

tstoeckler’s picture

I'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!

Anonymous’s picture

Status: Needs work » Needs review
FileSize
80.55 KB

this should fix the 'no private stream wrapper' fails. actually restores the global overrides.

Anonymous’s picture

Issue summary: View changes

Fixing typo

Status: Needs review » Needs work

The last submitted patch, 2098119-19.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
87.51 KB

add back locale overrides. fix config import tests.

Status: Needs review » Needs work

The last submitted patch, 2098119-21.patch, failed testing.

tstoeckler’s picture

To expand on #18 I do not see global overrides handled in this patch.

Anonymous’s picture

yep, 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.

catch’s picture

Priority: Major » Critical
tstoeckler’s picture

OK. Would still be great to have some reasoning for this.

catch’s picture

@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.

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Looked at language related stuff. There are obviously lots of dev code in this patch, which I did not call out :)

  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -50,25 +65,32 @@ class Config {
    -   * The storage used to load and save this configuration object.
    +   * The current module locale overrides.
        *
    -   * @var Drupal\Core\Config\StorageInterface
    +   * @var array
        */
    -  protected $storage;
    +  protected $localeOverrides;
    

    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.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -56,12 +63,15 @@ class ConfigFactory {
    +   * @param \Drupal\Core\Language\Language
    +   *   The language for this configuration.
    

    At other places language as the override criteria is nicely identified :) ++

  3. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -262,4 +242,44 @@ public function clearStaticCache() {
    +   * Get configuration name for this language.
    +   *
    +   * It will be the same name with a prefix depending on language code:
    +   * locale.config.LANGCODE.NAME
    +   *
    +   * @param string $name
    +   *   The name of the config object.
    +   *
    +   * @return string
    +   *   The localized config name.
    +   */
    +  public function getLocaleConfigName($name) {
    +    if (!isset($this->language)) {
    +      return FALSE;
    +    }
    +    return 'locale.config.' . $this->language->id . '.' . $name;
    

    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

agentrickard’s picture

Busy 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.

agentrickard’s picture

I 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.)

Anonymous’s picture

re #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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
86.15 KB

s/locale/language/g.

i've left the config name as 'locale.' ... to keep the patch smaller.

Status: Needs review » Needs work

The last submitted patch, 2098119-34.patch, failed testing.

Gábor Hojtsy’s picture

@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 :)

Anonymous’s picture

Status: Needs work » Needs review
FileSize
86.79 KB

here's another go, hopefully this one actually gets through to a test run.

Status: Needs review » Needs work

The last submitted patch, 2098119-37.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
95.01 KB

ok, 3 layers of the 4 layer layer cake in, all config tests pass locally.

Status: Needs review » Needs work

The last submitted patch, 2098119-39.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
94.48 KB

durp left in debug.

Status: Needs review » Needs work

The last submitted patch, 2098119-41.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
96.23 KB

this should have a few less fails.

Status: Needs review » Needs work

The last submitted patch, 2098119-43.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
95.76 KB

back out the ConfigFactory event changes. ConfigFactory should implement an identityMap for config objects.

Status: Needs review » Needs work

The last submitted patch, 2098119-45.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
95.71 KB

restore the hack to not return 'deleted' objects from the static cache. FML.

Status: Needs review » Needs work

The last submitted patch, 2098119-47.patch, failed testing.

Jose Reyero’s picture

Replying mostly to the issue summary:

The configuration system underlies most parts of Drupal, and should be as simple and fast as possible.

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.

We currently fire off two events per config object during a read

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.

we decided to replace the config context system with baked-in locale support

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.

Gábor Hojtsy’s picture

@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.

Anonymous’s picture

ok, i'll stop on this and wait for the discussion to be had again.

Gábor Hojtsy’s picture

Ok, 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 :/

catch’s picture

My 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.

Gábor Hojtsy’s picture

@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.

tstoeckler’s picture

Can 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.

chx’s picture

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.

This issue got stalled on not clarifying this, now that we have that please continue.

chx’s picture

Issue summary: View changes

I want a pony. Edit: this comment was not written by chx.

tstoeckler’s picture

Yes, 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.

catch’s picture

Also 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.

catch’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: +D8MI, +language-config, +Configuration context

I 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

Anonymous’s picture

re. 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.

Anonymous’s picture

here's a reroll. i've only tested that it installs.

Anonymous’s picture

Status: Needs work » Needs review

durp durp oh hai bot.

Status: Needs review » Needs work

The last submitted patch, 61: 2098119-61-cmi-overrides.patch, failed testing.

Anonymous’s picture

FileSize
110.18 KB

mkay, 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

Anonymous’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 64: 2098119-64.patch, failed testing.

Grayside’s picture

Overriding 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?

Anonymous’s picture

there 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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
111.81 KB

ok, new patch, adds ConfigFactory->disableOverrides() and ConfigFactory->enableOverrides() plus procedural wrappers, and uses that where we used to use the null config context code.

Status: Needs review » Needs work

The last submitted patch, 69: 2098119-69.patch, failed testing.

Grayside’s picture

@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.

Anonymous’s picture

@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?

Anonymous’s picture

Status: Needs work » Needs review
FileSize
111.81 KB

who knows, the tests may even run with this patch.

Status: Needs review » Needs work

The last submitted patch, 73: 2098119-73.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
112.24 KB

ok. let's just allow a null language to config_factory_set_language().

Status: Needs review » Needs work

The last submitted patch, 75: 2098119-75.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
115.77 KB

new patch adds a test module to exercise the 'config.module.overrides' event, should fix some fails.

Status: Needs review » Needs work

The last submitted patch, 77: 2098119-77.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
115.71 KB

now with 100% less ffs() durp.

Status: Needs review » Needs work

The last submitted patch, 79: 2098119-79.patch, failed testing.

Gábor Hojtsy’s picture

Content 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...

alexpott’s picture

Issue tags: +beta blocker

As this patch will make significant changes to the config API this is beta blocking.

chx’s picture

Assigned: Unassigned » chx
chx’s picture

Status: Needs work » Needs review
FileSize
114.97 KB

Down to 55 fail(s), and 7 exception(s). We shall overcome.

Sneak peek of a 14 fails, 4 exceptions version.

Status: Needs review » Needs work

The last submitted patch, 84: 2098119_83.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
121.01 KB

ok, this might be all greeen.

Status: Needs review » Needs work

The last submitted patch, 86: 2098119-86.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
121.11 KB

dammit.

diff --git a/core/modules/system/lib/Drupal/system/Form/CronForm.php b/core/modules/system/lib/Drupal/system/Form/CronForm.php
index f9158e2..6a070e0 100644
--- a/core/modules/system/lib/Drupal/system/Form/CronForm.php
+++ b/core/modules/system/lib/Drupal/system/Form/CronForm.php
@@ -30,10 +30,10 @@ class CronForm extends ConfigFormBase {
    *
    * @param \Drupal\Core\Config\ConfigFactory $config_factory
    *   The factory for configuration objects.
-   * @param \Drupal\Core\Config\Context\ContextInterface $context
-   *   The configuration context used for this configuration object.
+   * @param \Drupal\Core\KeyValueStore\StateInterface $state
+   *   The state keyvalue collection to use.
    */
-  public function __construct(ConfigFactory $config_factory, KeyValueStoreInterface $state) {
+  public function __construct(ConfigFactory $config_factory, StateInterface $state) {
     parent::__construct($config_factory);
     $this->state = $state;
   }
diff --git a/core/modules/system/lib/Drupal/system/Form/SiteMaintenanceModeForm.php b/core/modules/system/lib/Drupal/system/Form/SiteMa
index 4f4972d..535ecfd 100644
--- a/core/modules/system/lib/Drupal/system/Form/SiteMaintenanceModeForm.php
+++ b/core/modules/system/lib/Drupal/system/Form/SiteMaintenanceModeForm.php
@@ -32,7 +32,7 @@ class SiteMaintenanceModeForm extends ConfigFormBase {
    * @param \Drupal\Core\KeyValueStore\StateInterface $state
    *   The state keyvalue collection to use.
    */
-  public function __construct(ConfigFactory $config_factory, KeyValueStoreInterface $state) {
+  public function __construct(ConfigFactory $config_factory, StateInterface $state) {
     parent::__construct($config_factory);
     $this->state = $state;
   }

Status: Needs review » Needs work

The last submitted patch, 88: 2098119-88.patch, failed testing.

jhodgdon’s picture

You're not planning to commit a patch with

+++ b/core/includes/config.inc
@@ -1,13 +1,23 @@
 <?php
 
+/*
+Recoverable fatal error: Argument 1 passed to config_factory_set_language() 
+must be an instance of Drupal\Core\Language\Language, null given, called in
+ /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Datetime/Date.php
+ on line 146 and defined in config_factory_set_language() (line 42 
+of /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/config.inc).].
+*/

in it, right? :)

Anonymous’s picture

Status: Needs work » Needs review
FileSize
121.1 KB

ok, this one came back green on the patch test issue.

chx’s picture

Assigned: chx » Unassigned

Nice job, Justin!

andypost’s picture

#90 still needs to be cleared.
Is this really needed?

Anonymous’s picture

yep, #90 is right, i'll remove that in the next roll of the patch. more review welcome!

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch, 95: 2098119-95.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
120.42 KB

Status: Needs review » Needs work

The last submitted patch, 97: 2098119_97.patch, failed testing.

The last submitted patch, 97: 2098119_97.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
120.42 KB

Status: Needs review » Needs work

The last submitted patch, 100: 2098119_98.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
120.35 KB
alexpott’s picture

FileSize
20.54 KB
122.59 KB
  1. +++ b/core/includes/config.inc
    @@ -15,6 +17,35 @@
    +function config_factory_disable_overrides() {
    +  Drupal::service('config.factory')->disableOverrides();
    +}
    +
    +/**
    + * Enable overrides on the configuration factory.
    + */
    +function config_factory_enable_overrides() {
    +  Drupal::service('config.factory')->enableOverrides();
    +}
    

    Let's not introduce more helper functions to get rid of later - let's just call Drupal::configFactory()->function() since we're adding Drupal::configFactory() in this patch! I think we should also remove config_factory_set_language() that is also added by this patch.

  2. +++ b/core/includes/config.inc
    @@ -145,51 +174,23 @@ function config($name) {
    +function config_get_module_config_entities($module) {
    +  // While this is a lot of work to generate, it's not worth static caching
    +  // since this function is only called at install/uninstall, and only
    ...
    +  $info = entity_get_info();
    +  return array_filter($info, function($entity_info) use ($module) {
    +    return ($entity_info['module'] == $module) && is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityInterface');
    +  });
     }
    ...
    +  // once per module.
    

    No longer needed - has been removed.

  3. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -85,18 +114,38 @@ public function __construct(StorageInterface $storage, ContextInterface $context
       public function get($name) {
    -    $context = $this->getContext();
    -    $cache_key = $this->getCacheKey($name, $context);
    -    if (isset($this->cache[$cache_key])) {
    +    $names = array($name);
    +    if ($config = $this->loadMultiple($names)) {
    +      return $config[$name];
    +    }
    +    else {
    +      $cache_key = $this->getCacheKey($name);
    +      if (!isset($this->cache[$cache_key])) {
    +        $this->cache[$cache_key] = new Config($name, $this->storage, $this->eventDispatcher, $this->typedConfigManager, $this->language);
    +      }
           return $this->cache[$cache_key];
         }
    -
    -    $this->cache[$cache_key] = new Config($name, $this->storage, $context, $this->typedConfigManager);
    -    return $this->cache[$cache_key]->init();
       }
    

    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.

  4. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -85,18 +114,38 @@ public function __construct(StorageInterface $storage, ContextInterface $context
    +  public function getLanguageOverrides($name) {
    +    $language_name = $this->getLanguageConfigName($name);
    +    if ($language_name && $this->storage->exists($language_name)) {
    +      return $this->storage->read($language_name);
    +    }
    +    return FALSE;
    +  }
    

    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.

  5. +++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
    @@ -8,6 +8,8 @@
    +use Drupal\Core\Config\ConfigFactory;
    +use Drupal\Core\Config\NullStorage;
    

    Unnecessary.

  6. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigLocaleOverrideWebTest.php
    @@ -54,16 +54,6 @@ function testSiteNameTranslation() {
    -    // During path resolution the system.site configuration object is used to
    -    // determine the front page. This occurs before language negotiation causing
    -    // the configuration factory to cache an object without the correct
    -    // overrides. We are testing that the configuration factory is
    -    // re-initialised after language negotiation. Ensure that it applies when
    -    // we access the XX front page.
    -    // @see \Drupal\Core\PathProcessor::processInbound()
    -    $this->drupalGet('xx');
    -    $this->assertText('XX site name');
    

    Removing this negates the point of the test.

  7. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigLocaleOverride.php
    @@ -42,256 +43,30 @@ public function setUp() {
       function testConfigLocaleOverride() {
         $name = 'config_test.system';
    -    // The default language is en so the config key should be localised.
         $config = \Drupal::config($name);
         $this->assertIdentical($config->get('foo'), 'en bar');
    -    $this->assertIdentical($config->get('404'), 'herp');
    -
    -    // Ensure that we get the expected value when we avoid overrides.
    -    config_context_enter('config.context.free');
    -    $config_admin = \Drupal::config($name);
    -    $this->assertIdentical($config_admin->get('foo'), 'bar');
    -    $this->assertIdentical($config_admin->get('404'), 'herp');
    -
    -    // Leave the non override context.
    -    config_context_leave();
    -    $config = \Drupal::config($name);
    -    $this->assertIdentical($config->get('foo'), 'en bar');
    -    $this->assertIdentical($config->get('404'), 'herp');
    -  }
    

    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.

Status: Needs review » Needs work

The last submitted patch, 103: 2098119.103.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
122.64 KB
698 bytes

Fixing exceptions

The last submitted patch, 105: 2098119.105.patch, failed testing.

alexpott’s picture

105: 2098119.105.patch queued for re-testing.

The last submitted patch, 105: 2098119.105.patch, failed testing.

Anonymous’s picture

thanks 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.

alexpott’s picture

105: 2098119.105.patch queued for re-testing.

Gábor Hojtsy’s picture

I 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 :)

  1. +++ b/core/lib/Drupal.php
    @@ -242,6 +242,17 @@ public static function config($name) {
    +   * Retrieves the configuration factory.
    +   *
    +   * This is mostly used to change the language configuration objects are in.
    +   *
    +   * @return \Drupal\Core\Config\ConfigFactory
    +   */
    +  public static function configFactory() {
    

    Better docs would be to explain that this is for easier global Drupal::configFactory()->enableOverrides() and disableOverrides(). Not just for languages :)

  2. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -33,6 +34,20 @@ class Config {
    +   * The Language object used to override configuration data.
    
    @@ -103,28 +125,19 @@ class Config {
    +   *   The Language object used to override configuration data.
    
    +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -39,11 +35,25 @@ class ConfigFactory {
    +   * The Language object used to override configuration data.
    

    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.

  3. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -54,25 +69,32 @@ class Config {
    +   * The current runtime data ($data + $languageOverrides + $moduleOverrides + $overrides).
    ...
    -   * The current runtime data ($data + $overrides from Config Context).
    

    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.

  4. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -294,7 +308,7 @@ protected function replaceData(array $data) {
    -   * Sets overridden data for this configuration object.
    +   * Sets settings.php overrides this configuration object.
    
    @@ -304,8 +318,38 @@ protected function replaceData(array $data) {
    +   * Sets module overrides this configuration object.
    ...
    +   * Sets language overrides this configuration object.
    

    The "for" got lost resulting in a broken English sentence.

  5. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -304,8 +318,38 @@ protected function replaceData(array $data) {
    +   * @param array $data
    +   *   The overridden values of the configuration data.
    ...
    +   * @param array $data
    +   *   The overridden values of the configuration data.
    

    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.

  6. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -313,16 +357,24 @@ public function setOverride(array $data) {
    +   * Configuration overrides operate at three distinct layers: locale, modules
    +   * and settings.php, with the last of these winning. Overrides in
    ...
    +   * modules win over locale.
    ...
    +   * settings.php win over values provided by modules. Overrides provided by
    
    @@ -604,4 +655,29 @@ protected function castValue($key, $value) {
    +  /**
    +   * Get configuration name for this language.
    +   *
    +   * It will be the same name with a prefix depending on language code:
    +   * language.config.LANGCODE.NAME
    +   *
    +   * @return string
    +   *   The localized config name.
    +   */
    +  public function getLanguageConfigName() {
    +    if (!isset($this->language)) {
    +      throw new ConfigException("No language set, cannot return language config name for '{$this->name}'");
    +    }
    +    return 'locale.config.' . $this->language->id . '.' . $this->getName();
    +  }
    

    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).

  7. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -64,19 +74,38 @@ class ConfigFactory {
    +   *   The language for this configuration.
    ...
    +   * @param \Drupal\Core\Language\Language
    ...
        */
    -  public function __construct(StorageInterface $storage, ContextInterface $context, TypedConfigManager $typed_config) {
    +  public function __construct(StorageInterface $storage, EventDispatcher $event_dispatcher, TypedConfigManager $typed_config, Language $language = NULL) {
    

    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 :)

  8. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -85,18 +114,21 @@ public function __construct(StorageInterface $storage, ContextInterface $context
    +   * Returns a list of configuration objects for a given names.
    

    "a given names"?

  9. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -174,103 +253,126 @@ public function reset($name = NULL) {
    +   *   An array of cache keys that match the provided config name.
    

    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.

  10. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -174,103 +253,126 @@ public function reset($name = NULL) {
    +   * @param Language $language
    ...
    +   * @return ConfigFactory
    

    Should fully qualify these class names in docs.

  11. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -174,103 +253,126 @@ public function reset($name = NULL) {
    +  public function setLanguage(Language $language = NULL) {
    +    $this->language = $language;
    +    return $this;
       }
    

    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?

  12. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -174,103 +253,126 @@ public function reset($name = NULL) {
    +   * It will be the same name with a prefix depending on language code:
    +   * language.config.LANGCODE.NAME
    ...
    +        if (strpos($name, 'locale.config.') === 0) {
    ...
    +        $language_names[$name] = 'locale.config.' . $this->language->id . '.' . $name;
    ...
    +   * It will be the same name with a prefix depending on language code:
    +   * language.config.LANGCODE.NAME
    ...
    +    if (!isset($this->language) || strpos($name, 'locale.config.') === 0) {
    +      return FALSE;
    +    }
    +    return 'locale.config.' . $this->language->id . '.' . $name;
    

    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 :)

Gábor Hojtsy’s picture

Remaining part of my review. Please read the whole thing before starting to respond, I got some revelations midway :)

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    index 0000000..fd70cc9
    --- /dev/null
    
    --- /dev/null
    +++ b/core/lib/Drupal/Core/Config/ConfigModuleOverridesEvent.php
    
    +++ b/core/lib/Drupal/Core/Config/ConfigModuleOverridesEvent.php
    +++ b/core/lib/Drupal/Core/Config/ConfigModuleOverridesEvent.php
    @@ -0,0 +1,76 @@
    
    @@ -0,0 +1,76 @@
    +<?php
    +
    +namespace Drupal\Core\Config;
    +
    +use Symfony\Component\EventDispatcher\Event;
    +use Drupal\Core\Language\Language;
    +
    +class ConfigModuleOverridesEvent extends Event {
    

    Lacks file level and class level phpdoc.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigModuleOverridesEvent.php
    @@ -0,0 +1,76 @@
    +   * The Language object used to override configuration data.
    

    Same as in previous comment.

  3. +++ b/core/lib/Drupal/Core/Config/ConfigModuleOverridesEvent.php
    @@ -0,0 +1,76 @@
    +  /**
    +   * Configuration overrides.
    +   *
    +   * @var array
    +   */
    +  protected $overrides;
    

    Would be useful to document that this is keyed by config name IMHO. Eg. only one override value possible per name :)

  4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
    @@ -364,7 +364,7 @@ public function save(EntityInterface $entity) {
    -      $this->configFactory->rename($prefix . $id, $prefix . $entity->id());
    +      $config = $this->configFactory->rename($prefix . $id, $prefix . $entity->id());
    

    Why is this in the patch?! Seems unrelated?

  5. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
    @@ -78,7 +73,6 @@ protected function config($name) {
    -      $this->configFactory->enterContext($container->get('config.context.free'));
    

    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.

  6. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImporterTest.php
    @@ -210,6 +211,7 @@ function testUpdated() {
    +    \Drupal::service('config.factory')->reset($name);
    
    +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallTest.php
    @@ -47,6 +47,8 @@ function testModuleInstallation() {
    +    \Drupal::service('config.factory')->reset($default_config);
    +    \Drupal::service('config.factory')->reset($default_configuration_entity);
    
    +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallWebTest.php
    @@ -49,6 +49,8 @@ function testIntegrationModuleReinstallation() {
    +    \Drupal::service('config.factory')->reset($default_config);
    +    \Drupal::service('config.factory')->reset($default_configuration_entity);
    
    @@ -82,6 +84,8 @@ function testIntegrationModuleReinstallation() {
    +    \Drupal::service('config.factory')->reset($default_config);
    
    +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigLocaleOverride.php
    @@ -35,263 +35,32 @@ public function setUp() {
    +    \Drupal::service('config.factory')->setLanguage(language_default());
    ...
    +    \Drupal::service('config.factory')->setLanguage(language_load('fr'));
    ...
    +    \Drupal::service('config.factory')->setLanguage(language_load('de'));
    
    +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.php
    @@ -79,6 +79,7 @@ function testSnapshot() {
    +    \Drupal::service('config.factory')->reset($config_name);
    

    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?!

  7. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigLocaleOverride.php
    @@ -35,263 +35,32 @@ public function setUp() {
    -  /**
    -   * Tests locale override in combination with global overrides.
    -   */
    -  function testConfigLocaleUserAndGlobalOverride() {
    -    global $conf;
    -
    -    // Globally override value for the keys in config_test.system. Although we
    -    // override the foo key, there are also language overrides, which trump
    -    // global overrides so the 'foo' key override will never surface.
    -    $conf['config_test.system']['foo'] = 'global bar';
    -    $conf['config_test.system']['404'] = 'global herp';
    -
    

    Where is the replacement test which ensures the priorities of module / language, etc. overrides work as expected?

  8. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigModuleOverridesTest.php
    @@ -0,0 +1,47 @@
    + * Tests importing configuration from files into active store.
    + */
    +class ConfigModuleOverridesTest extends WebTestBase {
    

    Copy paste incorrect :)

  9. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOverrideTest.php
    @@ -61,14 +61,6 @@ function testConfOverride() {
    -    // Enter an override-free context to ensure the original data remains.
    -    config_context_enter('config.context.free');
    -    $config = \Drupal::config('config_test.system');
    -    $this->assertIdentical($config->get('foo'), $expected_original_data['foo']);
    -    $this->assertIdentical($config->get('baz'), $expected_original_data['baz']);
    -    $this->assertIdentical($config->get('404'), $expected_original_data['404']);
    -    config_context_leave();
    
    @@ -100,14 +92,6 @@ function testConfOverride() {
    -    // Enter an override-free context to ensure the original data remains saved.
    -    config_context_enter('config.context.free');
    -    $config = \Drupal::config('config_test.system');
    -    $this->assertIdentical($config->get('foo'), $expected_original_data['foo']);
    -    $this->assertIdentical($config->get('baz'), $expected_original_data['baz']);
    -    $this->assertIdentical($config->get('404'), $expected_original_data['404']);
    -    config_context_leave();
    

    Why remove these? Where is this already tested?

  10. +++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/EventSubscriber/ConfigModuleOverrideSubscriber.php
    @@ -0,0 +1,42 @@
    + * Contains \Drupal\config_test\ConfigTestController.
    ...
    + * Tests module overrides for configuration.
    ...
    +class ConfigModuleOverrideSubscriber implements EventSubscriberInterface {
    ...
    + */
    

    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.

  11. +++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php
    @@ -34,80 +30,29 @@ class LocaleConfigSubscriber implements EventSubscriberInterface {
    +    // @todo: this is probably wrong, help me Gabor, you're my only hope.
    +    if ($this->languageManager->isMultiLingual()) {
    +      $this->configFactory->setLanguage($this->languageManager->getLanguage(Language::TYPE_INTERFACE));
    +    }
    

    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 :/

  12. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathConfigEntityConverter.php
    @@ -29,17 +31,33 @@
    +   * Constructs a new EntityConverter.
    +   *
    +   * @param \Drupal\Core\Entity\EntityManagerInterface $entityManager
    +   *   The entity manager.
    +   */
    +  public function __construct(EntityManagerInterface $entity_manager, ConfigFactory $config_factory) {
    

    Copy-paste first phpdoc line :) Not using the right class name.

    Also, missing argument docs.

Anonymous’s picture

Status: Needs review » Needs work

thanks for the review Gabor! this is definitely needs work then :-)

Anonymous’s picture

FileSize
141.79 KB

ok, this patch addresses the functional parts of Gabor's review.

still need more tests, just want to see where this patch is at.

Anonymous’s picture

Status: Needs work » Needs review

durp durp durpal.

Status: Needs review » Needs work

The last submitted patch, 114: 2098119-114.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
122.83 KB

hmm, NFI what i did wrong with that last patch.

Status: Needs review » Needs work

The last submitted patch, 117: 2098119-116.patch, failed testing.

The last submitted patch, 117: 2098119-116.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
46.5 KB
154.18 KB

The attached patched:

  • Renames all the locale.config. to language.config. (this was the reason for many of the test failures in #116)
  • Moves the request event listener to the language module from the locale module for the same reasons as changing the file name
  • Allows multiple event listeners (and therefore modules) to override the same config object but different keys and tests this functionality in ConfigModuleOverridesTest
  • Adds tests for priorities of configuration overrides (settings.php > module > language) ConfigOverridesPriorityTest
  • Refactors the test module override event listener to have its own test module config_override
  • Adds a constant to the config factory for the language configuration file prefix and makes it the canonical and only source
  • Removes all the duplicate functions to create a language configuration object name from a config name and replaces them with a canonical method on the ConfigFactory
  • Makes ConfigFormBase work the same way that 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.

Status: Needs review » Needs work

The last submitted patch, 120: 2098119.120.patch, failed testing.

Anonymous’s picture

114: 2098119-114.patch queued for re-testing.

Anonymous’s picture

submitted for restest in because the bot segfaulted.

The last submitted patch, 114: 2098119-114.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.52 KB
156.9 KB

Fixed up test failures

tstoeckler’s picture

Wow, this is really impressive work. Kudos to everyone involved!
Found a couple things while reading through this.

  1. Super minor:
    - * @see \Drupal\Core\Config\StorageInterface
    ...
    + * @see Drupal\Core\Config\StorageInterface
    

    The starting slash should be retained.

  2. Minor:
    +   *   (optional) The language for this configuration. If a language is set then
    +   *   the config factory will use it to override configuration data is
    +   *   overrides are available.
    

    I think there should be a comman after "set", and the last "is" should be an "if".

  3. Super minor:
    +    $names = array($name);
    +    if ($config = $this->loadMultiple($names)) {
    

    Can be collapsed into one line.

  4. +    if ($config = $this->loadMultiple($names)) {
    +      return $config[$name];
    +    }
    +    else {
    +      $cache_key = $this->getCacheKey($name);
    +      if (!isset($this->cache[$cache_key])) {
    +        $this->cache[$cache_key] = new Config($name, $this->storage, $this->eventDispatcher, $this->typedConfigManager, $this->language);
    +      }
    

    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.

  5. Minor:
    +        $moduleOverrides = $this->loadModuleOverrides($names);
    ...
    +        $moduleOverrides = array();
    

    This should be $module_overrides.

  6. +          // Language override configuration is not regular configuration and
    +          // therefore not cached or overridden.
    

    I think this comment could be clearer. If I'm not mistaken what is meant is the following:

    Language override configuration cannot be overriden itself and is not cached indepently but as part of the configuration that it overrides.

    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.

Gábor Hojtsy’s picture

@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 :)

alexpott’s picture

So 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 both ConfigFactory::get() and ConfigFactory::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 in ConfigFactory::loadMultiple().

Secondly - in one of the comments we write

          // Language override configuration is used to override other
          // configuration. Therefore, when it has been added to the
          // $storage_data it is not statically cached in the config factory or
          // overridden in any way.

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 something Drupal::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.

alexpott’s picture

FileSize
11.23 KB
161.3 KB

d.o forgot the patches!

Status: Needs review » Needs work

The last submitted patch, 129: 2098119.128.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
161.71 KB

So 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 to ConfigFactory::get().

alexpott’s picture

FileSize
1.3 KB
161.75 KB

I 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.

alexpott’s picture

FileSize
547 bytes
161.8 KB

Oh and now we can get rid of the preg_match :)

The last submitted patch, 132: 2098119.132.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 133: 2098119.133.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.36 KB
162.49 KB

Interesting the reseting of the static caching in the override switching functions was also responsible for the clearing the cache during any config save!

alexpott’s picture

FileSize
2.55 KB
163.31 KB

Whilst improving the comment in ConfigFactory::onConfigSave() I realised that we could just replace the data on the existing cache entries instead.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

I 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 :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 137: 2098119.137.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

137: 2098119.137.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Random fail.

tstoeckler’s picture

Thanks @alexpott! The latest interdiffs all look great. This would have been above my paygrade to RTBC myself but I wholeheartedly endorse it. :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 137: 2098119.137.patch, failed testing.

catch’s picture

Fatal error: Nesting level too deep - recursive dependency? in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Config/ConfigFactory.php on line 451
FATAL Drupal\system\Tests\Menu\MenuRouterTest: test runner returned a non-zero error code (255).
alexpott’s picture

Status: Needs work » Needs review
FileSize
978 bytes
163.36 KB

Sorry 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 so system.theme and system.theme.disabled didn't conflict.

Anonymous’s picture

Assigned: Unassigned » catch

oooookay then. i think this is ready to fly. assigning to catch.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -85,18 +118,51 @@ public function __construct(StorageInterface $storage, ContextInterface $context
+      if (!isset($this->cache[$cache_key])) {

We 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.

alexpott’s picture

FileSize
4.57 KB
163.95 KB

Rerolled 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

      // If the config object has been deleted it will already exist in the
      // cache but self::loadMultiple does not return such objects.
      // @todo Explore making ConfigFactory a listener to the config.delete
      //   event to reset the static cache when this occurs.
      if (!isset($this->cache[$cache_key])) {

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.

Gábor Hojtsy’s picture

I agree this is still RTBC :)

catch’s picture

Title: Replace config context system with baked-in locale support and single-event based overrides » Change notice: Replace config context system with baked-in locale support and single-event based overrides
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

OK let's keep that as a @todo.

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Assigned: catch » Gábor Hojtsy

Working on change notice.

Gábor Hojtsy’s picture

Here 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.

Gábor Hojtsy’s picture

Also 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 :)

Gábor Hojtsy’s picture

Submitted a followup for those two issues at #2172561: Config overrides may spill over to undesired places. Please help get that in :)

Gábor Hojtsy’s picture

Title: Change notice: Replace config context system with baked-in locale support and single-event based overrides » Replace config context system with baked-in locale support and single-event based overrides
Priority: Major » Critical
Status: Active » Fixed
Issue tags: -Needs change record

[4:06pm] alexpott: GaborHojtsy: looks good to me
[4:06pm] alexpott: GaborHojtsy: I've not gone through with a fine tooth comb but nothing leapt out

agentrickard’s picture

I 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.

          // Get and apply any language overrides.
          if ($this->language) {
            $language_overrides = $this->storage->read($this->getLanguageConfigName($this->language->id, $name));
          }

But you can't do that outside the configFactory class, because $storage is protected.

What am I missing here?

Gábor Hojtsy’s picture

@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 :)

agentrickard’s picture

For some reason I didn't find these yesterday.

  config.cachedstorage.storage:
    class: Drupal\Core\Config\FileStorage
    factory_class: Drupal\Core\Config\FileStorageFactory
    factory_method: getActive
  config.storage:
    class: Drupal\Core\Config\CachedStorage
    arguments: ['@config.cachedstorage.storage', '@cache.config']
    tags:
      - { name: persist }

Will test.

agentrickard’s picture

Interesting. 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?

agentrickard’s picture

Here'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.

// DomainSubscriber
    $events[KernelEvents::REQUEST][] = array('onKernelRequestDomain', 300);
// DomainConfigSubscriber
    $events['config.module.overrides'][] = array('onDomainModuleOverride', 400);

These values do not:

// DomainSubscriber
    $events[KernelEvents::REQUEST][] = array('onKernelRequestDomain', 100);
// DomainConfigSubscriber
    $events['config.module.overrides'][] = array('onDomainModuleOverride', 400);
Anonymous’s picture

agentrickard - can you open a new issue for this?

agentrickard’s picture

Sutharsan’s picture

Related:
The current issue did not remove the 'config.init' event listener. I created #2177689: Removed deprecated event listeners.

Status: Fixed » Closed (fixed)

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