Document Config, ConfigBase, and StorableConfigBase

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.

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?

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

Defines the default theme settings object.

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

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

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.

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

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

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.

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

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

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

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

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.

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

Changes look very good :)

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

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

Committed and pushed to 8.x. Thanks!

Superb, thanks, this should help a lot!