Problem/Motivation

TypedConfigManager uses a custom implementation of discovery instead of relying on a discovery class with potential decorators. We can rely on Drupal 8 patterns instead and remove some custom code.

Proposed resolution

- Implement a discovery class for configuration schema.
- Add an alter decorator to allow altering instead of one-off implementation we used to have.
- Clean up the cache implementation by relying on existing code in the parent.

Remaining tasks

  • Resolve review points
  • More review

User interface changes

No.

API changes

config_translation_config_translation_type_info_alter() is replaced by hook_config_schema_info_alter(), same semantics.

Files: 
CommentFileSizeAuthor
#29 5633-schema-discovery-29.patch12.28 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,318 pass(es). View
#26 5633-schema-discovery-26.patch14.71 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 5633-schema-discovery-26.patch. Unable to apply patch. See the log in the details link for more information. View
#26 5633-interdiff-18-26.txt1.04 KBJose Reyero
#18 5633-interdiff-16-18.txt699 bytesJose Reyero
#18 5633-schema-discovery-18.patch14.26 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,283 pass(es). View
#16 5633-schema-discovery-16.patch13.58 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,639 pass(es), 1 fail(s), and 2 exception(s). View
#16 5633-interdiff-14-16.txt4.47 KBJose Reyero
#14 5633-schema-discovery-14.patch10.66 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,965 pass(es). View
#14 5633-08-14-diff.txt3 KBJose Reyero
#8 5633-interdiff-04-08.txt6.44 KBJose Reyero
#8 5633-schema-discovery-08.patch8.49 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,725 pass(es), 1 fail(s), and 0 exception(s). View
#4 5633-schema-discovery-05.patch4.28 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,731 pass(es). View

Comments

YesCT’s picture

Issue tags: -sprint

oops. probably not on sprint.

Gábor Hojtsy’s picture

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

Jose Reyero’s picture

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 ?

Jose Reyero’s picture

FileSize
4.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,731 pass(es). View

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.

Jose Reyero’s picture

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

Status: Needs review » Needs work
Issue tags: +sprint

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?

Gábor Hojtsy’s picture

Title: Convert ..config_translation_type_info_alter into a real typed data alter hook » Use standard discovery facility in TypedConfigManager instead of one-off implementation
Issue summary: View changes

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.

Jose Reyero’s picture

FileSize
8.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,725 pass(es), 1 fail(s), and 0 exception(s). View
6.44 KB

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

Jose Reyero’s picture

Also maybe LocaleConfigManager deserves some rewrite and simplification

Jose Reyero’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review

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.

Gábor Hojtsy’s picture

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

Status: Needs review » Needs work

The last submitted patch, 8: 5633-schema-discovery-08.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
3 KB
10.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,965 pass(es). View

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 }
Berdir’s picture

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

Jose Reyero’s picture

FileSize
4.47 KB
13.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,639 pass(es), 1 fail(s), and 2 exception(s). View

@Berdir,

Thanks for reviewing.

Replaced AlterDecorator by $this->alterInfo().

Status: Needs review » Needs work

The last submitted patch, 16: 5633-schema-discovery-16.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
14.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,283 pass(es). View
699 bytes

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good :) Thanks for the review @berdir.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

Gábor Hojtsy’s picture

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.

Jose Reyero’s picture

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

alexpott’s picture

+++ 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 :(

alexpott’s picture

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

Gábor Hojtsy’s picture

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

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
14.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 5633-schema-discovery-26.patch. Unable to apply patch. See the log in the details link for more information. View

@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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 5633-schema-discovery-26.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
12.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,318 pass(es). View

Rerolled.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

I just rerolled this, so moving back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Resistance is futile :( (I will regret this)

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

  • alexpott committed 9b7ead2 on 8.0.x
    Issue #2145633 by Jose Reyero, Gábor Hojtsy | YesCT: Use standard...
Gábor Hojtsy’s picture

Issue tags: -sprint

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

Status: Fixed » Closed (fixed)

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