Find hidden configuration schema issues

Unpostpone.

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

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

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

We 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?

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

Even if there is no interest in pursuing this, it would be good to remove that outdated todo then?

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

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

#2268975: Fix named schema elements missing when installing Drupal landed, this needs a reroll.

Well, the only time that exception is thrown is for elements in a mapping that are undefined in schema.

Improved 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?

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

Removed 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 :)

Much 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:

I wanted to review your config schema sandbox but did not find it on your user profile page… I looked into validation, since I’ve been thinking about it so much and it sucks typed config manager appears to be following typed data, eg. it says it follows TypedDataInterface::validate() but then does not work with constriants at all OTOH ideally we would be able to register errors and warnings (that is two levels of issues) from validation, eg. if a key was defined in schema but not in data, that would be a warning but if a schema is missing for an element or if the element type is different from the schema those would be errors… I think the schema system needs to be resilient enough to not throw up on all kinds of problems but we need a switch where we can tell it to report all the problems so we can test for them and pass on to developers … agree this is best done within the PHP code of the types but we’ll need to add config specific code then to typed data base classes… funky… let’s talk about it when both of us are online and have time

Also left this to @reyero:

btw the issue that made me think was https://drupal.org/node/2183983#comment-8828647 that proves hard failing on schema errors would probably be a mistake but we need a system where the schema types can tell us about the errors and we don’t need to re-implement that outside :) coming around to exactly what you said all week :D

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

Uploading #31 again to get tested.

Schema checking is now in a trait. Rerolled.

Somehow the Element.php changes did not end up in the prior patch. Again just a reroll no other changes.

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

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

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

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

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

Another version where we throw from the castValue().

No need to check for type, its already undefined, we know :D Now we may get more useful fails.

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

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

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

It 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?

I 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 :)

@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?

@catch see #2309247-14: Views do not depend on modules providing their displays

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

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

Recent content view has the same timestamp field.

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

Retesting now that #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration landed.

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

#2231059-47: [meta] Find out what config schemas are still missing and add them was marked duplicate of this one. Moving up to critical.

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

Talked to @Berdir, he suspects the field settings were the ones, let's try his patch only with the field changes.

@webflo: why did you include the image style tests?!

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

Found some schema issues in views in #1948418: Address SA-CONTRIB-2013-035 for views in D8

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

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

Opened #2368185: Content views shipped by node module have non-functional timestamp field for those two views BTW.

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

Postponing on that one, so we can close a lot of the fails here and then run the test script again.

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

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

#2371843: Add event listener to check schema on config save adds the ability to config saves into a strict schema mode.

Removing the circular reference.

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

I 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 :)

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

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

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

@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

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

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

Reuploading 104 patches again now that #2387141: Missing field configuration schemas across core tests and some others are in.

That 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 :)

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

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

We can come back here for a final patch on THIS issue once #2391245: Resolve remaining misc issues with configuration schema fails is in.

Wrote a change notice draft at https://www.drupal.org/node/2391795.

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

Somehow we lost this hunk along the way in #2391245: Resolve remaining misc issues with configuration schema fails:

@@ -130,7 +135,7 @@ function testLoading() {
     $this->resetAll();
     $this->container->get('plugin.manager.ckeditor.plugin')->clearCachedDefinitions();
     $editor_settings = $editor->getSettings();
-    $editor_settings['toolbar']['buttons'][0][] = 'Llama';
+    $editor_settings['toolbar']['rows'][0][0]['items'][] = 'Llama';
     $editor->setSettings($editor_settings);
     $editor->save();
     $this->drupalGet('node/add/article');

That's causing the last exception.

Confirmed suspicion in #170 locally, updated patch attached.

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

The problem that Wim found in #170 should have been committed on the other issue, so I think he is still eligible to RTBC.

+1

+++ b/core/modules/simpletest/src/TestBase.php
@@ -209,7 +209,7 @@
-  protected $strictConfigSchema = FALSE;
+  protected $strictConfigSchema = TRUE;

This 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 :)

Awesome work on this, all!! Greatly looking forward to not spending more time fixing these problems retroactively. ;)

Committed and pushed to 8.0.x. Thanks!

Woot, thanks. Published change notice at https://www.drupal.org/node/2391795

GáborHojtsy++

Provided a cheat sheet at http://hojtsy.hu/blog/2014-dec-12/drupal-8-configuration-schema-cheat-sheet :)

Gábor++

Drupal.org please. :)

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

Updated it once more at all 3 copies.

Yeah, no problem with it going on hojtsy.hu also, just wanted to make sure it was on d.o. And it is. Hooray!