Problem/Motivation

#2201437: [META-1] Config overrides and language has resulted in a patch that is too large. This issue it to take the patch in #2201437-24: [META-1] Config overrides and language and splits out the parts suggested as:

Way forward? (each step could be a separate issue)

Split the Config class into a couple of base classes so that the functionality can be reused - the attached patch does this and discovered an issue with ThemeSettings not using the same merge method as Config.
.
.

Proposed resolution

  • Add new abstract class ConfigBase and abstract subclass StorableConfigBase
  • Make Config extend StorableConfigBase
  • Make ThemeSettings extend ConfigBase

as initially committed #2201437-24: [META-1] Config overrides and language and suggested to be split out as a separate issue.

Remaining tasks

Complete patch to follow.

User interface changes

None.

API changes

ThemeSettings::mergeData() calls should now call the inherted merge() method.
All current instances of this call to be fixed in this issue.

Files: 
CommentFileSizeAuthor
#15 ConfigBaseRefactor (1).png30.97 KBGábor Hojtsy
#10 3-10-interdiff.txt584 bytesEli-T
#10 core-split_config_class-2215413-10.patch25.37 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,392 pass(es). View
#4 1-3-interdiff.txt727 bytesalexpott
#3 core-split_config_class-2215413-3.patch25.46 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,327 pass(es). View
#1 core-split_config_class-2215413-1.patch25.67 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,330 pass(es). View

Comments

Eli-T’s picture

Status: Active » Needs review
FileSize
25.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,330 pass(es). View

Attached patch should implement change in full.

alexpott’s picture

Status: Needs review » Needs work

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.

Eli-T’s picture

Status: Needs work » Needs review
FileSize
25.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,327 pass(es). View
alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
727 bytes

Thanks - this looks good to go

Gábor Hojtsy’s picture

Nice refactoring, agreed :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
Eli-T’s picture

Status: Needs work » Needs review
FileSize
25.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,392 pass(es). View
584 bytes
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Committed and pushed to 8.x. Thanks!

xjm’s picture

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

alexpott’s picture

Gábor Hojtsy’s picture

FileSize
30.97 KB

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

Gábor Hojtsy’s 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.

xjm’s picture

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

Gábor Hojtsy’s picture

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

Status: Fixed » Closed (fixed)

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