Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor

I still need to review this in full, but since you mentioned it:

Then overrides are merged only once

This might actually be a regression from HEAD then? The global $conf overrides are supposed to work dynamically at runtime. A contributed/custom module may have a good reason for temporarily overriding a config value, so as to be able to intentionally adjust/manipulate an otherwise hard-coded/static behavior elsewhere.

Right now, we're not caching the config objects yet. So with current HEAD, this regression would not be visible. However, @alexpott started to work on an intelligent LRU cache for config objects, which essentially caches a limited stack of, say, 20 repetitively used objects. As soon as that caching is implemented, we're going to return cached objects to the caller. Which in turn would mean that any adjustments to global $conf would not be picked up?

I believe you make sense. My intention was to point out that this is a significant change in behavior, which everyone should be aware of.

There is no LRU cache issue specific for the config system yet. It is developed in an abstracted way first, because the LRU logic makes sense for many kind of caches, not only config. Anyway, yes, separate issue. :)

I like this overall change proposal.

But the suggested changes contain too much magic. And too much automatic building and rebuilding logic.

So I heavily simplified and streamlined that. :)

Also, the $conf override functionality was barely tested, so I've rewritten the entire test case in order to cover all possible situations and expectations for them.

If you don't mind, I'd like to drive this home for you :)

15b6de7 Removed excessive (re)builds, cleaned up logic and naming, added proper tests.

Actually, there's one additional test I wanted to add — verifying that default config of a module (being imported) does not contain any possibly overridden values either (only the original/actually imported values). But I wasn't sure whether to put that into ConfigOverrideTest or ConfigInstallTest... will add that test, as soon I/we have figured out where to add it ;)

That's good news! :)

1) Yeah, I really don't like the idea that a new Config() automatically goes ahead and magically loads something/anything. The Config object does not need to load something, and there are use-cases for that. Whether something needs to be loaded or not is a decision the calling code needs to make. It's the common case for the procedural config() helper, but I really wouldn't like to go beyond that.

Likewise, since there is no dependency on loading anything, I also do not see a problem with the API documentation. You either load something or you don't. If you don't, then it is only natural that, say, isNew() returns TRUE, because, yeah, wouldn't it be wrong to expect that a new Config object that hasn't loaded any data is NOT new? ;) The methods on the Config object operate on the defined current data and state of the Config object itself only, and that's it. :)

Of course, I'm happy to be convinced otherwise, but, yeah... ;)

2) I can totally see where you're heading with the multiple overrides, and even considered already to change $this->overriddenData into a top-level-keyed array of "override sources" - e.g.:

  $this->overriddenData['conf'] = $data; // $conf
  $this->overriddenData['...'] = $data; // $whatever

However, at this point, this change (and likewise your suggestion) looks and feels preemptive/premature to me -- not so comfortable with doing that without the other architectural multi-context/override changes. Would you be OK with deferring this change to that issue? (whichever it is - I'd highly appreciate a meta-issue [or re-using the old roadmap issue] for laying out the plan of attack regarding contexts/overrides ;))

when you create a new one, you can always stop if from loading later just setting some data on it with setData() if that is your decision.

That's essentially one of the primary reasons for why I object this. Class methods that are clean only ever perform the action that was asked for. If I ask the class to get a value, then it should only get a value. The class shouldn't have any business in blatantly assuming when I want to auto-load or not — that's my business. :)

"You have to ->set() some data first, before you can use ->get() without triggering an auto-load" is the exact definition of "anti-clean" in my world ;)

However, please note that I left the ->resetCurrentData() calls in place for all data manipulation operations; i.e., ->set(), ->setData(), ->clear(), and ->delete() empty out the current data, so that overrides are re-merged upon the next ->get(). This is the part that made sense for me. :)

> The global $conf overrides are supposed to work dynamically at runtime.

1. Why? (Is that documented anywhere?)

It's not documented anywhere, but it's the behaviour of $conf overrides in Drupal 7 and D8 up until now. We only have the override there at all for backwards compatibility in lieu of something better turning up, so I'm not really keen on removing that feature without more discussion than there's been here.

In the back of my mind I've been thinking the $conf override could move to a second 'active store' that's checked before the main one, not sure if that's a good idea but it would isolate the global hackery to a storage implementation rather than having it baked anywhere in the config system.

Moving back to needs review in the hope this gets some more review, I haven't reviewed the rest of the changes in the patch yet but will try to when I've got more time.

Read back through the issue a bit and saw the LRU cache discussion, just crossposting #1199866: Add an in-memory LRU cache.

Please note that this patch does not remove the dynamic nature of $conf overrides entirely. Let me explain the difference:

Given hypothetical code of:

modules/do/config/do.something.yml:
execute = '1'

modules/do/do.module:
function do_something() {
  $config = config('do.something');
  module_invoke_all('do_something');

  $execute = $config->get('execute');
  var_dump($execute);
  if ($execute) {
    do_it();
  }
}

modules/custom_hack/custom_hack.module:
// Implements hook_do_something().
function custom_hack_do_something() {
  $GLOBALS['conf']['do.something']['execute'] = FALSE;
}

Before this patch (HEAD):

• Result: (bool) false

After this patch:

• Result: (string) "1"

Details:

• Global $conf overrides are only applied once when a Config object is instantiated.
• If the do.something override was defined in settings.php, it would have been applied.
• If the do.something override was dynamically injected into $conf during hook_init() or any other event/code/location that runs before do_something(), it would have been applied.
• If do_something() would reload the config object after invoking hooks (i.e., a second $config = config('do.something');), then the override would be applied.

Clarifying the issue title. I'd love to move forward here.

For now, as long as there is no static caching of config objects, I think this patch is a considerable improvement over the current code. AFAICS, the edge-cases I outlined in #18 do not exist throughout core currently.

It's also not clear at all yet whether we're going to add static caching for config objects. And even if we will, the layer that applies the static caching could simply call into $config->resetCurrentData() after retrieving the existing object from cache and before returning it to the caller, so any possible $conf changes would be re-applied/merged.

I think catch's complaint is well-addressed and so this is RTBC again but i would like a better name for $overriddenData; this because overriddenData suggests this contains the data with the overrides applied but what it contains are the overrides themselves. What do you think of calling it $overrides simply? In fact, what the previous called currentData I would call overriddenData, the explanation for protected $overriddenData says The current runtime data ($data + $overrides). That looks quite right, doesn't it?

The naming tweak in #20 makes sense to me, thanks.

Then I will RTBC this back because it was RTBC, catch had concerns which were addressed and I just did a few search-replace on the patch itself (and then applied it to see whether the tests still pass).

#20: 1671198_20.patch queued for re-testing.

Rerolled for NestedArray conversion. As the previous patch doesn't apply, I couldn't produce traditional interdiff but I created something which should help checking what I did is correct: I diffed the patches themselves.

Updated for #1380958: Replace $modules list for WebTestBase::setUp() with ::$modules class properties (plus a couple minor style cleanups).

#26: config-1671198-26.patch queued for re-testing.

OK the patch seems fine, but when reading it it mostly makes me want to change the dual storage mechanism into a chain similar to the work going on in #1651206: Added a cache chain implementation. Then ConfStorage could be inserted in front of DatabaseStorage in that chain and the actual Config class and Config factory don't need to know anything about it. I know this has been mentioned several times, mainly as a way of handling the 'write to files all or not all the time' issue as well as potentially adding caching without baking it into any particular storage controller, but no idea where that issue is if it's anywhere at all. Should I open one?

Yes, I also think that the config system should not have any special/built-in knowledge about global $conf. We discussed that many times already, as part of the "config contextual overrides" discussions.

Introducing a proper override system is the goal and purpose of these two issues:

#1646580: Implement Config Events and Listeners, and storage realms for localized configuration
#1616594: META: Implement multilingual CMI

This patch here just simplifies and cleans up the current support for $conf a bit.

That said, it might make sense to commit #1671080: Remove StorageDispatcher to simplify configuration system first.

With the StorageDispatcher gone and this issue pretty much blocking the other two I would recommend going ahead with this one.

#26: config-1671198-26.patch queued for re-testing.

Hopefully correct reroll

Thanks for the reroll. Here is a diff of the patches themselves for easy review.

OK I've gone ahead and committed this, but I've re-opened #1702080: Introduce canonical FileStorage + (regular) cache since it's a small, focused issue dealing with the hardcoding/assumptions here.