Problem/Motivation

1. The checkValue() method in ConfigSchemaTestBase checks for some conditions but misses others. It does not check if an array level element is defined as a sequence or mapping and just goes on to the next level. Therefore missing definitions on this level are not caught. Also it is not reusable as a debugging tool (in config_inspector, see #2266427: Add more info on schema validity to configuration inspector for debugging where we needed to make a copy out of it).

2. StorableConfigBase::castValue() uses similar code but still has a @todo to throw an exception.

Proposed resolution

1. Add testing for the array level so we catch any missing schema there. Move checkValue() to a more generic place so we can reuse it instead of copying it.
2. Add the exception. Solve fallout :D

Remaining tasks

Do it. Review.

User interface changes

None.

API changes

checkValue() should appear as an API addition somewhere.

Files: 
CommentFileSizeAuthor
#31 2266501-complete-missing-schema-31.patch10.51 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,272 pass(es). View
#28 2266501-diff-26-28.txt5.59 KBvijaycs85
#28 2266501-complete-missing-schema-28.patch10.46 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,224 pass(es). View
#26 interdiff.txt462 bytesGábor Hojtsy
#26 complete-schema-test-26.patch8.39 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,240 pass(es). View
#24 complete-schema-test-24.patch8.4 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#24 interdiff.txt1.11 KBGábor Hojtsy
#20 interdiff.txt1.72 KBGábor Hojtsy
#20 complete-schema-test-20.patch8.27 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,183 pass(es), 0 fail(s), and 11 exception(s). View
#18 interdiff.txt1.44 KBGábor Hojtsy
#18 complete-schema-test-18.patch7.16 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,240 pass(es), 0 fail(s), and 32 exception(s). View
#15 interdiff.txt984 bytesGábor Hojtsy
#15 complete-schema-test-15.patch6.72 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,241 pass(es), 0 fail(s), and 228 exception(s). View
#13 interdiff-2266501-10-13.txt1.02 KBYesCT
#13 complete-schema-test-13.patch6.68 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,228 pass(es), 10 fail(s), and 0 exception(s). View
#10 complete-schema-test-10.patch6.69 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,199 pass(es), 10 fail(s), and 0 exception(s). View
#10 interdiff.txt1.76 KBGábor Hojtsy
#8 interdiff.txt2.53 KBGábor Hojtsy
#8 complete-schema-test-8.patch4.93 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,251 pass(es), 14 fail(s), and 0 exception(s). View
#3 interdiff.txt1.26 KBGábor Hojtsy
#3 complete-schema-test-2.patch2.41 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,104 pass(es), 19 fail(s), and 0 exception(s). View
complete-schema-test.patch2.5 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,113 pass(es), 19 fail(s), and 0 exception(s). View

Comments

Gábor Hojtsy’s picture

Priority: Normal » Major

Some notes on what I did there.

  1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
    @@ -110,17 +114,13 @@ protected function checkValue($key, $value) {
    -        else {
    -          // @todo throw an exception due to an incomplete schema. Only possible
    -          //   once https://drupal.org/node/1910624 is complete.
    -        }
    -        $class = get_class($element);
             if (!$success) {
    +          $class = get_class($element);
               $this->fail("{$this->configName}:$key has the wrong schema. Variable type is $type and schema class is $class.");
             }
    

    Note that this @todo can be removed, because AFAIS the !$success already covers this entirely. When in this removed else case, $success was false.

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
    @@ -129,7 +129,18 @@ protected function checkValue($key, $value) {
    +          $this->fail("{$this->configName}:$key is not defined as a mapping or sequence but is not a scalar value.");
    

    There are lots of NOTs here, any feedback welcome on better message.

  3. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
    @@ -129,7 +129,18 @@ protected function checkValue($key, $value) {
    +      catch (SchemaIncompleteException $e) {
    +        $this->fail("{$this->configName}:$key has no schema.");
    +      }
    

    There was no catching this error before. We only caught missing schema for scalars. That means a new index for a mapping that was undefined vs. a mapping entirely undefined looked the same from the errors. Now the entirely undefined mapping throws an error itself.

Gábor Hojtsy’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
@@ -129,7 +129,18 @@ protected function checkValue($key, $value) {
+        if (!($element instanceof Mapping) && !($element instanceof Sequence)) {
+          $this->fail("{$this->configName}:$key is not defined as a mapping or sequence but is not a scalar value.");

We can also test for ArrayElement instead for a more general approach with contrib extensions in mind that are array based. Then the fail should be more general.

Gábor Hojtsy’s picture

FileSize
2.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,104 pass(es), 19 fail(s), and 0 exception(s). View
1.26 KB

More general array test as per #2.

The last submitted patch, complete-schema-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: complete-schema-test-2.patch, failed testing.

Gábor Hojtsy’s picture

Great, the patch now found several undefined group level items that did not have children so were not picked up by the prior test :) All fails seem to be legit and need fixes. Woot!

Gábor Hojtsy’s picture

Title: Config schema testing is incomplete, not reusable » Do complete schema testing, fix missing schemas identified

Retitled so we can fix the ones found here, which makes sense since we have tests to prove they work. :)

Gábor Hojtsy’s picture

FileSize
4.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,251 pass(es), 14 fail(s), and 0 exception(s). View
2.53 KB

Here are some fixes with fail examples:

views.view.who_s_new:display.default.display_options.cache.options has no schema. et. al. indicated display options lack an options key defined. Note that most other keys did not define types either, so I did not define the type here. Since this is an array type, I think we are onto more fails about its type now :D We should figure out whether this is a sequence of some things or a mapping with potential keys...

update_test.settings:xml_map is not defined as an array type (eg. mapping or sequence) but is not a scalar value. and similar are funny; the type was misspelled in the schema :P

views.view.taxonomy_term:display.default.display_options.arguments.term_node_tid_depth.validate_options.bundles is not defined as an array type (eg. mapping or sequence) but is not a scalar value. in this case the bundle was defined as a boolean while the actual value is an array; I *think* this is a sequence of bundle names, so gave that in the schema.

search.page.user_search:configuration is not defined as an array type (eg. mapping or sequence) but is not a scalar value. is a dynamic typed item per plugin type but there was no definition for the user_search plugin (as opposed to the node search one for example); however user search seems to have no configuration options based on the plugin code, so this is an empty mapping

This is IMHO a great sampling of what kind of schema errors are found by this little rounding out of the schema test. There are some other items that I did not yet cover, but wanted to send this set for a round of testing.

Gábor Hojtsy’s picture

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

FileSize
1.76 KB
6.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,199 pass(es), 10 fail(s), and 0 exception(s). View

The other three missing things are good use cases in themselves too:

field.instance.node.article.field_tags:default_value is not defined as an array type (eg. mapping or sequence) but is not a scalar value. as well as the settings both of which are field_type dependent dynamic types. Turns out the tags field on articles has **no field_type** specified in the default config. That is a bit surprising... My active storage copy has it, so I think its somehow fixed in the install process when its resaved... Oh well.

field.instance.user.user.user_picture:default_value is not defined as an array type (eg. mapping or sequence) but is not a scalar value. and same for article image field indicates the image field default value has no typing.

entity.form_display.node.article.default:hidden has no schema. apparently entity form displays got a list of hidden elements that were not documented yet; I *think* this is a sequence of things but not sure of what, need to figure out.

This may still fail on type mismatches as indicated above but at least found all the places where things were misspecified/missing.

The last submitted patch, 8: complete-schema-test-8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: complete-schema-test-10.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
6.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,228 pass(es), 10 fail(s), and 0 exception(s). View
1.02 KB

just comment change and tried to reword that fail message with all the not's

Status: Needs review » Needs work

The last submitted patch, 13: complete-schema-test-13.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
6.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,241 pass(es), 0 fail(s), and 228 exception(s). View
984 bytes

Fixed the two places where I defined the key but not the type although the item was array type. This now should pass but still most of my "fixes" are just fixing the fail itself and not documenting the type proper. Need to figure out the right types more granularly. Anyone up to help? :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: complete-schema-test-15.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,240 pass(es), 0 fail(s), and 32 exception(s). View
1.44 KB

Should now only fail on views. Managed to figure out image field default keys for the mapping and that entity displays hidden components are a string keyed boolean list. Did not manage to track down where the views cache options are stored and how... Seems to be the only one to fix the current scope of this issue.

Status: Needs review » Needs work

The last submitted patch, 18: complete-schema-test-18.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +Configuration schema
FileSize
8.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,183 pass(es), 0 fail(s), and 11 exception(s). View
1.72 KB

All right, the options are already somewhat defined, the empty options need to be defined too though. This will merge type definitions well I think and more in line with how base types vs. actual types are defined in views and elsewhere.

Gábor Hojtsy’s picture

Title: Do complete schema testing, fix missing schemas identified » Array level schema errors are not reported, fix wrong schemas identified

Retitled for array level only not claiming any other ones are fixed here. There are more issues elsewhere.

Status: Needs review » Needs work

The last submitted patch, 20: complete-schema-test-20.patch, failed testing.

Gábor Hojtsy’s picture

Opened #2268439: String introspection backwards data typing is confusing / incorrect as the scalar level counterpart of this one to tighten up schema validation.

Gábor Hojtsy’s picture

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

Was thinking how to define "an empty array that will always be empty" and we don't really have a direct way in schema to define that. So opted to use a sequence with items of type undefined. Since these will never have any items, the undefined type will never be reached but is syntactically required as the definition of schema. I *think* this will finally pass all tests.

I personally verified all items in this schema are correct.

Status: Needs review » Needs work

The last submitted patch, 24: complete-schema-test-24.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
8.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,240 pass(es). View
462 bytes

Definition of type item should be on the right level.

vijaycs85’s picture

Looks nice, few comments:

  1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
    @@ -110,17 +113,13 @@ protected function checkValue($key, $value) {
               if ($type == 'string' && ($element instanceof StringInterface || $element instanceof Property)) {
    
    @@ -129,7 +128,18 @@ protected function checkValue($key, $value) {
    +      catch (SchemaIncompleteException $e) {
    

    Now, we can move this try.. catch block out as it is there in both if & else.

  2. +++ b/core/modules/update/tests/modules/update_test/config/schema/update_test.schema.yml
    @@ -5,19 +5,19 @@ update_test.settings:
    -      type: seqeuence
    ...
    -      type: seqeuence
    ...
    -      type: seqeuence
    

    :( my bad. Thanks for fixing. do we need to add test to find this type of issues?

  3. +++ b/core/modules/views/config/schema/views.argument_validator.schema.yml
    @@ -22,8 +22,11 @@ views.argument_validator_entity:
    +      sequence:
    +        label: 'Bundle'
    +        type: string
    

    may need - at the front?

  4. +++ b/core/modules/views/config/schema/views.cache.schema.yml
    @@ -1,20 +1,40 @@
    +views_cache:
    

    Can be moved to views.data_types.schema.yml

vijaycs85’s picture

FileSize
10.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,224 pass(es). View
5.59 KB

Here is a patch with few more updates with my review fixes:

  1. Removing getInfo() from test base class.
  2. Reg: #27
    #27.1 - Fixed.
    #27.3 - Fixed.
    #27.4 - Fixed.

If this patch is fine, +1 to RTBC as a whole.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

As for #27.2: yeah this test is what you have for now :D Then we should start throwing exceptions from the casting stuff, which will unearth further issues.

RTBC as per review in #27 and #28 :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
10.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,272 pass(es). View

Rerolled. Best to wait for testbot I guess.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c98b2fd and pushed to 8.x. Thanks!

  • Commit c98b2fd on 8.x by alexpott:
    Issue #2266501 by Gábor Hojtsy, vijaycs85, YesCT: Array level schema...
Gábor Hojtsy’s picture

Yay, superb!

Gábor Hojtsy’s picture

All right, now opened #2270815: Make schema testing code available as generic API to abstract this code out. This was part of the original goal but then all the bugs we found made it impractical to do it here.

Status: Fixed » Closed (fixed)

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