hook_config_schema_info_alter has the following documentation:

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

Given the importance of static parsing to translatability and validation pre import I think we should enforce this.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the statci analysis of configuration using schema is how we translate it.
Issue priority Normal because core does not use hook_config_schema_info_alter this way.
Unfrozen changes Unfrozen because it is a normal bug.
Files: 
CommentFileSizeAuthor
#13 2414991.13.patch9.57 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,747 pass(es). View
#10 2414991.10.patch9.43 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2414991.10.patch. Unable to apply patch. See the log in the details link for more information. View
#10 6-10-interdiff.txt6.12 KBalexpott
#6 2414991.6.patch7.41 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,258 pass(es). View
#6 1-6-interdiff.txt1.49 KBalexpott
#1 2414991.1.patch7.29 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,894 pass(es), 0 fail(s), and 1 exception(s). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
7.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,894 pass(es), 0 fail(s), and 1 exception(s). View
Gábor Hojtsy’s picture

Patch looks good. Adding related issue.

tstoeckler’s picture

Because of the strict check (!==) this will also fail if someone chooses to reorder the schema types. I don't know why anyone would do that, but it seems strange to throw an exception in that case nonetheless.

Gábor Hojtsy’s picture

I support this patch because it removes a loophole that people attempted to use for dynamic schema generation. This will make issues like #2382671: Automatically define schema for themes impossible to implement. I think that's a good thing and we should work on tooling to help that instead of leaving and using loopholes. See #2266357: Add basic schema autogeneration based on data introspection.

Status: Needs review » Needs work

The last submitted patch, 1: 2414991.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
7.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,258 pass(es). View

Fixed the test - problems caused by the fact that uninstall can not unload code so the hook still fires even after the config_schema_test module has been uninstalled as part of ConfigInstallTest.

Fixed #3.

dawehner’s picture

An alternative could be that config translation replaces/decoratores the TypedConfigManager and we just don't throw the alter hook.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

@dawehner: the alter hook was originally in config translation, see #2145633: Use standard discovery facility in TypedConfigManager instead of one-off implementation. The reason it was moved one layer up is so modules can also alter more general things, like the class implementation of types, eg. if they want to swap out the class backend implementation for mappings and sequences for better debugging (config translation not even involved).

I think this approach/patch looks good.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Looks good apart from a consistent typo. Assuming because it's consistent that's why tests still pass. One minor question on the exception message.

  1. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
    @@ -295,4 +296,15 @@ public function hasConfigSchema($name) {
    +  protected function alterDefintions(&$definitions) {
    

    s/alterDefintions/alterDefinitions

  2. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
    @@ -295,4 +296,15 @@ public function hasConfigSchema($name) {
    +    parent::alterDefintions($definitions);
    

    And here.

  3. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
    @@ -295,4 +296,15 @@ public function hasConfigSchema($name) {
    +      throw new ConfigSchemaAlterException('Invoking hook_config_schema_info_alter() has added or removed schema definitions');
    

    Is it worth adding the diff to the exception?

  4. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -224,9 +224,7 @@ protected function findDefinitions() {
    +    $this->alterDefintions($definitions);
    

    And here.

  5. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -242,4 +240,16 @@ protected function findDefinitions() {
    +  protected function alterDefintions(&$definitions) {
    

    And also here.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.12 KB
9.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2414991.10.patch. Unable to apply patch. See the log in the details link for more information. View

Fixed everything from #9. I ummed and ahhed already about listing the differences in the exception. I think we should because it is super helpful to the developer.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

The changes look all good. Good catch on the typo and the additional debug info will indeed be super useful.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2414991.10.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,747 pass(es). View

  • catch committed e152fef on 8.0.x
    Issue #2414991 by alexpott: Prevent hook_config_schema_info_alter from...
catch’s picture

Title: Prevent hook_config_schema_info_alter from adding or removing defintions » Prevent hook_config_schema_info_alter from adding or removing definitions
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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