Use standard discovery facility in TypedConfigManager instead of one-off implementation

The current hook name is a lie, it does not alter the type info but rather it alters an actual element (and supposed to alter it based on type, but that is not really the scope of the hook now). It can alter anything on an actual element. Vs. the name of the hook suggesting it would alter type info (a list of all the types in their abstract level, not as applied to elements).

Well, I'm not so sure about this one.

@Gábor, right it doesn't alter the type itself, but also it doesn't alter the element itself but the DataDefinition of the element

I think it's like Type Definition + When applied to actual data instances -> Data Definition. Only our data definitions happen to need some preprocessing by using actual data. So yes, the whole thing is confusing.

Anyway, what I think we really need is that 'hook_config_type_info_alter()' mentioned on some other issue and dismissed. Because by altering the 'type definition', that would be inherited by the corresponding 'data definition' and we'd have a lot less stuff to alter.

I'm thinking: what about adding proper discovery for TypedConfigManager and then we just add one of that alter hook decorators?

Whatever, not having that 'hook_config_type_info_alter()' is causing us all kind of weird hook implementations and issues for us (and config translation) so maybe we should retry that one in the first place ?

This patch:
- Uses proper discovery classes for TypedConfigManager so it gets still simpler.
- Adds a ConfigSchemaDiscovery, as a wrapper for ExtensionInstallStorage
- Adds a config_schema_info_alter() hook that would fix all our current issues / ugly hooks with config translation.

I know it may be more than simply renaming a hook, but is the proper / clean way to get where we want IMO.

This totally makes sense. Fantastic! Can we also remove the ugly one-off code from config translation type altering to demonstrate how standard this makes that?

Retitled to something maybe better :) Review points on top of #6:

1. +++ b/core/lib/Drupal/Core/Config/Schema/ConfigSchemaDiscovery.php
   @@ -0,0 +1,50 @@
   + * Contains \Drupal\Core\Plugin\Discovery\ConfigSchemaYamlDiscovery.
   

   Not the actual path and name of the class either.

2. +++ b/core/lib/Drupal/Core/Config/Schema/ConfigSchemaDiscovery.php
   @@ -0,0 +1,50 @@
   +   * Construct a ConfigSchemaDiscovery object
   

   Dot at the end.

3. +++ b/core/lib/Drupal/Core/Config/Schema/ConfigSchemaDiscovery.php
   @@ -0,0 +1,50 @@
   +   * @param $schemaStorage
   ...
   +  function __construct(StorageInterface $schemaStorage) {
   +    $this->schemaStorage = $schemaStorage;
   

   As per our practices / coding styles, arguments are still $schema_storage, while class properties are $schemaStorage style.

4. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
   @@ -11,19 +11,17 @@
   +// Schema discovery and decorators
   

   We don't have sections in the use list elsewhere. Let's use the alphabetic ordering like we do elsewhere.

5. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
   @@ -64,7 +55,9 @@ class TypedConfigManager extends TypedDataManager implements TypedConfigManagerI
   +    // Set discovery with alterdecorator.
   

   I don't think this line adds any clarity to the code :) I would remove.

Also updated the issue summary.

Updated patch:
- Fixed @Gábor's issues in #7
- Renamed hook_config_translation_type_info_alter() to hook_config_schema_info_alter()
- Removed one-off code from config_translation, and some module handler dependency for ConfigTranslationFormBase

Still to do:
- Move hook documentation to... config module I guess (?)
- Think about completely removing config.storage.schema service and maybe ExtensionInstallStorage..

Also maybe LocaleConfigManager deserves some rewrite and simplification

Just in case someone else is wondering, the answer to my last two questions:
- Think about completely removing config.storage.schema service and maybe ExtensionInstallStorage..
Possible but that would take tying the TypedConfigManager to a specific folder storage which I don't think is a good idea (not removing ExtensionInstallStorage which is used by install system)
- LocaleConfigManager deserves some rewrite and simplification
Yes, but that's a different issue. I may explore in a separate patch how it can use but not extend TypedConfigManager saving a lot of parameters passed back and forth..

Looks good to me but testbot will be useful too :) IMHO since the hook is exposed by the base system, not the config module, it should be in system.api.php. I believe that is the standard place to system level stuff.

BTW looks good to me except the place of the docs. :)

Update patch:
- Moved hook documentation to system.api.php
(and reworderd according to new place which is 'system wide' more than 'config translation' specific)
- Fixed previous test failure which was a caching issue and as a result, an config text getting the wrong element for translation (textfield instead of text area)

I don't know whether it is the proper thing to do, but this fixes it (adding the tag to the service) in core.services.yml

  config.typed:
    class: Drupal\Core\Config\TypedConfigManager
    arguments: ['@config.storage', '@config.storage.schema', '@cache.discovery']
    tags:
      - { name: plugin_manager_cache_clear }

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -64,7 +52,8 @@ class TypedConfigManager extends TypedDataManager implements TypedConfigManagerI
+    $this->discovery = new AlterDecorator(new ConfigSchemaDiscovery($schemaStorage), 'config_schema_info');

AlterDecorator should not be used, use $this->alterInfo() instead.

@Berdir,

Thanks for reviewing.

Replaced AlterDecorator by $this->alterInfo().

Of course, the constructor in the test needs to be updated too.

Looks good :) Thanks for the review @berdir.

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -61,10 +49,13 @@ class TypedConfigManager extends TypedDataManager implements TypedConfigManagerI
+    $this->alterInfo('config_schema_info');

+++ b/core/modules/system/system.api.php
@@ -2841,6 +2841,25 @@ function hook_config_import_steps_alter(&$sync_steps, \Drupal\Core\Config\Config
 /**
+ * Alter config typed data definitions.
+ *
+ * For example you can alter the typed data types representing each
+ * configuration schema type to change default labels or form element renderers
+ * used for configuration translation.
+ *
+ * @param $definitions
+ *   Associative array of configuration type definitions keyed by schema type
+ *   names. The elements are themselves array with information about the type.
+ */
+function hook_config_schema_info_alter(&$definitions) {
+  // Enhance the text and date type definitions with classes to generate proper
+  // form elements in ConfigTranslationFormBase. Other translatable types will
+  // appear as a one line textfield.
+  $definitions['text']['form_element_class'] = '\Drupal\config_translation\FormElement\Textarea';
+  $definitions['date_format']['form_element_class'] = '\Drupal\config_translation\FormElement\DateFormat';
+}

There are been many attempts to add an alter hook to config schema and I've resisted every single one. Alter config data structures will make standard config very difficult to maintain and dynamic schema will be impossible to translate. If we can remove this part of the change and just proceed with the discovery changes I'd be happier.

I think there are possible valid use cases, like a module introducing a telephone type and modifying existing known schemas to use this type, ie. providing more specificity without busting the whole system. I see the alter hook makes it too easy to do wrong things as well. Not having the alter still makes it possible to do this for someone but they would need to swap out the typed config manager. That is certainly more of a change for a contrib module and hard to say "Drupal core supported it". I see the reasons for your resistance and not having the alter will indeed make it easier to police projects to do the right thing. It will unfortunately make the innocent improvements also possible with the alter that much harder :/ I agree the other improvements on the issue are in themselves worth it.

(Cross-posted with Gabor's comment, so I may be repeating some reasons, posting anyway)

@alexpott,

Alter config data structures will make standard config very difficult to maintain

Maybe, but I think the same kind of reasoning will apply to every other alter hook.
Won't altering plugin definitions make any plugin data more difficult to maintain?

Also if only for consistency with TypedData, which has an alter hook, you may need to be able to alter typed config accordingly.

Plus this replaces a pretty ugly hook, that happens to be already in core (hook_config_translation_type_info_alter) adding some DX consistency here. So I'd say this is not adding a hook, but fixing an existing one.. ;-)

Anyway, it was not the main purpose of this patch, so if you still think it's a bad idea, I'll be re-rolling the patch without the hook.

+++ b/core/modules/config_translation/config_translation.api.php
@@ -89,24 +89,5 @@ function hook_config_translation_info_alter(&$info) {
- * Alter config typed data definitions.
- *
- * Used to automatically generate translation forms, you can alter the typed
- * data types representing each configuration schema type to change default
- * labels or form element renderers.
- *
- * @param $definitions
- *   Associative array of configuration type definitions keyed by schema type
- *   names. The elements are themselves array with information about the type.
- */
-function hook_config_translation_type_info_alter(&$definitions) {
-  // Enhance the text and date type definitions with classes to generate proper
-  // form elements in ConfigTranslationFormBase. Other translatable types will
-  // appear as a one line textfield.
-  $definitions['text']['form_element_class'] = '\Drupal\config_translation\FormElement\Textarea';
-  $definitions['date_format']['form_element_class'] = '\Drupal\config_translation\FormElement\DateFormat';
-}

Well if we are going to keep this in - I'd like a much better description about what can go wrong with using this hook to alter existing schema... enhancing schema like what occurs in the usage in core is great - but altering :(

Also plugins are not subject to static analysis. Config is special :)

What if we disallow adding new items? That would make it a true alter :) Is that evil? :D

@Gábor,
Yes, we can forbid adding new items

/**
 * It is strictly forbidden to use this hook for adding new data types.
 */

:-P

Now seriously, these are the new comments for the hook, as suggested by @alexpott:

/**
 * Alter config typed data definitions.
 *
 * For example you can alter the typed data types representing each
 * configuration schema type to change default labels or form element renderers
 * used for configuration translation.
 *
 * It is strongly advised not to use this hook to add new data types or to
 * change the structure of existing ones. Keep in mind that there are tools
 * that may use the configuration schema for static analysis of configuration
 * files, like the string extractor for the localization system. Such systems
 * won't work with dynamically defined configuration schemas.
 *
 * For adding new data types use configuration schema YAML files instead.
 *
 * @param $definitions
 *   Associative array of configuration type definitions keyed by schema type
 *   names. The elements are themselves array with information about the type.
 */

Does it look good?

Now time for style and grammar corrections, I guess :-D

The added explanation looks good to me and it satisfied @alexpott's concern that he pushed this back from RTBC.

Rerolled.

I just rerolled this, so moving back to RTBC.

Resistance is futile :( (I will regret this)

Committed 9b7ead2 and pushed to 8.0.x. Thanks!

Thanks all! We'll hopefully see all kinds of good uses of this instead of bad.