Fix override-free context, move global config overrides back to an event listener [#1929136]

Problem

In #1763640: Introduce config context to make original config and different overrides accessible @pounard rightly points out that by implementing the global config overrides as a context rather than an event listener, once you enter a different context, the global config overrides are not used anymore. Eg. if you enter a user specific context, your global overrides are no more. Previously global config overrides were implemented as a config event listener, so they reacted on config events like other overrides. Keeping that behavior would have let global overrides coexist with language specific overrides. That was certainly a point we missed with the original patch. By implementing global overrides as a context, going into a different context also assumed to solve "no overrides applied", while that is not actually what is happening. The config events are still firing and language specific overrides will still apply nonetheless.

Proposal

Move global config overrides back to an event listener (as was before #1763640: Introduce config context to make original config and different overrides accessible), like the locale config event listener still is.
Separate concerns of contexts and overrides clearly by removing the direct context specific override capability. Nothing in core used it anyway. Now overrides react to contexts as appropriate and concerns are clearly separate.
Fix the override-free context to actually be override-free. This was previously implemented to stack on other contexts and while it avoided global overrides, it did not avoid locale overrides.
Add plenty more test coverage to the locale overrides co-exisiting with non-overriden values and the locale overrides co-existing with global overrides.

Comment	File	Size	Author
#29	27-29-interdiff.txt	1.68 KB	alexpott
#29	1929136.global-as-event-listener.29.patch	25.57 KB	alexpott
#29
#27	22-27-interdiff.txt	4.87 KB	alexpott
#27	1929136.global-as-event-listener.27.patch	25.57 KB	alexpott
#27
#22	global-as-event-listener-22.patch	23.58 KB	YesCT
#22
#22	interdiff-20-22.txt	6.24 KB	YesCT
#21	interdiff.txt	5.99 KB	Gábor Hojtsy
#20	global-as-event-listener-20.patch	23.5 KB	Gábor Hojtsy
#20
#12	global-as-event-listener-12.patch	19.2 KB	Gábor Hojtsy
#12
#12	interdiff.txt	1.44 KB	Gábor Hojtsy
#11	interdiff.txt	13.45 KB	Gábor Hojtsy
#11	global-as-event-listener-11.patch	19.33 KB	Gábor Hojtsy
#11
#6	global-as-event-listener.patch	6.47 KB	Gábor Hojtsy
#6

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 27 February 2013 at 15:26

Issue tags:

+sprint

And putting on D8Mi sprint for expediting a solution :)

Comment #2

pounard

French

CreditAttribution: pounard commented 27 February 2013 at 16:05

I tend to think this is not a specific problem due to global config override, but a generic problem covering many other use cases. The user context (which will translate stuff thanks to the locale listener) is actually a great example: if I stack a new context over a user context, I probably will loose language: that's where context properties should be stacked and not isolated.

Comment #3

pounard

French

CreditAttribution: pounard commented 27 February 2013 at 16:06

Forgot that I think this bug is heavily related if not a duplicate of #1929176: Possibly inherit context properties between levels of the context stack, even thought we still can implement this specific response for the global config listener and provide another one for others.

Comment #4

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 27 February 2013 at 16:13

@pounard: yeah, at least the global context should be moved back to a global config event based override like it was before #1763640: Introduce config context to make original config and different overrides accessible. It is true if we want to implement the override-less config as something other than a stacked context, then this is more related to #1929176: Possibly inherit context properties between levels of the context stack (I guess the override-less context would stop inheriting things, and when it is left, the inheritance would work again like before), if we keep the override-less context as a stacked context.

First the global override should be removed from being a context though :D

Comment #5

pounard

French

CreditAttribution: pounard commented 27 February 2013 at 16:26

Indeed, global overrides are an important thing that should never be lost!

Comment #6

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 27 February 2013 at 17:08

Status:

Active

» Needs review

File	Size
global-as-event-listener.patch	6.47 KB

All right, so first let's experiment with moving back to a global override event listener instead of a context. This should make the global overrides active for example even if you are in a user context (or some other contrib context). This totally hoses the override-free context, since that was previously built on the assumption that the overrides are just not set in the one and only ConfigOverrideSubscriber (that assumption was not true with the locale subscriber using a different method already). So this will hose the override free context at least for now.

I've also removed the OVERRIDE stuff from the general ConfigContext, so it can be purely an intention indicator or the listeners. The listeners do the override. (The OVERRIDE constant based data storage was only used in the global config override implementation anyway).

Comment #7

pounard

French

CreditAttribution: pounard commented 27 February 2013 at 17:39

Ok this removes the global config context, but we still need to have a stub context implementation (null implementation) at least to be the very first one in the stack, ensuring that we will never hit a null pointer error when giving that to config object instances if there is no other stacked context instance.

EDIT: We cannot leave the ConfigFactory without a context, at least not now because the very first Config instance will probably hit a null instead of its context and raise a PHP fatal error.

BTW: I think the config override listener is not a bad thing to keep but implemented otherwise: we could legally imagine that any context could arbitrary carry key/value pairs to replace in the config object? This is another topic, but I'll leave this comment live its life here to keep that idea somewhere. Note that this statement would also be problematic because any context don't know on which config object it applies.

Comment #8

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 27 February 2013 at 18:12

@pounard: I think the default context is already there, the config factory gets initialized with an empty ConfigContext. Since it does not have any context parameters set, it is essentially the simplest context possible now.

As for removing the generic override listener and the override data to be stored on the context, there is no core use case. Do you have a contrib use case? If not, I think its valuable to keep the cleaner separation between context and overrides.

(Hope you don't keep editing your comment further :)

Comment #9

27 February 2013 at 19:52

Status:

Needs review

» Needs work

The last submitted patch, global-as-event-listener.patch, failed testing.

Comment #10

pounard

French

CreditAttribution: pounard commented 27 February 2013 at 22:02

lol no I won't keep editing my comment further :) I missed the point where a default context were created in the factory, so that's OK if it does. I was thinking mainly for contrib and custom module, there is a lot of places where we could want to provide contextual overrides. One that jumps into mind right now is if you want to reproduce a context (as in the context module) / spaces (as in the space module) types overrides, this feature could be quite useful. But, due to the highly extendable/flexible model core has with listeners and such, it's not a requirement for core to provide such implementation.

Comment #11

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 February 2013 at 11:05

Status:

Needs work

» Needs review

File	Size
global-as-event-listener-11.patch	19.33 KB

interdiff.txt	13.45 KB

All right, here is an update. Plenty of stuff.

1. I've introduced an $allowOverrides property on ConfigContext that can be retrieved from the allowOverrides() method. This way contexts can set if they don't want to allow any overrides, and Config() reacts to this by not taking any of the overrides coming in via it's setOverrides() call. While this sounds like we are trapping overrides the last minute, reality is overrides can get in via different events. The prior global overrides were set for example as one array all at once, while locale overrides can only work on a config key basis since they are loaded from files. I did not make this property modifyable but instead inherited to a FreeConfigContext that is the new config.context.free. Now this actually makes overrides not apply. Unlike before, where it only made the overrides applied via the contexts go away and not overrides applied via event listeners.

2. I realized init() is not invoked anymore on ConfigContext because earlier it was invoked when some kind of data was set on config, and user context invoked it when the user was set, global config context invoked it on init, etc. However, only the user context has direct data on it now, the free context and the base context does not. So init()-ing it when the data is set is no option. So I made the config factory init the context when it is put on the stack. This made the user context needing to re-init itself when a new account is set. I think this makes sense since the API theoretically allows the same context to be reused with different account values, and re-init()-ing will give it a new uuid as well as give it a new fresh cache in Config. Also removed the user context uuid to be derived from the user id, since we init the uuid before we know the user and even use the fact in tests that the uuid will be unique even if we set the same user again. So better leave no impression that we want to tie the uuid to the uid.

3. Added extra test coverage for non-override values in the locale override test and added more comments to make it clear why is even the base English config is overriden.

4. Fixed the config override test. It was based on the assumption that changing global $conf would change the config values (by re-init()-in the global overrides all the time). Since global overrides are not a context anymore and re-initing an unrelated context to get global overrides re-read sounded awkward, I made use of the free context to cross-verify data where needed.

The tests that were failing before are passing nicely for me with this solution. While this keeps the free context as a stacked context and it does not inherit context values, it does clean up the relation between contexts and overrides and make global overrides work truly globally (even if in user context and/or if language overrides apply in some way anyway). There should be some more tests coverage added for that, but otherwise I think this is a good first step in the philosophy of the current system.

Comment #12

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 February 2013 at 11:07

File	Size
interdiff.txt	1.44 KB
global-as-event-listener-12.patch	19.2 KB

Wups left in some debug stuff in the tests that will fail.

Comment #13

pounard

French

CreditAttribution: pounard commented 28 February 2013 at 11:16

I'm not really OK with that approach: I'd better give the opportunity for the API user to fetch a "context-less raw config object" than stacking a "context free context": this is what I explained in the other issue: we could need both at the same time. For example, the raw config would be only used to set values into form fields, and the normal contextual config would give me my field titles.

The other reason why I think this is incredibly bad is necause overrides as used today are hardcoded settings, and they cannot be changed under any circumstances: the only place where we could see them as they exist in files instead of the override is in form field values; all other use cases for this are a security risk. Overrides are environment related/admin driven overrides, and they must not be bypassed under any circumstances.

Comment #14

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 February 2013 at 11:24

@pounard: problem is the two core use cases when you need to skip overrides (a) admin forms and (b) import involve multiple operations on config. It is not like you can take config('...', NO_OVERRIDE_PLEASE) and be done with that. That would make you need to invoke all config() that can be ever involved in an admin form build with this, and if its an API function that is used to do stuff, then it would need to take this as an argument. That sounds like would need large sweeping changes around all of the APIs that would retrieve config data at one point and could also be involved in building a form or importing config.

So even if it is not a stacked config, it needs to be from here on give me original config all the time or it needs to be passed around very diligently all around our APIs.

I understand you'd love to see all the problem you identified with config context fixed all at once, but I think we can solve the global overrides to work universally first. Override free is an already existing context, so it is not like I'm introducing anything new here, I just fixed it to actually be override free.

Comment #15

pounard

French

CreditAttribution: pounard commented 28 February 2013 at 12:41

I think that using a context free config should be a very rare operation, because it exposes potential security risks, it should be explicitely passed around everywhere it needs to be. But I understand that's a bigger job than this patch. Let's get this simple patch in!

But we should definitely fix this in another issue, it's too dangerous to allow this easily config to be context free. Let's say that someone needs a context free config for building a form, hooks are invoked during this form build, all hook implementors would inherit from a context free object: very wrong.

Comment #16

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 February 2013 at 13:02

Let's say that someone needs a context free config for building a form, hooks are invoked during this form build, all hook implementors would inherit from a context free object: very wrong.

Well, that is the intention, right? The hooks invoked from the original config changed should also work with the original data, that is why the hook got invoked at the first place. If it would still see the (unchanged) overrides, then what's the point in invoking it? I agree working with the original config is rare, that is why we don't make the free context the default.

Comment #17

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 February 2013 at 13:15

Issue tags:

+Needs tests

Patch passed, however still needs more tests to prove mixing global overrides with language overrides work and which one takes precedence. Should depend on the weight of the event listener. Possibilities! :)

Comment #18

pounard

French

CreditAttribution: pounard commented 28 February 2013 at 13:59

Well, that is the intention, right? The hooks invoked from the original config changed should also work with the original data, that is why the hook got invoked at the first place.

I do not agree, because context free (without environment specific overrides) is not a context, it is a security breach: a config object should never fall down into user/contrib code without at least the overrides.

EDIT: For all other use cases, I agree it's the point of stacking contextes. But we should never treat a config without global overrides as a context, it's not.

Comment #19

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 February 2013 at 15:13

So if it is not stacked on the context, it is a standalone thing that says (ignore the context), then then you should still be able to retrieve stuff where the context is taken into account. Whether that is implemented by putting an empty context on top of the stack or by ignoring the stack and then later not ignoring it, the resulting effect on the stack is the same. Anyway, let's keep the scope of this issue and then either discuss removing free as a context in #1929176: Possibly inherit context properties between levels of the context stack or somewhere else. I think this could be in scope in #1929176: Possibly inherit context properties between levels of the context stack, where contexts are suggested to merge values from prior contexts, which is where the free context needs to be revisited again, since that would not merge any values if implemented as a context still. So I think it is the place to continue discuss this aspect when we have this patch in :) I'm concentrating on solving the global overrides first.

Comment #20

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 February 2013 at 16:33

File	Size
global-as-event-listener-20.patch	23.5 KB

Ok here are those tests. This is as far as I'm concerned as complete as solution as **this** issue is concerned. We should continue the discussion in other issues about other things. This is scoped to deal with making global overrides co-exist with others and fix the override-free context to actually be override-free.

In this update:

1. I realised there is already a test case for verifying the override-free context, so no need for that that I added in the above patch.
2. Added test coverage for (a) the non-overriden value in the 404 key (at all places) (b) a global override for the 404 key under the same config file and tests to ensure it mixes well with the language override for separate keys (c) conflicting overrides to test that the language override prevails over the global override for the same key (expected).

Only tests are changed.

Comment #21

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 February 2013 at 16:51

File	Size
interdiff.txt	5.99 KB

Oh and an interdiff to make that easier to review. Hope this helps.

Comment #22

YesCT CreditAttribution: YesCT commented 28 February 2013 at 19:34

File	Size
interdiff-20-22.txt	6.24 KB
global-as-event-listener-22.patch	23.58 KB

some docs stuff and coding style.

will probably need re-rolled if the follow-up to clean up #1763640: Introduce config context to make original config and different overrides accessible gets in first.

[edit: added some details below]

Here are some details with links to the standards... just ... in case it helps someone. These are not all the changes, so check the interdiff.

+++ b/core/lib/Drupal/Core/Config/Context/ConfigContext.phpundefined
@@ -120,4 +104,11 @@ public function notify($config_event_name, Config $config = NULL) {
+  /**
+   * Implements Drupal\Core\Config\Context\ContextInterface::allowOverrides().
+   */

http://drupal.org/node/1354#classes

"If you use a namespace in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash)."

+++ b/core/lib/Drupal/Core/Config/Context/ContextInterface.phpundefined
@@ -85,4 +77,12 @@ public function getUuid();
+  /**
+   * Whether this context allows overrides.
+   *
+   * @return boolean
+   *   TRUE if overrides are allowed, FALSE otherwise.
+   */
+  public function allowOverrides();

* Whether this context allows overrides.

to make it a third person verb
http://drupal.org/node/1354#functions

* Returns whether this context allows overrides.

and
http://drupal.org/node/1354#types
bool instead of boolean

+++ b/core/lib/Drupal/Core/Config/Context/FreeConfigContext.phpundefined
@@ -0,0 +1,19 @@
+  protected $allowOverrides = FALSE;
+}

newline at end of class

+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigGlobalOverrideSubscriber.phpundefined
@@ -0,0 +1,40 @@
+   * Override configuration values with global $conf.

Overrides

Comment #23

sun

German

Karlsruhe

CreditAttribution: sun commented 28 February 2013 at 21:11

I disagree with the strong statement in #15.

The ultimate question is: Should hook_form_alter() implementations be aware of the context they are operating in?

My stance is No. There can be multiple active contexts, and hook implementations are just simply not able to "rebuild" or resemble the full stack of contexts. Because how on earth would they be able to gather that information?

Likewise, if the context is free, then that is just simply the context that hook implementations will operate in. The form is built for that particular scope and context, so it does not make sense to suddenly lose that information.

Comment #24

pounard

French

CreditAttribution: pounard commented 1 March 2013 at 06:50

The question is not do context should be passed to form_alter hooks? Because this answer is of course true, but it is Should a context free object with no global overrides be considered as a context? and here I'm saying a definite no. If we allow this, we allow settings.php hardcoded variable bypass, and that's a no go.

Comment #25

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 1 March 2013 at 08:17

@pounard / @sun: the override free-context is pre-existing this patch. So let's consider the current patch in it's scope, review and get in. Then we can discuss the context free problem and you all don't need to repeat yourself again on a separate issue :) I get that if we happen to remove the override-free context as a context, we'd remove the allowOverrides stuff added here, but that is minimal (and adding it was required to actually make override-free work). Let's get patch reviews!

@YesCT: thanks for the changes, looks good to me :)

Comment #26

pounard

French

CreditAttribution: pounard commented 1 March 2013 at 08:28

Status:

Needs review

» Reviewed & tested by the community

Yes, let's make this patch in.

The listener approach for global overrides sounds good and it seems to restore the original settings.php overrides feature quite well (even in context free contextes if I'm not mistaken I didn't see any exceptions).

The only note I would make about this patch is that is the 30 priority for ConfigGlobalOverrideSubscriber is enough? Are we sure this will be run *after* other listeners?

But this put aside RTBC as long as tests pass.

Comment #27

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 5 March 2013 at 10:31

Status:

Reviewed & tested by the community

» Needs review

File	Size
1929136.global-as-event-listener.27.patch	25.57 KB

22-27-interdiff.txt	4.87 KB

Okay I like what the patch does but I think we can get a more elegant solution by moving the override data from the configuration object to the config context. This means we can avoid this code:

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -275,8 +275,10 @@ protected function replaceData(array $data) {
+    if ($this->context->allowOverrides()) {
+      $this->overrides = NestedArray::mergeDeepArray(array($this->overrides, $data), TRUE);
+      $this->resetOverriddenData();
+    }

And put all the implementation details in the Drupal\Core\Config\Context\FreeConfigContext class.

I think this is architecturally cleaner too as overrides are as a result of your current config context. So this means in a followup patch we can explore not re-reading config unoverridden data when we switch contexts! Less queries! And we can make the config factory cache simpler as well.

RE @pounard's rtbc in #26 global overrides do not apply in the override free context... neither this patch nor Gabor's has changed this. It is the whole point of this context to not have them apply.

Comment #28

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 5 March 2013 at 10:43

Status:

Needs review

» Needs work

I'm fine with this update, however you added Drupal\... and not \Drupal\... at various places. That should be fixed.

Comment #29

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 5 March 2013 at 12:31

Status:

Needs work

» Needs review

File	Size
1929136.global-as-event-listener.29.patch	25.57 KB

27-29-interdiff.txt	1.68 KB

Whoops :) here you go!

Comment #30

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 5 March 2013 at 12:39

Status:

Needs review

» Reviewed & tested by the community

All right, so this just moves override maintenance into the context. Prior code maintained overrides in Config and cleared it up when entering a new context. So it works the same way either way. This is the type of thing where we can pick either one we like. I'm fine with this one too.

Comment #31

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 5 March 2013 at 15:07

Status:

Reviewed & tested by the community

» Fixed

Took me a bit to understand what this was doing, but Gábor walked me through last night, and in the meantime looks like it got even better. Thanks, Alex! :)

Committed and pushed to 8.x. Thanks!

Comment #32

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 5 March 2013 at 15:47

Updated our existing docs for this change. Minor changes were needed. http://drupal.org/node/1928898/revisions/view/2582328/2590502

Also tying this into our existing change notice at http://drupal.org/node/1928922

I think this is resolved fully.

@pounard I took your global override concerns to heart in #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects, and I'll try to help drive people there (and post it to the security team for example). I hope I properly represented your opinions. I mixed them up with the other viewpoint admittedly :)

Comment #33

pounard

French

CreditAttribution: pounard commented 12 March 2013 at 09:26

I was in holydays, so I'm a bit late on this: @alexpott yes, that's much much cleaner! It avoids tainting the Config objects, this approach is way better!

One small note thought: in order to continue this way the original Config object should probably never know about the context, but the context should implement the Config interface (this is a very bad design flaw that Config objects don't have an interface) and decorate it: the user should get a Context instance and fetch data using get() over it and not over the Config object. But this should be the topic for another issue.