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

Files: 
CommentFileSizeAuthor
#172 2183983-schema-full-force-on-172.patch39.15 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,063 pass(es). View
#172 interdiff.txt5.54 KBGábor Hojtsy
#171 2183983-schema-full-force-on-171.patch33.61 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,066 pass(es). View
#166 2183983-schema-full-force-on.patch32.85 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,179 pass(es), 0 fail(s), and 1 exception(s). View
#157 2183983-config-schema-strict-104.patch390 bytesGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,851 pass(es), 18 fail(s), and 25 exception(s). View
#157 2183983-config-schema-exception-104.patch6.57 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,742 pass(es), 22 fail(s), and 21 exception(s). View
#153 2183983-config-schema-exception-104.patch6.57 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,877 pass(es), 74 fail(s), and 24 exception(s). View
#153 2183983-config-schema-strict-104.patch390 bytesGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,729 pass(es), 105 fail(s), and 25 exception(s). View
#149 2183983-config-schema-exception-with-field-fixes-149.patch31.17 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,929 pass(es), 120 fail(s), and 58 exception(s). View
#149 2183983-config-schema-strict-with-field-fixes-149.patch24.98 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,662 pass(es), 159 fail(s), and 81 exception(s). View
#27 2183983-config-schema-exception-27.patch6.09 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#31 2183983-config-schema-exception-31.patch5.67 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183983-config-schema-exception-31.patch. Unable to apply patch. See the log in the details link for more information. View
#35 2183983-config-schema-exception-31.patch5.67 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183983-config-schema-exception-31_0.patch. Unable to apply patch. See the log in the details link for more information. View
#38 2183983-config-schema-exception-38.patch4.08 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#40 2183983-config-schema-exception-40.patch5.96 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#44 2183983-config-schema-exception-44.patch3.63 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,329 pass(es). View
#45 interdiff.txt1.55 KBGábor Hojtsy
#45 2183983-config-schema-exception-45.patch5.18 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,614 pass(es). View
#46 2183983-config-schema-exception-46.patch4.41 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,363 pass(es). View
#46 interdiff.txt1.68 KBGábor Hojtsy
#47 2183983-config-schema-exception-47.patch5.05 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 52,835 pass(es), 518 fail(s), and 38 exception(s). View
#47 interdiff.txt1.73 KBGábor Hojtsy
#49 2183983-config-schema-exception-49.patch5 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 53,921 pass(es), 499 fail(s), and 377 exception(s). View
#49 interdiff.txt987 bytesGábor Hojtsy
#51 2183983-config-schema-exception-51.patch5.74 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 55,712 pass(es), 490 fail(s), and 359 exception(s). View
#51 interdiff.txt758 bytesGábor Hojtsy
#61 interdiff.txt2.07 KBGábor Hojtsy
#61 2183983-config-schema-exception-61.patch6.57 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 55,706 pass(es), 504 fail(s), and 362 exception(s). View
#63 2183983-config-schema-exception-63.patch8.5 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 55,703 pass(es), 505 fail(s), and 364 exception(s). View
#63 interdiff.txt1.93 KBGábor Hojtsy
#65 2183983-config-schema-exception-65.patch10.45 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,011 pass(es), 566 fail(s), and 292 exception(s). View
#65 interdiff.txt1.94 KBGábor Hojtsy
#67 interdiff.txt1.75 KBGábor Hojtsy
#67 2183983-config-schema-exception-67.patch12.14 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#74 2183983-config-schema-exception-74.patch20.23 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,354 pass(es), 625 fail(s), and 264 exception(s). View
#74 2183983-config-schema-exception-74-interdiff.txt8.08 KBBerdir
#78 2183983-config-schema-exception-78-based-on-74-only-field-changes.patch10.95 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 57,635 pass(es), 501 fail(s), and 376 exception(s). View
#81 2183983-config-schema-exception-81-based-on-74-only-field-changes.patch0 byteswebflo
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,993 pass(es). View
#82 2183983-config-schema-exception-81-based-on-74-only-field-changes.patch11.13 KBwebflo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 58,964 pass(es), 496 fail(s), and 391 exception(s). View
#89 2183983-config-schema-exception-89.patch6.57 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 60,928 pass(es), 511 fail(s), and 394 exception(s). View
#91 interdiff.txt3.88 KBGábor Hojtsy
#91 2183983-config-schema-exception-91.patch10.45 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,427 pass(es), 648 fail(s), and 311 exception(s). View
#96 2183983-config-schema-exception-96.patch22.16 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,007 pass(es), 557 fail(s), and 271 exception(s). View
#104 2183983-config-schema-exception-104.patch6.57 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,259 pass(es), 318 fail(s), and 172 exception(s). View
#104 2183983-config-schema-strict-104.patch390 bytesGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,131 pass(es), 292 fail(s), and 147 exception(s). View
#136 2183983-config-schema-exception-136.patch88.29 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,469 pass(es), 266 fail(s), and 124 exception(s). View
#136 2183983-config-schema-strict-136.patch82.1 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183983-config-schema-strict-136.patch. Unable to apply patch. See the log in the details link for more information. View
#145 2183983-config-schema-exception-with-field-fixes-145.patch20.5 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,816 pass(es), 138 fail(s), and 66 exception(s). View
#145 2183983-config-schema-strict-with-field-fixes-145.patch14.31 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,517 pass(es), 172 fail(s), and 91 exception(s). View

Comments

Berdir’s picture

Status: Postponed » Active

Unpostpone.

vijaycs85’s picture

Ok, first level of test shows that the problem is installer trying to use the configuration schema of a disabled uninstalled 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).

Gábor Hojtsy’s picture

Title: Throw exception for incomplete configuration schema. » Throw exception for incomplete configuration schema

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

catch’s picture

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.

vijaycs85’s picture

#4 right, that looks the correct way to me.

Berdir’s picture

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

Gábor Hojtsy’s picture

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?

Gábor Hojtsy’s picture

Title: Throw exception for incomplete configuration schema » Add tests for incomplete schemas of default configuration
Priority: Critical » Major
Status: Postponed » Active
Issue tags: -beta blocker
FileSize
807 bytes
PASSED: [[SimpleTest]]: [MySQL] 63,367 pass(es). View

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.

vijaycs85’s picture

Status: Active » Needs review

The else {} clause still there seems to have no use since it would be equal to the if (!$success) case that immediately follows.

Yep, else part may not need after the try-catch addition. Lets test this patch...

Gábor Hojtsy’s picture

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

xjm’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Active

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.

Gábor Hojtsy’s picture

Title: Add tests for incomplete schemas of default configuration » Be stricter about schema issues once a config has schema
Issue summary: View changes
Related issues: -#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall +#2268975: Fix named schema elements missing when installing Drupal
Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue tags: +sprint
vijaycs85’s picture

Status: Active » Needs review
FileSize
6.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Had 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).

Status: Needs review » Needs work

The last submitted patch, 16: 2183983-config-schema-exception-16.patch, failed testing.

Gábor Hojtsy’s picture

vijaycs85’s picture

FileSize
5.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Rerolled and updated the dependencies to use config_dependencies data type instead of sequence. Still the install would fail as per #16

Gábor Hojtsy’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 19: 2183983-config-schema-exception-19.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
1.21 KB

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?

Status: Needs review » Needs work

The last submitted patch, 23: 2183983-config-schema-exception-23.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.04 KB
10.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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.

Status: Needs review » Needs work

The last submitted patch, 25: 2183983-config-schema-exception-25.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, 27: 2183983-config-schema-exception-27.patch, failed testing.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

vijaycs85’s picture

Status: Needs work » Postponed
FileSize
5.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183983-config-schema-exception-31.patch. Unable to apply patch. See the log in the details link for more information. View

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

xjm’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Status: Postponed » Needs review

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.

Gábor Hojtsy’s picture

FileSize
5.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183983-config-schema-exception-31_0.patch. Unable to apply patch. See the log in the details link for more information. View

Uploading #31 again to get tested.

The last submitted patch, 31: 2183983-config-schema-exception-31.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 35: 2183983-config-schema-exception-31.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Schema checking is now in a trait. Rerolled.

Status: Needs review » Needs work

The last submitted patch, 38: 2183983-config-schema-exception-38.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, 40: 2183983-config-schema-exception-40.patch, failed testing.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Status: Needs work » Postponed

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.

Gábor Hojtsy’s picture

Status: Postponed » Needs review
FileSize
3.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,329 pass(es). View

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.

Gábor Hojtsy’s picture

FileSize
1.55 KB
5.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,614 pass(es). View

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.

Gábor Hojtsy’s picture

FileSize
4.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,363 pass(es). View
1.68 KB

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.

Gábor Hojtsy’s picture

FileSize
5.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 52,835 pass(es), 518 fail(s), and 38 exception(s). View
1.73 KB

Another version where we throw from the castValue().

Status: Needs review » Needs work

The last submitted patch, 47: 2183983-config-schema-exception-47.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 53,921 pass(es), 499 fail(s), and 377 exception(s). View
987 bytes

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

Status: Needs review » Needs work

The last submitted patch, 49: 2183983-config-schema-exception-49.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 55,712 pass(es), 490 fail(s), and 359 exception(s). View
758 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 51: 2183983-config-schema-exception-51.patch, failed testing.

Gábor Hojtsy’s picture

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.

catch’s picture

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.

Gábor Hojtsy’s picture

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?

catch’s picture

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

The last submitted patch, 51: 2183983-config-schema-exception-51.patch, failed testing.

Gábor Hojtsy’s picture

Title: Be stricter about schema issues once a config has schema » Find hiding configuration schema issues
Assigned: Unassigned » Gábor Hojtsy
Status: Needs work » Needs review
FileSize
2.07 KB
6.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 55,706 pass(es), 504 fail(s), and 362 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 61: 2183983-config-schema-exception-61.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
8.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 55,703 pass(es), 505 fail(s), and 364 exception(s). View
1.93 KB

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.

Status: Needs review » Needs work

The last submitted patch, 63: 2183983-config-schema-exception-63.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,011 pass(es), 566 fail(s), and 292 exception(s). View
1.94 KB

Recent content view has the same timestamp field.

Status: Needs review » Needs work

The last submitted patch, 65: 2183983-config-schema-exception-65.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
12.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

Status: Needs review » Needs work

The last submitted patch, 67: 2183983-config-schema-exception-67.patch, failed testing.

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work

The last submitted patch, 67: 2183983-config-schema-exception-67.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 67: 2183983-config-schema-exception-67.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
20.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,354 pass(es), 625 fail(s), and 264 exception(s). View
8.08 KB

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.

Status: Needs review » Needs work

The last submitted patch, 74: 2183983-config-schema-exception-74.patch, failed testing.

Gábor Hojtsy’s picture

Priority: Major » Critical

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 57,635 pass(es), 501 fail(s), and 376 exception(s). View

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

Status: Needs review » Needs work
catch’s picture

Issue tags: +D8 upgrade path
webflo’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,993 pass(es). View
webflo’s picture

FileSize
11.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 58,964 pass(es), 496 fail(s), and 391 exception(s). View

Status: Needs review » Needs work
Gábor Hojtsy’s picture

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

alexpott’s picture

Title: Find hiding configuration schema issues » Find hidden configuration schema issues
Gábor Hojtsy’s picture

Status: Needs work » Postponed

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.

webflo’s picture

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

tstoeckler’s picture

Status: Postponed » Needs work
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 60,928 pass(es), 511 fail(s), and 394 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 89: 2183983-config-schema-exception-89.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.88 KB
10.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,427 pass(es), 648 fail(s), and 311 exception(s). View

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The last submitted patch, 91: 2183983-config-schema-exception-91.patch, failed testing.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Status: Needs work » Postponed

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

Gábor Hojtsy’s picture

Status: Postponed » Needs review
FileSize
22.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,007 pass(es), 557 fail(s), and 271 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 96: 2183983-config-schema-exception-96.patch, failed testing.

Berdir’s picture

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.

Gábor Hojtsy’s picture

Status: Needs work » Postponed
xjm’s picture

alexpott’s picture

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

jibran’s picture

Removing the circular reference.

jibran’s picture

Gábor Hojtsy’s picture

Status: Postponed » Needs review
FileSize
6.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,259 pass(es), 318 fail(s), and 172 exception(s). View
390 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,131 pass(es), 292 fail(s), and 147 exception(s). View

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.

The last submitted patch, 104: 2183983-config-schema-exception-104.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 104: 2183983-config-schema-strict-104.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 104: 2183983-config-schema-strict-104.patch, failed testing.

The last submitted patch, 104: 2183983-config-schema-exception-104.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 104: 2183983-config-schema-exception-104.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 104: 2183983-config-schema-strict-104.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 104: 2183983-config-schema-strict-104.patch, failed testing.

The last submitted patch, 104: 2183983-config-schema-exception-104.patch, failed testing.

Gábor Hojtsy’s picture

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

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 104: 2183983-config-schema-strict-104.patch, failed testing.

The last submitted patch, 104: 2183983-config-schema-exception-104.patch, failed testing.

The last submitted patch, 104: 2183983-config-schema-exception-104.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 104: 2183983-config-schema-strict-104.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 104: 2183983-config-schema-exception-104.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 104: 2183983-config-schema-strict-104.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 104: 2183983-config-schema-exception-104.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 104: 2183983-config-schema-strict-104.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
88.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,469 pass(es), 266 fail(s), and 124 exception(s). View
82.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183983-config-schema-strict-136.patch. Unable to apply patch. See the log in the details link for more information. View

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.

The last submitted patch, 136: 2183983-config-schema-exception-136.patch, failed testing.

The last submitted patch, 104: 2183983-config-schema-strict-104.patch, failed testing.

Gábor Hojtsy’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 136: 2183983-config-schema-strict-136.patch, failed testing.

jibran’s picture

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.

xjm’s picture

Gábor Hojtsy’s picture

@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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,517 pass(es), 172 fail(s), and 91 exception(s). View
20.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,816 pass(es), 138 fail(s), and 66 exception(s). View

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

The last submitted patch, 145: 2183983-config-schema-strict-with-field-fixes-145.patch, failed testing.

The last submitted patch, 145: 2183983-config-schema-exception-with-field-fixes-145.patch, failed testing.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

FileSize
24.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,662 pass(es), 159 fail(s), and 81 exception(s). View
31.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,929 pass(es), 120 fail(s), and 58 exception(s). View

The last submitted patch, 149: 2183983-config-schema-strict-with-field-fixes-149.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 149: 2183983-config-schema-exception-with-field-fixes-149.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +Ghent DA sprint
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
390 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,729 pass(es), 105 fail(s), and 25 exception(s). View
6.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,877 pass(es), 74 fail(s), and 24 exception(s). View

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

The last submitted patch, 153: 2183983-config-schema-strict-104.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 153: 2183983-config-schema-exception-104.patch, failed testing.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,742 pass(es), 22 fail(s), and 21 exception(s). View
390 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,851 pass(es), 18 fail(s), and 25 exception(s). View

The last submitted patch, 157: 2183983-config-schema-exception-104.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 157: 2183983-config-schema-strict-104.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.57 KB
390 bytes

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.

The last submitted patch, 161: 2183983-config-schema-exception-104.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 161: 2183983-config-schema-strict-104.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Postponed

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Postponed » Needs review
FileSize
32.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,179 pass(es), 0 fail(s), and 1 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 166: 2183983-schema-full-force-on.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 166: 2183983-schema-full-force-on.patch, failed testing.

Wim Leers’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
33.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,066 pass(es). View
889 bytes

Confirmed suspicion in #170 locally, updated patch attached.

Gábor Hojtsy’s picture

FileSize
5.54 KB
39.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,063 pass(es). View

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.

Wim Leers’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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 :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • webchick committed 1419122 on 8.0.x
    Issue #2183983 by Gábor Hojtsy, vijaycs85, Berdir, Wim Leers, webflo,...
Gábor Hojtsy’s picture

Issue tags: -sprint

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

penyaskito’s picture

GáborHojtsy++

Gábor Hojtsy’s picture

Wim Leers’s picture

Gábor++

webchick’s picture

Drupal.org please. :)

vijaycs85’s picture

@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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Updated it once more at all 3 copies.

webchick’s picture

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

Status: Fixed » Closed (fixed)

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