Problem/Motivation
Over in #1653026: [META] Use properly typed values in module configuration we stopped casting all configuration to strings. In #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct we started to use config schema to ensure that config was saved with the correct data types. Unfortunately many of schemas are incomplete or incorrect and where they are correct the default configuration might be wrong.
Proposed resolution
We should add a test that reads all the default configuration provided by modules and profiles and checks to see if it complies with the provided schema. The test should also report that if schema are missing.
Remaining tasks
- todo: solve #2172941: Endless looping on Config::castValue() for undefined config data type. It is blocking this issue.
- todo: Fix all the things. (get it green)
- todo: review
Not to be solved in this issue
- #2172931: Explore Config schema fallback and mismatch was split off from this issue to make this more reasonable. (not blocking this)
User interface changes
None
API changes
None
Suggested commit message
Issue #2167623 by danilenko_dn, sidharthap, Nitesh Sethia, krishnan.n, aitiba, alexpott, ashwinikumar, Barrett, damiankloip, deepakaryan1988, foxtrotcharlie, ianthomas_uk, neetu morwani, nonsie, piyuesh23, Sharique, sivaji, sushantpaste, swentel, vijaycs85, YesCT: Add test for all default configuration to ensure schema exists and is correct.
Comment | File | Size | Author |
---|---|---|---|
#73 | 2167623.73.patch | 362.12 KB | alexpott |
#73 | 70-73-interdiff.txt | 2.5 KB | alexpott |
#70 | 2167623-diff-65-70.txt | 985 bytes | vijaycs85 |
#70 | 2167623-default-config-schema-fix-70.patch | 361.77 KB | vijaycs85 |
#65 | 2167623-diff-60-65.txt | 6.09 KB | vijaycs85 |
Comments
Comment #1
alexpottDefault configuration 83 passes, 2490 fails, and 0 exceptions!!!!
The patch attached detects incomplete schema (views are the cause of most of the fails) and mismatches between default config and what the schema declares.
One issue is the patch only scans the minimal profile at the moment - will have to extend InstallStorage to fix this.
Comment #2
alexpottThere's no such thing as a major beta blocker :)
I think considering that this patch adds a test we should complete all the schema / fix them / fix default config here - otherwise we never going to be sure we've got it right.
So this will involved config schema and default config change - therefore it is beta blocking and critical.
Comment #3
alexpottMore fixes.
Comment #6
vijaycs85Update:
Moved total pass from 114 to 183 and below is the remaining fail status:
Comment #8
alexpottManageFieldsTest::testLockedField()
Comment #10
vijaycs85Focused on views and here is the updates:
Problems:
Over all the total fails reduced from 2100 to 330.
Comment #12
YesCT CreditAttribution: YesCT commentedso.. we are discussing this in the d8mi meeting.
will we be closing all the remaining issues in #1910624: [META] Introduce and complete configuration schemas in all of core and #1653026: [META] Use properly typed values in module configuration ?? and doing those things in this issue?
Comment #13
vijaycs85@YesCT, we have merged all the sub-tasks already. The remaining work on that front is, closing them and get all committers to add in this issue commit message.
Comment #14
Gábor HojtsyComment #15
vijaycs85Adding commit message with committers from other sub-tasks (Ref: task list)
Comment #16
vijaycs85On this patch:
Comment #18
vijaycs85Removing locale.config.it.tour.tour.tour-test.yml
Comment #20
damiankloip CreditAttribution: damiankloip commentedHere is a reroll for now. Let's see where that gets us.
Comment #23
vijaycs85Comment #24
vijaycs85Comment #26
vijaycs85More update on schemas...
Comment #28
vijaycs85Updating fix for ConfigCRUDTest fail.
Comment #31
vijaycs85Updated few test cases on config_test module, not sure it would break what we are trying to test, but it is all green in ConfigSchemaTest.php.
Comment #34
vijaycs85Comment #35
YesCT CreditAttribution: YesCT commentedThe interdiff looked good.
?
I'm going to try and figure out how I can look up in the schema what type of thing value should be.
(and in general how to locate the schema for parts of various configs)
Might be able to talk to vijay later.
Comment #37
vijaycs8534: 2167623-default-config-schema-fix-34.patch queued for re-testing.
Comment #39
vijaycs85Made few assumptions in configs:
Comment #40
YesCT CreditAttribution: YesCT commentedplease check to see if update to issue summary is accurate. (that #2172941: Endless looping on Config::castValue() for undefined config data type is *blocking* this.)
Comment #41
YesCT CreditAttribution: YesCT commentedI see the new type: views_entity_row, it has view_mode. But where did relationship go?
Comment #42
vijaycs85it is part of views_row data type.
Comment #43
vijaycs85Updating commit message from remaining sub-tasks.
Comment #44
YesCT CreditAttribution: YesCT commentedThese are notes of things I want to check. I started a day or two ago looking at patch #31, so some stuff might have changed with the more recent patch. Posting here so I dont lose the notes.
I'll try and answer my own questions and come back here with the answers.
Especially I want to look up what the types should be.
Vijay explained to me how to do that. Now I get to practice it.
Some of these are nits I would not mark it needs work for, and I can make those nit changes.
Add @throws doc
maybe override should be true?
this looks weird. is that a string, 'numeric' ?
maybe '' (empty string) or null ?
this should probably be false
is max_length an int?
[edit]
trying to look up what type max_length should be:
let's see. here are the steps I tried tracking this down:
views.view.test_aggregator_items.yml
it is in the alter part of plugin_id: aggregator_title_link
$ ag "aggregator_title_link" core
TitleLink.php has * @PluginID("aggregator_title_link")
so. I want a schema file in aggregator that has something to do with views.
first try opening files in phpstorm that might match views*schema
core/modules/aggregator/config/schema/aggregator.views.schema.yml
gr. it's empty.
try the file locator button in phpstorm.
there is another schema file there.
core/modules/aggregator/config/schema/aggregator.schema.yml
no that's not views stuff.
maybe the schema is missing.
check the fails on the issue to see if there is a "no schema" fail. ... no it's green.
hm.
current thought is that this one is just using the default. and inheriting something, but vijay says in irc that aggregator was just not finished. so maybe I was looking in the correct spot.
how would the patch be green?
vijaycs85 says: test doesn't fail, because we don't have a view in core that uses any of those missing plugins
we have covered schema for all in use in core (functionality + tests)
preserve_tags is text? (but strip_tags is bool and false?)
?
?
.?
s?
is this how we escape quotes?
this might be true?
these should be true/false
true?
false?
false?
contains
this is a new file, we could order this group of uses alphabetically
missing docs
check rest of this (new) file
some temporary code to fix or take out
hex and octal are integers?
do we use string_int?
false?
we have a date_format data type. should we use it here?
oh, wait, this is not the value of the format. I think this is the string that is the name of the date format. ok
missing newline
copy paste in the comment?
I dont see hidden here.
I forget. label or text?
there was a core: 8 elsewhere. check.
?
I dont think we put uuids in default config
is this the hidden?
false?
I just read through the patch, I did not try using the config inspector, or saving config.
I stopped at files.views.schema.yml
Comment #45
vijaycs85Comment #46
vijaycs85Adding views plugin schemas (were missing) in below modules:
Missing:
All modules that has views plugin(s) in core(just to make a note):
P.S: There are some test plugins in test modules (under views module). They are not covered here.
Comment #47
Gábor HojtsyThere was a @todo somewhere here to add the exception, that should be removed. Also a @throws doc part should be added to the method.
In other words, why is this not updated?
A little above the changed code. I thought the primary goal of this issue is to remove / implement that @todo (it referenced an old META that was closed down in favor of this one).
Comment #48
vijaycs85Patch with below updates:
Comment #51
vijaycs8548: 2167623-default-config-schema-fix-48.patch queued for re-testing.
Comment #52
vijaycs85#44.1 - Moved to #2172941: Endless looping on Config::castValue() for undefined config data type
#44.2 - Fixed.
#44.3 - Thats type of plugin in config (not config schema). It looks correct.
#44.4 to 7 - Fixed.
#44. 8 - yeah, there was chat with @sun and the bottom line is label should support 3 status (show, hide, invisible- for screen readers). So it is string. cache has multiple options. So it is int.
#44. 9 - New field display plugin for comment module (same way we write views plugin. @see fields.schema.yml for more details).
#44.10 - thats what label says boss ;)
#44.11 - Fixed.
#44.12 - Yes, single quotes to escape. what you are seeing is the worst case scenario of escaping single quotes with single quotes (like \\ in PHP world).
#44.13 - Fixed
#44.14 - Fixed all. So the quickest way to fix any of this config is copy the file to active folder-> reference cache->go to views-> just save without change anything -> copy back from active to module location. As we got the schema covered for them already + check type at save, the get proper value back :)
#44.15 to 17 - Fixed.
#44.18 - Fixed.
#44.19 - Not fixed
#44.20 - Fixed
#44.21 - Fixed
#44.22 - Yep, they are integer and
#44.23 - nope, no type string_int
#44.24 - Yes, inside expose it is boolean (ref: /core/modules/views/config/schema/views.data_types.schema.yml 604)
#44.25 - Yep, OK
#44.26 - Fixed.
#44.27 - Fixed
#44.28 - Updated comment.
#44.29 - Label for text field and text for text area. They are testfield.
#44.30 - Fixed.
#44.31 - Yeah, they are in data_type of fields.
#44.32 - We do almost all default config. Here is the source #1969800: Add UUIDs to default configuration
#44.33 - Yes it is.
#44.34 - Yes it is integer as per (/core/modules/text/lib/Drupal/text/Plugin/Field/FieldType/TextItem.php)
Comment #54
vijaycs8552: 2167623-default-config-schema-fix-52.patch queued for re-testing.
Comment #55
alexpottNeeded a reroll due to #2121849: Move file_chmod_directory and file_chmod_file out of config and into settings so that drush_chmod() is independant of the config system basically all the changes relating chmod configuration have been removed from the patch since this is now in settings.
Comment #56
alexpottThis patch adds schema and configuration storages to access all schema and default configuration regardless of whether or not configuration is enabled. This means that standard profile configuration is now tested :)
Comment #59
vijaycs85Another re-roll...
Comment #60
alexpottAdd again :)
Comment #61
Gábor HojtsyReviewed again :) I really like the structuring of the schemas (eg. separation of views integration into their own schema files). Cool. Some notes:
Docs missing.
Woot :)
The linked issue only covers language.config. Do we have an issue for migrations?
Docs missing, BUT is not the point of this test to test the schema validation code in the runtime system and NOT a parallel schema validation system only in the tests? I see the runtime system is not modified anymore. Hum. This is confusing to me.
I think this was proven above to open more can of worms if solved here, but that issue was closed down as duplicat of this issue. So new issue needs to be opened and referenced here. Also same in other (existing) occurance of the same reference in the code, not just this added one.
Can we summarize on one line and expand on further lines? :)
Not the views module :)
I cannot parse the English in this comment :)
Let's add those newlines.
Extra empty line should not be there.
Why the :? (I assume the quotes were needed due to the : in the name) What that this supposed to mean? Seems to be the only type with a : in it?!
Comment #62
Berdir11. : is the standard plugin derivative separator, I guess it generates one for every entity type.
Comment #63
Gábor Hojtsy11: Yeah, hm, well, it looks odd, but I get what you mean there :/
Comment #64
Gábor HojtsyAlso the config update test module was just removed in #2167109: Remove Variable subsystem so that should be removed from here too. No need to add a schema for it anymore :) => the above patch will not apply anymore.
Comment #65
vijaycs85Thanks for the review @Gábor Hojtsy and @Berdir for the clarification. Adding new patch that address review comments detailed below:
#61:
1. Fixed.
2. alexpott++ :)
3. Added #2183957: Provide configuration schema for Migration module and updated @todo.
4. Fixed doc. Not sure about run time system referenced.
5. Created #2183983: Find hidden configuration schema issues and updated @todo
6. Fixed.
7. Fixed.
8. Fixed.
9. Fixed.
10. Fixed.
11. @Berdir answered in #62
#64:
hope you mean system_test module, but we don't have any change in it and still the patch in #60 applies.
Comment #66
Gábor Hojtsy#2167109: Remove Variable subsystem removed the config_upgrade.module (and associated files). This patch still adds a schema for that (now nonexistent) module :) That's what I meant.
As for the runtime system vs. test, the prior patches used to modify core/lib/Drupal/Core/Config/Config.php to add an unsupported type exception. The current patches don't do that anymore and instead have the casting copy-paste from Config.php in the test and only throw the exception in the test (not in the runtime anymore). That may not be what we want? With this patch applied we have two copies of the casting code and we only test the test copy. If the two become out of sync, our tests would still pass fine even if the runtime stuff does not work anymore as expected. Why was testing the runtime code instead of having our own code copy abandoned?
Comment #67
alexpottWe have to copy the casting functionality because it is protected and it has no business being public - that is not the functionality under test here. We are testing that the config schema and the default configuration match our expectations.
Comment #68
Gábor Hojtsy@alexpott: On the other hand, the runtime may me modified anytime to understand / support different formats in which case the test may fail on new config that is valid or the runtime may fail on old config that still passes the test. Why/how is that not a concern?
Comment #69
Gábor HojtsyAll right, talked through the testing concern that I have on IRC and I now agree this is fine as-is in the patch. Now only the other issue is left :)
"core/modules/system/tests/modules/config_upgrade/config/config_upgrade.test.yml" was removed in #2167109: Remove Variable subsystem whereas "core/modules/system/tests/modules/config_upgrade/config/schema/config_upgrade.schema.yml" (to cover that test yaml) is attempted to be added in this patch.
Comment #70
vijaycs85Thanks @Gábor Hojtsy. Removed it now.
Comment #71
Gábor HojtsyAll righto, looks good to me now :) Thanks for the quick turn-around. Let's get this in ASAP before it gets outdated (anytime :D).
Comment #73
alexpott#2166703: Config SchemaStorage uses InstallStorage landed which means we have to update some of the helper classes.
Setting back to rtbc as the changes are minimal and as Gabor says we need to get this in.
Comment #74
catchDreditor struggles with this so I didn't do a full review, however agreed we absolutely have to get this in - and if there's any regressions the follow-ups are unlikely to be 362kb patches. Committed/pushed to 8.x, thanks!
Comment #75
catchComment #76
webchickGREAT work, all!! This makes me so excited!! :D
Comment #77
yched CreditAttribution: yched commentedFollowup : #2185689: Remove stale entries in field / field instance schemas
The 'widget' entry added to field.instance.* schema is irrelevant.
Comment #78
Wim LeersWOOOHOOO! THANK YOU! This is one of my favorite patches ever :) Thanks vijaycs85, alexpott & others!
Comment #79
Gábor HojtsyYay, thanks! This was a truly herculean effort :)
Comment #80
sunI ran into the following hidden offender while working on a Config FileStorage/SchemaStorage rewrite:
#2197877: Remove empty views.display_extender.schema.yml file
Comment #82
Wim Leers#3427106: Config validation: config entities should get the same validation errors when validated as plain config vs ConfigEntityAdapter is solving a few loose ends that this left behind.