Problem/Motivation
We attempted to introduce configuration schema for all configuration in Drupal core (with exception of some test configuration). We maintained that configuration schemas are optional and our introduction for all of configuration is best practice driven.
However we have been very forgiving for missing or incorrect schemas across the board. We are throwing SchemaIncompleteException when a mapping definition is missing a key but not when any other schema is incomplete or incorrect. Eg. when improper types are used (such as type: array
) or when a dynamic type element has no resolution. And even when we threw exceptions, we swallow them up one level up in the call chain, so they don't end up casing actual failures.
This was all good while building schemas, so we can work while some things are incomplete or incorrect. We should make a stand in handling incomplete/incorrect schemas and either throw exceptions for all error cases and not swallow them up or skip throwing the exceptions because they are useless in practice anyway.
Proposed resolution
Figure out the feasibility of throwing exceptions for all issues. #2268975: Fix named schema elements missing when installing Drupal has examples where views defines data on behalf of disabled modules, so the schema cannot be complete. Those would fail if we just throw fails at all times. We need to figure out how to resolve cases like that.
Consider unifying exceptions as well, the duality of UnsupportedDataTypeConfigException vs. SchemaIncompleteException is confusing...
Remaining tasks
Discuss. Implement. Review. Commit.
User interface changes
None.
API changes
Likely none.
Postponed until
#2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration
#2183983: Find hidden configuration schema issues
Comment | File | Size | Author |
---|---|---|---|
#172 | 2183983-schema-full-force-on-172.patch | 39.15 KB | Gábor Hojtsy |
#172 | interdiff.txt | 5.54 KB | Gábor Hojtsy |
#171 | 2183983-schema-full-force-on-171.patch | 33.61 KB | Wim Leers |
#166 | 2183983-schema-full-force-on.patch | 32.85 KB | Gábor Hojtsy |
#157 | 2183983-config-schema-strict-104.patch | 390 bytes | Gábor Hojtsy |
Comments
Comment #1
BerdirUnpostpone.
Comment #2
vijaycs85Ok, first level of test shows that the problem is installer trying to use the configuration schema of a
disableduninstalled module. Here is an example:views.view.content trying to use the field of type 'content_translation_link' which is part of content_translation module (not enabled by standard profile).
Comment #3
Gábor Hojtsy@vijaycs85: can you post a patch that shows this? :) Also other issues that cropped up with the initial patch?
As per @alexpott, this is due to views having support for handlers for modules that may be disabled (in which case they are ignored). The CMI system does not have a notion of parts of a config file being ignored, so these are at odds.
Comment #4
catchSince modules can no longer be disabled, we no longer need to support that. Uninstall/default config with dependencies on not-yet-installed modules should be covered by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall and related issues. We might need to postpone this on those though.
Comment #5
vijaycs85#4 right, that looks the correct way to me.
Comment #6
Berdir@catch: Nope, that still exists. The handlers are disabled, not the modules. Example is the node admin overview, which has a language field in there that is provided by language.module and only shows up if that is enabled. But if the module isn't there, then the schema for it might not exist.
Comment #7
Gábor HojtsyWe discussed this with @vijaycs85 and @alexpott and figured we can do the test with the full schema available (as is the default config tests is doing it now). If parts of the schema is not available there, then THAT should be a fail. As for having schema for all parts of a config file in *runtime*, that is actually a much bigger undertaking due to the disabled views handler example that Berdir explained.
My understanding is the current test ensures all schema for core config is good but does not make it mandatory for all default config to have schema does it? @vijaycs85?
Comment #8
Gábor HojtsyRepurposed as test coverage for incomplete default configuration. I'm not sure though that there is something to do here? We already fail the test if there was a SchemaIncompleteException or if we did not have $success identifying the type (the union of which cover the missing schema?). The else {} clause still there seems to have no use since it would be equal to the if (!$success) case that immediately follows. @vijaycs85 what do you think? :)
So we seem to be covering this in the tests already?! Let's confirm/refute this first. At least as far as I see we are not as bad as I thought earlier, since we have this covered in the default config tests.
Once that is verified, then the question is whether we want to do it in the runtime as well. So the question is if missing *parts* of the schema is a valid case of missing schema in general, and therefore totally acceptable (schemas being optional). Or if something has a schema on some level, do we require it to have full schema coverage (on the config file level). THEN throwing this exception in the runtime would make sense. And *then* the problem of the optional handlers comes up.
Removing beta blocker and demoting to major as per discussions with Alex Pott.
Comment #9
vijaycs85Yep, else part may not need after the try-catch addition. Lets test this patch...
Comment #10
Gábor HojtsyEven if there is no interest in pursuing this, it would be good to remove that outdated todo then?
Comment #11
xjmSee #2231059: [meta] Find out what config schemas are still missing and add them. There's definitely some stuff that's still not covered. Any insights there would be helpful.
Comment #12
Gábor HojtsyThat @todo is not even there anymore. Moving back to active as we'll still need to throw exceptions at one point if we want to tighten up our enforcement of schema if the top level is defined.
Comment #13
Gábor HojtsyComment #14
Gábor HojtsyComment #15
Gábor HojtsyComment #16
vijaycs85Had to introduces 'dependencies' element introduced by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall. However don't see #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall solves the soft dependency problem explained in #2. This patch has:
1. removes the try catch block to blow up the incomplete exception.
2. Adds 'dependencies' to config entities.
3. Some of the type changes (fixed as part of #2268975: Fix named schema elements missing when installing Drupal) - Just to make the installation pass.
Locally, can see installation failing on installing view with soft dependency on disabled module (translation_link from translation module).
Comment #18
Gábor Hojtsy#2268975: Fix named schema elements missing when installing Drupal landed, this needs a reroll.
Comment #19
vijaycs85Rerolled and updated the dependencies to use config_dependencies data type instead of sequence. Still the install would fail as per #16
Comment #20
Gábor HojtsyWell, the only time that exception is thrown is for elements in a mapping that are undefined in schema.
Comment #21
vijaycs85Yeah, the problem is we don't have schema defined for translation_link at the scope as the module is not enabled? Setting to 'review' to see the exact error.
Comment #23
Gábor HojtsyImproved error reporting so it shows the translation_link stuff explictly. The prior patch failed with The configuration property text doesn't exist. while the updated patch fails with The configuration property translation_link.text doesn't exist.. It would still be important to target the error message better for debugging, but this is already a significant improvement with just a little code change.
Indeed, this seems like to be one of the many cases of default views depending on disabled modules for site building. Now what do we do? Some options:
1. We don't go stricter but somehow make those errors accessible instead of swallowing them for debugging.
2. We do go stricter in which case we need to make schema aware of module dependencies. I don't know how could we do this since the elements are dynamic and apart of mandating the data tells us something about its dependencies, we cannot encode dependency in the type since the problem IS that the type is not available. We cannot really dress up the type with anything because the type is not there. Is there a way to add something to the data or is there already something in the data about the module dependency that we can rely on?
3. Better ideas?
Comment #25
Gábor HojtsyI sufficiently got fed up with the lack of sensible error reporting here that I bought this a few steps further and made error reporting work. This patch now reports the fail exactly as pointing to
views.view.content:display.default.display_options.fields.translation_link.text
MUCH BETTER, right? This should aid debugging a lot.I needed to make instantiations pass on the name which they did not do before. I'm inclined to make the name required, but that needs the argument order changed, so would have wider spanning effects.
Comment #27
Gábor HojtsyRemoved all the actual schema changes, since those are covered in #2273631: Unify config entity schemas with a base schema type. I want to take this issue into a direction where we improve error reporting and report more errors. Maybe not to where it was originally to be more strict but have a strict and a forgiving mode. For now just testing how the patch fairs without the schema changes but WITH the more strict validation.
Imagine interdiff as minus all the config schema file changes :)
Comment #29
Gábor HojtsyMuch better errors "system.date_format.fallback:.dependencies" but the ":." stuff is happening for some reason. I'll keep thinking about where to move this, I left reyero some notes on IRC:
Comment #30
Gábor HojtsyAlso left this to @reyero:
Comment #31
vijaycs85Ok, rerolling with current code base and we 2 problems.
1.Soft dependency:
There is a soft dependency on content_translation. #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall solved the problem on config entities and may need to solve the same problem in config schema as well?
2. Translation sync settings:
if I enable content_translation as part of installation, hitting another problem caused by field.instance.custom_block.basic.body:settings.translation_sync. this will be fixed as part of #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration
Postponing for now.
Comment #32
xjmComment #33
xjmComment #34
Gábor HojtsyLet's run the tests to see where we are and what is still incorrect/missing :) Now that #2183957: Provide configuration schema for Migration module landed.
Comment #35
Gábor HojtsyUploading #31 again to get tested.
Comment #38
Gábor HojtsySchema checking is now in a trait. Rerolled.
Comment #40
Gábor HojtsySomehow the Element.php changes did not end up in the prior patch. Again just a reroll no other changes.
Comment #42
Gábor HojtsyAll right, we are back at
views.view.content:display.default.display_options.fields.translation_link.text does not exist or does not have schema defined
. Our work on #2266427: Add more info on schema validity to configuration inspector for debugging actually reveals a lot more issues and is a better way for now to find these issues. So we can return to this later on if we figure out the optional dependencies.Comment #43
Gábor HojtsyAll right, actually postponing on #2293063: Config schema mapping parsing is inconsistently forgiving, does not conform to the interface now. That is actually surfacing and need to fix lots of mismatches. While the intention there is to make schema more resilient, it is to some degree more strict too :D So may not need this anymore once that is done.
Comment #44
Gábor HojtsyRerolled and removed the menu UI hunk which is covered in #2295737: Not all shipped configuration passes validation even with all modules enabled. I don't think this will fail on anything because the undefined schema elements are now throwing exceptions anymore. If we start throwing exceptions for Undefined elements in general here, that should fail and show holes in schemas :) Those should be only a few as #2295737: Not all shipped configuration passes validation even with all modules enabled and #2301045: Standard profile has views which include elements dependent on uninstalled modules, not valid in config covers most of them.
Comment #45
Gábor HojtsyLet's actually throw exceptions from the creation of undefined. Will still fail due to #2301045: Standard profile has views which include elements dependent on uninstalled modules, not valid in config at least.
Comment #46
Gábor HojtsyErm, the elements are not parsed unless explicitly traversed into. Let's try exceptions in get(), which should be invoked from Config::save() if the parent config has schema.
Comment #47
Gábor HojtsyAnother version where we throw from the castValue().
Comment #49
Gábor HojtsyNo need to check for type, its already undefined, we know :D Now we may get more useful fails.
Comment #51
Gábor HojtsyAll righto, the fun thing is user module ships with a view for who is new. It exposes a block. The user module does not depend on block module. So the schema in user module will not be valid for the view. The view is only ever imported if views is also enabled thanks to config dependencies.
*Just for kicks* (not seriously suggesting though), let's try if user module depended on block module and see what fails it leaves for us. Should be way less. This is repeating most. Ideally the views config entity dependencies should include block module when the view exposes a block, that is not happening for some reason.
Comment #53
Gábor HojtsyMore fails not good ;D Broken out the views dependency problem to #2309247: Views do not depend on modules providing their displays. We may want to try this patch with a good fix from there, once that is settled. In the meantime there are other fails to pick to fix for someone.
Comment #54
catchDo we need an issue to move the who's new block to block module (since user is still required for the moment)? Or alternatively I'd personally be fine with ditching either the block display or the whole view.
Comment #55
Gábor HojtsyIt could be moved to block module but the larger systemic problem is #2309247: Views do not depend on modules providing their displays that needs to be fixed either way, and I think it would fix it for this use case, no? Once the view depends on block module, it would not be imported until block module is available, right?
Comment #56
catchI forgot we'd already done #2082117: Install default config only when the owner and provider modules are both enabled, that should be OK then yes :)
Comment #57
Gábor Hojtsy@catch: maybe not. Looking at that, it just deals on the CMI key level. Not on the entity dependency level. So if module Z provides a config entity that is for a type defined in module Y, if the entity itself has internal dependencies on yet more modules (X and A), the entity will be installed anyway. So looks like an issue is still in order to not install config that depends on modules not installed? Or are modules required to depend on all the modules that they are to provide config for? Like user module would need to depend on block module unless we move the view of course :D?
Comment #58
Gábor Hojtsy@catch see #2309247-14: Views do not depend on modules providing their displays
Comment #61
Gábor HojtsyLet's consider this a "testing" issue of some sort because it seems unlikely we would go stricter :/ Adding more debug data to the fails, so we see what is going on. I identified a few issues based on the debug data, but let's look for bigger patterns. Hopefully we can fix them with mostly systemic fixes. Will fail as much as before.
Comment #63
Gábor HojtsyThe optional history element in the node admin view causes a WHOLE LOT of the fails. Let's try it without that. Since the configs are failing on the first exception found, it may not reduce the number of exceptions dramatically but at least this peals up the first layer. We can still decide later on what to do with this one.
Comment #65
Gábor HojtsyRecent content view has the same timestamp field.
Comment #67
Gábor HojtsyStarted at looking at random ones, but did not notice much of a pattern. There are some overlaps, eg. block plugin related stuff in block module, but once you look closer they are different. So did not find any big patterns that are apparent, just random small bad things everywhere. We need a group of people I guess to divide and conquer these fails because it is not for one person...
Looked at and hopefully solved the following:
- Drupal\block\Tests\BlockLanguageTest: actually used an outdated setting that language block visibility does not support now (uh, did not see where that was removed, sad)
- Drupal\block\Tests\BlockUiTest: uses a test block that has no schema for its setting, while we previously wanted to avoid having schemas for test stuff, it makes it this much harder to see actual fails in tests
- Drupal\block\Tests\Views\DisplayBlockTest: uses a 'field' key in the display instead of where a 'fields' key should be, the test view fields list for the block is broken
These are all very local and pretty different... Unfortunately looks like most others need similar fixes as well.
Comment #70
Gábor HojtsyRetesting now that #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration landed.
Comment #74
BerdirWorking on some fixes here, targetting the fatal errors, with the goal to get proper results back.
Fixed:
- action_bulk_test didn't depend on node, so it was installed first, and then the node views schema didn't exist yet.
- test_field was missing a bunch of schemas. This should fix all the cache tags tests
- ContentTranslationSettingsTest: a bunch of core field types including entity_reference did not have a field type schema in core. Started moving stuff, but handler and so on is module specific, but comment does set it without depending on it. This test is passing now, but only because nobody is setting handler settings.
Not figured out yet completely:
- Ckeditor: Made some fixes that I'm not sure are correct, the plugins are not working somehow.
- WizardTest: views.view.bvhi1jlestmiw8rl:display.default.display_options.access.dependencies. Seems very generic, so not sure why that is not working.
Comment #76
Gábor Hojtsy#2231059-47: [meta] Find out what config schemas are still missing and add them was marked duplicate of this one. Moving up to critical.
Comment #77
Gábor Hojtsy@berdir: do you know which ones are actually *required* to fix the fatals? The ones I added are certainly not, and I suspect some of yours also not? I'd love to open an issue to get just those committed so we can move on with a divide and conquer method here where we use the test code and divide the tests to be fixed among multiple issues. But that would require the base ones fixed that require the tests to function at least.
Comment #78
Gábor HojtsyTalked to @Berdir, he suspects the field settings were the ones, let's try his patch only with the field changes.
Comment #80
catchComment #81
webflo CreditAttribution: webflo commentedComment #82
webflo CreditAttribution: webflo commentedComment #84
Gábor Hojtsy@webflo: why did you include the image style tests?!
Comment #85
alexpottComment #86
Gábor HojtsyAll right, #2361615: Field type config schemas are not in the base schema needs to be resolved so we can continue with these tests here. So postponing on that.
Comment #87
webflo CreditAttribution: webflo commentedFound some schema issues in views in #1948418: Address SA-CONTRIB-2013-035 for views in D8
Comment #88
tstoecklerComment #89
Gábor HojtsyRerolled with the test ^ debug changes only. Now that #2361615: Field type config schemas are not in the base schema landed, this should hopefully generate errors proper. We can try with the fixes then if they still apply / still improve things.
Comment #91
Gábor HojtsyLots of tests are obscured by the extra timestamp fields in the base content views. Removing those should give us better results to check if the rest still applies.
Comment #92
Gábor HojtsyOpened #2368185: Content views shipped by node module have non-functional timestamp field for those two views BTW.
Comment #94
Gábor HojtsyAnother issue opened with common things from this list of fails: #2368349: Entity view and form display configuration schemas are too verbose / key ones missing. This covers tens of fails if not a hundred or so fails in the patch.
Comment #95
Gábor HojtsyPostponing on that one, so we can close a lot of the fails here and then run the test script again.
Comment #96
Gábor HojtsyRolling another version of this to find the next big set of schema issues to fix basically. Because the current fixes are not yet landed, this includes all the schema fixes from #2368349: Entity view and form display configuration schemas are too verbose / key ones missing and #2370305: Refactor field type configuration schemas for DX, easier to find errors. The views patch in #2368185: Content views shipped by node module have non-functional timestamp field landed already, so no need to include that anymore.
Comment #98
BerdirStill a bunch of missing field storage schemas, seeing a bunch of undefined settings keys?
The out of memory errors in the contact tests are scary, look like an endless loop?
Looks like pretty much every single views test is failing (or at least a huge amount of them), so maybe we should start with those? We already have two related issues, one for testing default views configuration and one resaving default views, we could push those, or we could look into the fails separately.
Comment #99
Gábor HojtsyComment #100
xjmComment #101
alexpott#2371843: Add event listener to check schema on config save adds the ability to config saves into a strict schema mode.
Comment #102
jibranRemoving the circular reference.
Comment #103
jibranComment #104
Gábor HojtsyRerolled with just the test for producing our next set of fails to recognise errors. #2368349: Entity view and form display configuration schemas are too verbose / key ones missing landed. #2370305: Refactor field type configuration schemas for DX, easier to find errors is in progress to fix some field schemas and refactor them to fix several of the fails here.
Another variant of this path is attached to turn strict schema matching on by default in all tests. It may help see the full extent of errors but may not give as good errors as my patched debug info provides.
Comment #119
Gábor HojtsyI think once #2370305: Refactor field type configuration schemas for DX, easier to find errors lands, this one will report less or at least very different issues :)
Comment #136
Gábor HojtsyBecause I am impatient and want to get started on the next frontier I rolled versions of 104 with #2325269: Test and fix views in test_views directories against their configuration schema included to test. Let's see.
Comment #140
Gábor HojtsySince I now have good views schema experience thanks to #2325269: Test and fix views in test_views directories against their configuration schema and #2381973: View wizard creates 'invalid' views out of the box, missing plugin_ids, insecure permissions, opened #2385805: Views tests don't pass strict schema checking to fix all of them. These tests show that is a non-trivial portion of the leftover fails. Hope to progress well on those with my existing views experience.
Comment #142
jibranHi @Gábor Hojtsy, how is everything in views schema land? Please summaries your trip in some change notice or blog when you'll finish your trip so that I can fix my views contrib module schema. Good luck and thanks for all the great work.
PS: Please let me know if I can help you in anyway. I know it's quite frustrating task.
Comment #143
xjmComment #144
Gábor Hojtsy@jibran: for the record our conversation about that topic is recorded at the end of the meeting log at https://groups.drupal.org/node/451143
Comment #145
Gábor Hojtsy#2387141-7: Missing field configuration schemas across core tests is now passing so just trying to make sure there are no other field related missing things found here so we can solve them all there. Rolling that into the testing patches.
Comment #148
Gábor HojtsySo these few field fixes bring down fails with static checking:
292 fail(s), and 147 exception(s) to
172 fail(s), and 91 exception(s).
With exception throwing:
266 fail(s), and 124 exception(s) to
138 fail(s), and 66 exception(s)
Woot! Brought some more entity field view mode related fails to #2387141: Missing field configuration schemas across core tests to see how far we can go with these simple fixes.
Comment #149
Gábor HojtsyComment #152
Gábor HojtsyComment #153
Gábor HojtsyReuploading 104 patches again now that #2387141: Missing field configuration schemas across core tests and some others are in.
Comment #156
Gábor HojtsyThat looks really promising. So this is now down to I think three issues:
1. #2358269: Migration bugs in block visibility, field overrides, cron, maintenance settings and form modes found by configuration schema checking has fix, needs review.
2. #2391021: Config schema issues in config tests themselves has test, working on fixes.
3. One issue to be created for the remainder of the issues that will also turn the switch over in all tests (and remove the scattered TRUEs all around). I'll open this once the prior two are done.
I think we are really close :)
Comment #157
Gábor HojtsyComment #160
Gábor HojtsySo resolving the remaining fails is down to #2391021: Config schema issues in config tests themselves and #2391245: Resolve remaining misc issues with configuration schema fails.
Comment #161
Gábor HojtsyOnly #2391245: Resolve remaining misc issues with configuration schema fails left. Let's see if there is anything new since then. Once that is in, we should still keep this issue open to get the strict switch in and remove the then useless multiplications of it :) Then all tests except a very few deliberate ones will be strict schema checking.
Comment #164
Gábor HojtsyWe can come back here for a final patch on THIS issue once #2391245: Resolve remaining misc issues with configuration schema fails is in.
Comment #165
Gábor HojtsyWrote a change notice draft at https://www.drupal.org/node/2391795.
Comment #166
Gábor HojtsyCreated a schema full force patch that will need to be committed here once #2391245: Resolve remaining misc issues with configuration schema fails lands. Once that lands, it will need adjustment to remove more extra flags.
Comment #170
Wim LeersSomehow we lost this hunk along the way in #2391245: Resolve remaining misc issues with configuration schema fails:
That's causing the last exception.
Comment #171
Wim LeersConfirmed suspicion in #170 locally, updated patch attached.
Comment #172
Gábor HojtsyAnd finally we should remove all of the one-off TRUE elements from #2391245: Resolve remaining misc issues with configuration schema fails since we enable it wholesale on TestBase here. And that's it for the remaining scope of this issue. The problem that Wim found in #170 should have been committed on the other issue, so I think he is still eligible to RTBC.
Comment #173
Wim Leers+1
Comment #174
Wim LeersThis is the key change.
EVERY other change, except for the one I had to add in #171, is to remove the override in a zillion test classes from FALSE to TRUE. Previously, tests had to OPT IN to strict config schema testing. Now they have to opt out. Which makes it so that we will continue to have strictly complying config files and schemas… and no longer requiring somebody to spent a month of their life on fixing all the schema issues.
Thank you for your wonderful work here, Gábor :)
Comment #175
webchickAwesome work on this, all!! Greatly looking forward to not spending more time fixing these problems retroactively. ;)
Committed and pushed to 8.0.x. Thanks!
Comment #177
Gábor HojtsyWoot, thanks. Published change notice at https://www.drupal.org/node/2391795
Comment #178
penyaskitoGáborHojtsy++
Comment #179
Gábor HojtsyProvided a cheat sheet at http://hojtsy.hu/blog/2014-dec-12/drupal-8-configuration-schema-cheat-sheet :)
Comment #180
Wim LeersGábor++
Comment #181
webchickDrupal.org please. :)
Comment #182
vijaycs85@webchick, Looks like @Gábor Hojtsy has already drupal.org-ised and updated all relevant documents.
https://www.drupal.org/node/1905070
https://www.drupal.org/node/2391795
Gabor++ * 100
Comment #183
Gábor Hojtsy@webchick: I not only updated the change notice and the schema docs page at the same time as my blog post but I also uploded the file right away to drupal.org to avoid any future sourcing problems. I also updated both immediately when I updated my post with Wim's feedback. I have at least one more round of fixes coming which will again happen in all three places. My blog is the only place to quickly disseminate the news AND take feedback (which is happening in comments there and helps refine the cheat sheet), so it made sense to point people there as the current source and a persistent copy is on drupal.org of the latest version anyway.
Comment #184
Gábor HojtsyUpdated it once more at all 3 copies.
Comment #185
webchickYeah, no problem with it going on hojtsy.hu also, just wanted to make sure it was on d.o. And it is. Hooray!