This patch implements a number of improvements to the Config object without any real* funtional change. Though these may seem unrelated issues, I think they make sense all in one patch for easier discussion:

- Delay config loading till the data is actually used, which will save some queries when building config objects.
- Move global $conf overrides to the ConfigFactory. It seems to me that using a global $conf inside a Config object is really bad encapsulation.*
- Use config name as a parameter for Config object constructor, which makes it cleaner and soves some lines.
- For the rest, it adds more flexible config overrides which are just read but never saved, that in case we need some runtime override of some value for anything.

* Minor changes to previous behavior:
- If you create a config object outside of the config factory, it won't get overrides which is actually a feature for install operations and in any case for giving modules full control over config objects
- Overrides from global $conf are only applied when the config object is created, so changing global $conf after we got the config object won't have any effect on it (which I believe is also a feature as as it gives us better encapsulation and more predictable data). See short discussion on #4, #5, #6 below

Also this is a small step for a bigger feature D8MI, which is #1616594: META: Implement multilingual CMI for what we need more flexible config overrides.

Files: 
CommentFileSizeAuthor
#34 interdiff.txt3.02 KBchx
#33 config-1671198-33.patch11.13 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 40,010 pass(es). View
#26 config-1671198-26.patch10.49 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-1671198-26.patch. Unable to apply patch. See the log in the details link for more information. View
#26 interdiff.txt1.21 KBxjm
#25 1671198_25.patch10.44 KBchx
PASSED: [[SimpleTest]]: [MySQL] 39,814 pass(es). View
#25 interdiff.txt753 byteschx
#20 1671198_20.patch10.45 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1671198_20.patch. Unable to apply patch. See the log in the details link for more information. View
#8 config.cleanup.8.patch10.42 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,273 pass(es). View
#8 interdiff.txt11.63 KBsun
#2 1671198-2-config_override.patch7.11 KBJose Reyero
PASSED: [[SimpleTest]]: [MySQL] 37,057 pass(es). View
config-override.patch6.92 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] 36,733 pass(es), 313 fail(s), and 253 exception(s). View

Comments

Status: Needs review » Needs work

The last submitted patch, config-override.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
7.11 KB
PASSED: [[SimpleTest]]: [MySQL] 37,057 pass(es). View

Fixed version, just one line, missing build() in Config::set()

Jose Reyero’s picture

Some extracts from the patch (Before/After):

Example 1: Get configuration value (Config class)

Before: Note global array is merged every time and this is really bad encapsulation as we get values from global variables.

class Config {
  public function get($name) {
    global $conf;

    $name = $this->getName();
    if (isset($conf[$name])) {
      $merged_data = drupal_array_merge_deep($this->data, $conf[$name]);
    }
    else {
      $merged_data = $this->data;
    }
    ....
  }
}
....

After: global variable is only in the config factory, it is computed just once. Then overrides are merged only once, when building current values, we save an array merge for each subsequent Config::get().

class ConfigFactory {
  public function get($name) {
    global $conf
    $config = new Config($name, $this->storageDispatcher);
    // Add any overridden values in global $conf
    if (isset($conf[$name])) {
      $config->setOverride($conf[$name]);
    }
    return $config;
  }
}

class Config {
  public function build() {
    if (!isset($this->current)) {
      if (!isset($this->data)) {
        $this->load();
      }
      $this->current = $this->data;
      if (!empty($this->override)) {
        $this->current = drupal_array_merge_deep($this->current, $this->override);
      }
    }
    return $this;
  }
}

Example 2:Create config object, config.inc

Before:

        $old_config = new Config($storage_dispatcher);
        $old_config
          ->setName($name)
          ->load();

After: Improved Config constructor, it doesn't need explicit loading as it will be done when needed.

       $old_config = new Config($name, $storage_dispatcher);
sun’s picture

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?

Jose Reyero’s picture

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

1. Why? (Is that documented anywhere?)
2. So you mean there's no chance a piece of code can create a config object and be sure a minute later (after any other function call) that it is still the same object with the same values? That is what I'd call really bad encapsulation as we would be passing parameters through globals.
3. This is also an issue with any module manipulating (updating) config objects as you cannot know whether the value you get with $config->get() is the same that is going to be saved.

About that schema you mention for caching config objects I'd like to know where it is being developed / documented because I have serious objections with that and this is not the place for discussing them.

sun’s picture

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

Jose Reyero’s picture

@sun,
Ok, added that notes to the patch description at the top for easier review.

About LRU cache, I think we would be better off with a plain static one which would be a significant performance improvement anyway over D7 and would add the feature of reusing config objects in a predictable way. Anyway please let's not discuss this here, just let me know when there's an issue to discuss that. (Shouldn't we create an issue for adding static caching to config factory?)

sun’s picture

Assigned: Unassigned » sun
FileSize
11.63 KB
10.42 KB
PASSED: [[SimpleTest]]: [MySQL] 37,273 pass(es). View

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.

sun’s picture

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

Jose Reyero’s picture

Ok with the renaming and most of the changes, the only issue is you seem to have drop completely the 'deferred load', which I think was quite straight forward, and besides simplifying a little bit the code (you don't need explicit loading of Config objects) was fixing some issue with API documentation.
That is when a function is passed a Config object you cannot know whether it's been loaded or not (loaded is not the same as isNew).
If we don't have that, for good API documentation, we'd need to add on every function taking a config object whether it expects a loaded one or not because their behavior is quite different depending on that.

Also I'd ask for one more change that was coming in my next version of the patch, in order to allow multiple overrides merging them together one after another:

public function setOverride(array $data) {
- $this->overriddenData = $data;
+ $this->overriddenData = isset($this->overriddenData) ? drupal_array_merge_deep($this->overriddenData, $data) : $data;
...
sun’s picture

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

Jose Reyero’s picture

1) It is mostly a consistency issue.

For a config object not loaded, isNew() will be always true, whatever is in the db
If the object is loaded, isNew() will depend on whether it had any data previously or not in the db / other storage.

Same when do do a set() operation, it may merge with loaded data or create new data depending on whether it was loaded previously or not.

The whole point of this is when you pass around config objects, not needing to go back 100 lines of code to see whether it was loaded or not before. Still, 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.

These objects, that behave differently depending on past history and may be either coulpled/or decoupled from storage are really bad for debugging code.

2) Ok, so you've got more time to reconsider 1) ;-)

(But you can see it here where we've got global $conf overrides on top of locale overrides, #1646580: Implement Config Events and Listeners, and storage realms for localized configuration

sun’s picture

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

Jose Reyero’s picture

Status: Needs review » Reviewed & tested by the community

Ok, since we have different ideas of what a good API is, I just give up that point.

Please go ahead with the rest of the patch.

catch’s picture

Status: Reviewed & tested by the community » Needs review
> 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.

catch’s picture

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

Jose Reyero’s picture

About $conf overrides, the only usage that AFAIK is documented for Drupal core is using it in settings.php (changing it by modules at runtime is just an ugly hack IMHO but not explicitly supported by Drupal core so there's nothing really to support here).
Anyway, if you want to see some example of how to work it out cleanly, really isolating the 'hackery' check out #1646580: Implement Config Events and Listeners, and storage realms for localized configuration

sun’s picture

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.
sun’s picture

Title: Small improvements to Config object. Better config overrides, more flexible and encapsulated, +performance deferring config load » Merge $conf overrides only once per instantiated Config object, and move initial setName() into Config constructor

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.

chx’s picture

FileSize
10.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1671198_20.patch. Unable to apply patch. See the log in the details link for more information. View

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?

sun’s picture

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

aspilicious’s picture

Issue tags: -Configuration system

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

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system

The last submitted patch, 1671198_20.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
753 bytes
10.44 KB
PASSED: [[SimpleTest]]: [MySQL] 39,814 pass(es). View

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.

xjm’s picture

FileSize
1.21 KB
10.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-1671198-26.patch. Unable to apply patch. See the log in the details link for more information. View
sun’s picture

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

catch’s picture

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?

sun’s picture

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.

chx’s picture

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

aspilicious’s picture

Issue tags: -Configuration system

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

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system

The last submitted patch, config-1671198-26.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
11.13 KB
PASSED: [[SimpleTest]]: [MySQL] 40,010 pass(es). View

Hopefully correct reroll

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.02 KB

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

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.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.