We split up the Config class in #2215413: Split the Config class into StorableConfigBase and ConfigBase. Make ThemeSettings inherit from ConfigBase. to make it easier to re-use. Unfortunately the new classes are not well documented to explain why they are split up and when to use.

See #2201437: [META-1] Config overrides and language for some of the rationale.

Files: 
CommentFileSizeAuthor
#19 interdiff-config-docs.txt2.66 KBxjm
#19 config-2216961-18.patch7.59 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,405 pass(es). View
#15 interdiff.txt1.28 KBGábor Hojtsy
#15 config-base-class-docs-15.patch7.34 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,140 pass(es), 1 fail(s), and 0 exception(s). View
#9 config-base-class-docs.patch7.34 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,293 pass(es). View

Comments

xjm’s picture

Issue tags: +Configuration system
xjm’s picture

Title: Improve documentation of Config, ConfigBase and StorableConfigBase » Document Config, ConfigBase, and StorableConfigBase
Gábor Hojtsy’s picture

The most evident benefit of that change was the removal of several copy-pasted methods from ThemeSettings and instead relying 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.

xjm’s picture

My question from the original issue:

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?

xjm’s picture

The docblock from ThemeSettings is also less-than-helpful:

Defines the default theme settings object.

(But hey, at least it bothered to add one!) ;)

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.

Gábor Hojtsy’s picture

Note that theme conversion patch landed in 2013 May, so the existence of the copy-pasted code and the only-runtime config dates back to that.

Eli-T’s picture

Assigned: Unassigned » Eli-T
Gábor Hojtsy’s picture

Assigned: Eli-T » Unassigned
Status: Active » Needs review
FileSize
7.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,293 pass(es). View

Here is a patch with proposed wording explaining which base class does what and when to use which. I also found some methods in Config which are already defined on StorableConfigBase and are identical. So removed those too. This helps make the differences clearer as you can clearly see the methods in each class belong to their respective responsibilities.

ConfigBase (runtime config handling, eg. derived config like ThemeSettings) >>> StorableConfigBase (with storage, will be useful for non-overridable configuration like languages as proposed in #2201437: [META-1] Config overrides and language) >>> Config (also provides override support).

Status: Needs review » Needs work

The last submitted patch, 9: config-base-class-docs.patch, failed testing.

Eli-T’s picture

Have checked all removed methods are identical to their inherited counterparts, and all looks good.

Very minor comments with two of the class comments:

+++ b/core/lib/Drupal/Core/Config/ConfigBase.php
		@@ -11,6 +11,17 @@
		use Drupal\Component\Utility\String;
		use \Drupal\Core\DependencyInjection\DependencySerialization;
		
		+/**
		+ * Provides a base class for configuration objects with get/set support.
		+ *
		+ * Encapsulates all capabilities needed for runtime configuration handling for
		+ * a specific configuration name. Extend from this class for non-storable
		+ * configuration where the configuration API is desired but storage is not
		+ * possible. For example, if the data is derived runtime.

Should end ...if the data is derived at runtime..

+++ b/core/lib/Drupal/Core/Theme/ThemeSettings.php
		@@ -10,7 +10,13 @@
		use Drupal\Core\Config\ConfigBase;
		
		/**
		- * Defines the default theme settings object.
		+ * Provides a configuration API wrapper for runtime merged theme settings.
		+ *
		+ * Theme settings use configuration for base values but the runtime theme
		+ * settings are calculated based on various site settings and is therefore
		+ * not persisted.

should end and are therefore not persisted

Gábor Hojtsy’s picture

Issue tags: +Random fail

Looks like a random fail to me:

Segmentation fault
FATAL Drupal\toolbar\Tests\ToolbarAdminMenuTest: test runner returned a non-zero error code (139).

That wast the only fail. Retesting.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

9: config-base-class-docs.patch queued for re-testing.

Eli-T’s picture

The same test passes on my local with the patch applied:

Drupal test run
---------------

Tests to be run:
- Toolbar admin menu (Drupal\toolbar\Tests\ToolbarAdminMenuTest)
- Toolbar hook_toolbar (Drupal\toolbar\Tests\ToolbarHookToolbarTest)
- Toolbar menu translation (Drupal\toolbar\Tests\ToolbarMenuTranslationTest)

Test run started:
Thursday, March 13, 2014 - 15:07

Test summary
------------

Toolbar hook_toolbar 17 passes, 0 fails, 0 exceptions
Toolbar menu translation 34 passes, 0 fails, 0 exceptions
Toolbar admin menu 389 passes, 0 fails, 0 exceptions

Gábor Hojtsy’s picture

FileSize
7.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,140 pass(es), 1 fail(s), and 0 exception(s). View
1.28 KB

Fixed those concerns with the text :) @alexpott, @xjm do you think this is a worthy improvement to commit?

xjm’s picture

Assigned: Unassigned » xjm

Huge improvement. I had a long dreditor post of comments to improve the docs but then realized it'd just be faster to tweak the patch myself, so doing so. :)

xjm’s picture

Issue tags: -Random fail

Oh, the "Random test failure" tag is canonical (for the issues that are actually about particular fails, not just where they happen). See #2216701: Random test failure in ToolbarAdminMenuTest for that one.

Status: Needs review » Needs work

The last submitted patch, 15: config-base-class-docs-15.patch, failed testing.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
7.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,405 pass(es). View
2.66 KB

Clarified the relationship between the base classes and implementations a bit, and fixed a couple run-on sentences.

xjm’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Changes look very good :)

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks and reads great.

jhodgdon’s picture

Component: documentation » configuration system

The docs here look fine to me. However, this is not a docs issue (lots of code changes it looks like? Whole swaths of code are removed in the patch).

Gábor Hojtsy’s picture

@jhodgdon: the code removed is purely duplicate copy-paste repeated on the inheritance chain superfluously. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Superb, thanks, this should help a lot!

Status: Fixed » Closed (fixed)

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