Split the Config class into StorableConfigBase and ConfigBase. Make ThemeSettings inherit from ConfigBase.

Very much +1 to this change as it will allow us to reuse StorableConfigBase to manage language overrides without them being overridable!

Also the ThemeSettings tidy up has some nice consequences - namely...

+++ b/core/lib/Drupal/Core/Config/ConfigBase.php
@@ -0,0 +1,209 @@
+  public function merge(array $data_to_merge) {
+    return $this->setData(NestedArray::mergeDeepArray(array($this->data, $data_to_merge), TRUE));
+  }

+++ b/core/lib/Drupal/Core/Theme/ThemeSettings.php
@@ -7,154 +7,37 @@
-  public function mergeData ($data) {
-    $this->data = NestedArray::mergeDeep($this->data, $data);
-    return $this;
-  }

Nice! This is a bug fix - if a theme added a setting with a numeric key it would break because of the old code. Plus reuse of the ConfigBase by everything means that ThemeSettings has way more test coverage too.

+++ b/core/lib/Drupal/Core/Theme/ThemeSettings.php
@@ -7,154 +7,37 @@
-   * The theme of the theme settings object.
+   * The name of the theme settings object.
...
-   * @param string $name
+   * @param string $theme

This is not really a name especially in with respect to how the configuration system uses the term. ThemeSettings objects are nameless. This is the theme that the settings are for.

Thanks - this looks good to go

Nice refactoring, agreed :)

I should not have rtbc'd this being that it is a rework of my work on #2201437: [META-1] Config overrides and language - reviews please.

I reviewed the patch when I posted #5, I think it is totally RTBC.

Looks good to me — just one remark:

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -391,56 +218,23 @@ protected function resetOverriddenData() {
   public function clear($key) {
...
-    return $this;
...
+    return $this->resetOverriddenData();

(and elsewhere)

Could we change the return lines to back to return $this?

I'm aware it is still $this, but returning it explicitly avoids having to look up what the called method returns (and whether the phpDoc is correct).

Yay, still looks good. Does the same as before. Also @sun-approved[TM]. What more to ask? :)

This looks straight-forward enough. One more beta blocker down the drain!

Committed and pushed to 8.x. Thanks!

+++ b/core/lib/Drupal/Core/Config/ConfigBase.php
@@ -0,0 +1,209 @@
+abstract class ConfigBase extends DependencySerialization {

+++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
@@ -0,0 +1,185 @@
+abstract class StorableConfigBase extends ConfigBase {

These are kind of important classes to be missing docblocks altogether. I also don't understand the purpose of this change from reading this issue, the parent issue, or the patch.

@xjm - good point - created #2216961: Document Config, ConfigBase, and StorableConfigBase to address.

@xjm: the most evident benefit is the removal of several copy-pasted methods from ThemeSettings and instead rely on actual OOP features :) Here is the before/after picture:

Also StorageConfigBase is very useful to do other config storages that may not support overrides for example as proposed by #2201437: [META-1] Config overrides and language for language.

Ahh. A picture truly is worth a thousand words.

I still don't get the deal with ThemeSettings, though. Why does it need its own specialflower subclass? Will #1067408: Themes do not have an installation status and friends make this obsolete? And how do we deploy it if it's not storable? Like. What does config that's not storable even mean?

Edit: better discuss this in #2216961: Document Config, ConfigBase, and StorableConfigBase

@xjm: the answer to that lies in http://drupalcode.org/project/drupal.git/commitdiff/31513883d9b15c997c14... which was committed from #1712250: Convert theme settings to configuration system. The class is only used for runtime config wrapping. See the theme.inc hunk there for how it merges all kinds of data like global theme settings, theme defaults, etc. to construct the config relevant for this specific theme. This is not storable, it will need to be regenerated based on these things later on. Its derivative data, so the configuration storage system is not really suitable for it.