Updated: Comment #15

Problem/Motivation

While core no longer casts configuration to strings, HEAD has a lot of incorrect YAML files.

Proposed resolution

Write a script that uses the config schema to cast.

Remaining tasks

#13 committed the passing patch in #7

Review the attached no_schema.txt and do them manually. (See #15)

This task covered most of them except:
1) config that doesn't have a schema
2) config of test modules.

To deal with the remaining:
Approach 1: We can use those sub-tasks to validate and make sure all the type-castings are removed. If nothing else to do, we can close them.

Approach 2:
The no-schema list that chx attached in this issue (https://drupal.org/files/no_schema.txt) can be fixed, if we can add schema (also adding schema helps other places as well). So I thought below steps would be good to take in order
1. Provide missing schemas and get them into core.
2. Run @chx script and issue one more patch as Core config has everything as string - part 2
3. Then move to sub-tasks and do validation, etc (all the old story above).
This way we can make most of them done before move to sub-task.

For Approach 2 #1910624: [META] Introduce and complete configuration schemas in all of core tracks adding schemas. Go there to help. :)

User interface changes

None.

API changes

None.

#1653026: [META] Use properly typed values in module configuration

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

FileSize
2.03 KB
176.16 KB

Improved the discovery of config files so that we find them in the subdirectories. Up to 1464.

I have all modules enabled and now most configs get rewritten if it has a schema. The only exceptions are generated field.field.* , field.instance.* and taxonomy.vocabulary.forums . I think this is as complete as scripting can get.

chx’s picture

FileSize
175.16 KB

Small opsie: I lost the comments from core/modules/filter/config/filter.format.plain_text.yml ; restored.

chx’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, 2106459_2.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
173.4 KB
2.32 KB

Here, this avoids adding NULLs which caused the fails (we added permissions: null to the anonymous role because the system was descending and thus creating every key that had a schema even if it didn't actually exist in config) and avoids rewriting files not needing rewriting. 1442 conversions done.

tim.plunkett’s picture

Only noticed two problems.

  1. +++ b/core/modules/block/custom_block/config/custom_block.type.basic.yml
    @@ -1,5 +1,6 @@
    +status: false
    

    I think this should have had a status: true in here, but the script it filling in the defaults.

  2. +++ b/core/modules/book/config/node.type.book.yml
    @@ -3,17 +3,16 @@ uuid: c5ca890d-7db7-4c45-bf0f-0a12430923ff
    -      # Not promoted to front page.
    
    +++ b/core/modules/forum/config/node.type.forum.yml
    @@ -3,17 +3,16 @@ uuid: c14b392d-0889-46bd-889e-a239e8b6cd89
    -      # Not promoted to front page.
    

    Losing a comment here.

chx’s picture

FileSize
1.78 KB
173.03 KB
2.27 KB

Moved the NULL protection up.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! This is awesome.

chx’s picture

FileSize
6.4 KB
44.4 KB

This gets us 3/4 there: ag "'\d" core/**/config/*yml reports 520 occurences across 102 files :(

chx’s picture

FileSize
182.25 KB
6.72 KB

Anything that is at least two digits must be a number. The lack of schema coverage stands in #9, this is done by shell scripting: while read line; do sed -i -r -f pattern $line; done < "files.txt" where the file pattern contains s/: '([0-9]{2,})'/: \1/. This adds 24 more. Not a lot but every bit helps. We can also use this technique to pick up practically anything that looks like numbers or booleans and do them in batches instead of manually. (Promote etc is the next one)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2106459_10.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community

Just commit #7 we will deal with #10 later (it's permissions probably being parsed into 755 instead of 0755. Lots of fun!)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

AWESOME work. Thanks so much for this.

Committed and pushed to 8.x. YEAH! :D

I'll try and do another CMI test this week to ensure this fixes the issue I ran into.

jhodgdon’s picture

So if this has been committed, do we still need all the sub-tasks on #1653026: [META] Use properly typed values in module configuration? As maintainer of one of the components that got assigned a sub-task, I kind of need to know.

vijaycs85’s picture

@jhodgdon This task covered most of them except:
1) config that doesn't have a schema
2) config of test modules.

Approach 1: So we can use those sub-tasks to validate and make sure all the type-castings are removed. If nothing else to do, we can close them.

Approach 2:
The no-schema list that chx attached in this issue (https://drupal.org/files/no_schema.txt) can be fixed, if we can add schema (also adding schema helps other places as well). So I thought below steps would be good to take in order
1. Provide missing schemas and get them into core.
2. Run @chx script and issue one more patch as Core config has everything as string - part 2
3. Then move to sub-tasks and do validation, etc (all the old story above).
This way we can make most of them done before move to sub-task.

I'm on Approach 2(#1910624: [META] Introduce and complete configuration schemas in all of core has sub-tasks for schema and you are most welcome to help there) :).

vijaycs85’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

to be clear about what was done/committed and what to do next (schemas and checking other sub issues)

YesCT’s picture

updated the summary. :)

YesCT’s picture

Issue summary: View changes

corrected updated comment number

Status: Fixed » Closed (fixed)

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