Problem/Motivation

A. The property AKA uknown AKA default type needs to be clarified.
B. While the above made it appear Property is the type to use to mark items as 'whatever type' that is not true.

Proposed resolution

A1. core.data_types.schema.yml defines it as both 'undefined' and 'default'. The 'default' type is not used. Should be removed.
A2. ConfigSchemaTestBase::checkValue() refers to Property in inappropriate contexts, inside checking for values implementing PrimitiveInterface while Property does not do that. That reference should be removed.

B1. An explicit 'ignore' type should be introduced to identify when something was explicitly defined as having no discernible type. This is so far only needed for migrations and we are not encouraging people to use that at all in other scenarios although we'll not be able to stop them. It should be explained this is done because migrations are optimized to be written by hand unlike most configuration entities which are saved by code thus have a more rigid schema.

Remaining tasks

Do it.

User interface changes

None.

API changes

- Less redundancy/confusion in the usage of the property type.
- New 'ignore' type.

Files: 
CommentFileSizeAuthor
#44 interdiff.txt3.29 KBGábor Hojtsy
#44 2264179.44.patch19.05 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,693 pass(es). View
#40 2264179.40.patch17.84 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,606 pass(es). View
#35 interdiff.txt1.38 KBGábor Hojtsy
#35 2264179-add-property-35.patch18.34 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,490 pass(es). View
#32 interdiff.txt2.44 KBGábor Hojtsy
#32 2264179-add-property-32.patch17.27 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,284 pass(es). View
#29 2264179-add-property-29.patch17.37 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,259 pass(es), 3 fail(s), and 0 exception(s). View
#29 interdiff.txt4.63 KBGábor Hojtsy
#28 interdiff.txt1.82 KBGábor Hojtsy
#28 2264179-add-property-28.patch13.34 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,255 pass(es). View
#26 interdiff.txt889 bytesGábor Hojtsy
#26 2264179-add-property-26.patch12.47 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#24 interdiff.txt531 bytesGábor Hojtsy
#24 2264179-add-property-24.patch12.27 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#21 interdiff.txt5.33 KBGábor Hojtsy
#21 2264179-add-property-21.patch12.2 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#18 2264179-add-property-18.patch7.69 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,230 pass(es). View
#18 interdiff.txt4.24 KBGábor Hojtsy
#11 2264179-diff-8-11.txt948 bytesvijaycs85
#11 2264179-add-property-11.patch7.24 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,221 pass(es). View
#8 2264179-diff-6-8.txt1.89 KBvijaycs85
#8 2264179-add-property-8.patch6.32 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,622 pass(es), 1 fail(s), and 0 exception(s). View
#6 2264179-diff-0-6.txt3.79 KBvijaycs85
#6 2264179-add-property-6.patch6.24 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
schema.patch3.12 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,386 pass(es). View

Comments

benjy’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
alexpott’s picture

Status: Active » Needs review
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
@@ -92,42 +92,40 @@ public function assertConfigSchema(TypedConfigManagerInterface $typed_config, $c
+    if ($element && $element instanceof Property) {
+      return $value;
+    }

Can we get a comment for this?

chx’s picture

We can but first a) let's get approval that Property is indeed anything goes b) let's clarify NULLs. If only non-NULLs are OK then this code needs to change anyways.

vijaycs85’s picture

Clarify NULL. The test has a comment // Null values are allowed for all types. but Property has return isset($this->value);. Should the latter be return TRUE;, then?

isset() is in validate(). and === doesn't allow FALSE to go through. So we should fine (still I don't see where we are getting it called).

let's get approval that Property is indeed anything goes

- After looking at the history of configuration schema issues (#1648930: Introduce configuration schema and use for translation, #1866610: Introduce Kwalify-inspired schema format for configuration) Property means anything or unknown (initially the class was Undefined.php and renamed to Property.php). so Yes.

let's clarify NULLs. If only non-NULLs are OK then this code needs to change

IMO, if property can be anything, then that includes NULL as well.

vijaycs85’s picture

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

Updating castValue to stop converting property to string.

Status: Needs review » Needs work

The last submitted patch, 6: 2264179-add-property-6.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,622 pass(es), 1 fail(s), and 0 exception(s). View
1.89 KB

Fixing test fails...

Status: Needs review » Needs work

The last submitted patch, 8: 2264179-add-property-8.patch, failed testing.

vijaycs85’s picture

1 fail is because of:

      // If the config schema doesn't have a type it should be casted to string.
      'no_type' => 1,

in core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php:257

vijaycs85’s picture

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

Let's fix the test as we don't type cast to string, if no type provided.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

This looks excellent - with this we can now use config schema to mark something as a property and then have no other opinion about it.

chx’s picture

We could add a drush command that derives a minimal schema from any CMI YAML containing just type: property and label: same-as-the-key , it is much easier to iterate from this schema than writing your own.

Gábor Hojtsy’s picture

@chx: in fact that is a feature we had in mind for config inspector which is the current best place to debug your schemas. It was recently modified to let you know about config files that have no schema, next step would be for it to generate a rough schema. Drush integration is also a great idea. #2266357: Add basic schema autogeneration based on data introspection

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
@@ -92,40 +92,39 @@ public function assertConfigSchema(TypedConfigManagerInterface $typed_config, $c
+    catch (SchemaIncompleteException $e) {
+      if (is_scalar($value) || $value === NULL) {
+        $this->fail("{$this->configName}:$key has no schema.");
+      }
+    }
+    // Do not check value of property, as it is unknown or mixed.
+    if ($element && $element instanceof Property) {
+      return $value;
+    }

I think this has the same problem as I identified in #2266501: Array level schema errors are not reported, fix wrong schemas identified that array level elements will not get reported. Since there are several bugs around those schemas in core now, and I don't think this issue is worth holding up for fixing those, just wanted to note that #2266501: Array level schema errors are not reported, fix wrong schemas identified is intended to fix that. If it lands after this code, then it will adjust.

OTOH using Property as an "anything, do not traverse into" type is IMHO problematic exactly because Property is also the type used as a default fallback in config typing. So this makes it impossible to detect cases where an array type element **should have been** defined vs. where an array element was defined as **please ignore**.

So long as you don't traverse into property elements, that is ignore them if they are not scalars, you'll not find some of the bugs I found in #2266501: Array level schema errors are not reported, fix wrong schemas identified.

I think the only way to reconcile this is to define an ignore type that then does not care about scalar/array types and is never traversed into.

This half-ignore type that was made out of the Property here is not very useful and makes life much harder for debugging where schemas are actually to be provided.

Gábor Hojtsy’s picture

Title: ConfigSchemaTestBase, Property element is never used. » Clarify use of property/undefined and add an ignore type in configuration schema
Issue summary: View changes
Issue tags: +Configuration schema, +Configuration system

Updated issue summary/title.

chx’s picture

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

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
4.24 KB
7.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,230 pass(es). View

Here is a first untested attempt. Please refine :)

Also this changes the core schema system because:

1. It adds a new ignore type.
2. It removes the (I believe unused) 'default' type.

So I think this would be considered beta blocking, but that would require a core committer to classify.

Also needs tests for new type vs. schema validation, etc.

alexpott’s picture

This seems like a sensible API change.

chx’s picture

I am certainly not in the position to approve this (vijaycs85 is, I'd guess) but

-      'no_type' => '1',
+      'no_type' => 1,

looks remarkably sane.

Gábor Hojtsy’s picture

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

While we are at this, there is no point in keeping 'Property' as a class name. It is proven to be only used for undefined types, not extended from, etc. So why not name it 'Undefined' then like it is used. Better DX, less WTF. So after this patch we have Ignore and Undefined "special" types with names that make sense. :) How about that? :)

Status: Needs review » Needs work

The last submitted patch, 21: 2264179-add-property-21.patch, failed testing.

chx’s picture

I think Ignore should override validate much like the new Undefined / old Property does and return TRUE;

Gábor Hojtsy’s picture

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

@chx: yeah I removed the validate() from Undefined/Property because I believe that is pure fake. I don't believe that is being used in any way. The typed data API based tree validator was removed in #1905230: Improve the typed data API usage of configuration schema as it was prematurely added and then the further scope of #1905230: Improve the typed data API usage of configuration schema never materialised since it would have necessitated a rewrite of major parts of config schema with the later changes of typed data. Oh well.

Anyway, looks like I was a bit over-zealous to remove the sequence type altogether, heh :D

Status: Needs review » Needs work

The last submitted patch, 24: 2264179-add-property-24.patch, failed testing.

Gábor Hojtsy’s picture

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

The removal of validate() started surfacing several undefined keys such as node delete/unsticky actions :D I looked into fixing them, but that is truly unrelated, so we better keep property elements valid for now (also we use property as undefined top level items, so we'll keep it valid later on and handle lower level items in our own validation)...

Made Ignore valid at all times based on @chx suggestion from #23.

Status: Needs review » Needs work

The last submitted patch, 26: 2264179-add-property-26.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
13.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,255 pass(es). View
1.82 KB

The actual problem was with the *root type* resolution. This was the only place where the default vs. undefined type was handled differently. Since we handle it the same way everywhere else (eg. it resolved to the same class so we casted or type-checked it the same way, the hasSchema() method handled it the same way, etc), the only difference was the label was 'Unknown' instead of 'Undefined' which is just there to confuse people :D

This installs proper for me. I did not run all the tests obviously :D

So the missing validate() in Undefined and Ignore was not the problem. That said, we could remove the validate()s from Undefined and Ignore, but having them there leaves the theoretical possibility that people can "validate" them as well as lists (mapping/sequence) which delegate validation to their items. This is not really the kind of validation we do with checking/casting, but we keep that around, why not :) Definitely not in the scope of this patch to decide the fate there.

Gábor Hojtsy’s picture

Issue tags: -Needs tests
FileSize
4.63 KB
17.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,259 pass(es), 3 fail(s), and 0 exception(s). View

Untested tests for the ignore type. This should be the complete scope now of this patch. To be reviews and get to RTBC then assuming it will be green :D

Status: Needs review » Needs work

The last submitted patch, 29: 2264179-add-property-29.patch, failed testing.

Gábor Hojtsy’s picture

The fails in #21-#26 inspired me to open #2268975: Fix named schema elements missing when installing Drupal which should fix missing top level schemas at least for the installer at first :) Looking into the actual fails here.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
17.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,284 pass(es). View
2.44 KB

There were two issues with the test:

1. The test data used a different label then what I tested for. That was obvious to fix. Fixed in the data.
2. The internal elements cannot be queries directly like that, we need to get the top level config and then get the item and definition from there.

Also removed the useless assert messages, it is WAY easier to debug without them.

I think this should be green now and fully solves the scope and is test covered. Reviews please.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Gábor Hojtsy. The new type makes it more clear now(Property vs undefined vs ignore). This is good to go.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
Status: Reviewed & tested by the community » Needs work

@alexpott pointed out we should test type casting also ignores the Ignore type, which I did not add tests for. On that.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
18.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,490 pass(es). View
1.38 KB

Test coverage for saving config with ignore type, to ensure the data is not changed.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: 2264179-add-property-35.patch, failed testing.

Gábor Hojtsy’s picture

I don't think this is related to this patch:

Could not open input file: ./core/scripts/run-tests.sh
sh: 1: /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/bin/phpunit: not found
PHP Warning:  Invalid argument supplied for foreach() in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 651
Gábor Hojtsy’s picture

35: 2264179-add-property-35.patch queued for re-testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
17.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,606 pass(es). View
vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

As per #36

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/config/schema/core.data_types.schema.yml
    @@ -18,21 +18,23 @@ uri:
    +
    +# Explicit type when no data typing is possible. Avoid if at all possible.
    +ignore:
    

    Should probably say something like 'Only use this if you are unable to use an explicit type'? The avoid... doesn't explain what the reason would be for using it.

  2. +++ b/core/config/schema/core.data_types.schema.yml
    @@ -18,21 +18,23 @@ uri:
    +
    +# Container data types with lists with known and uknown keys.
     sequence:
    

    s/uknown/unknown.

  3. +++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
    @@ -167,37 +169,34 @@ protected function validateValue($key, $value) {
    +    catch (SchemaIncompleteException $e) {
    +      // @todo throw an exception due to an incomplete schema.
    +      // Fix as part of https://drupal.org/node/2183983.
    +    }
    +    // Do not cast value if it is unknown or defined to be ignored.
    +    if ($element && ($element instanceof Undefined || $element instanceof Ignore)) {
    +      return $value;
    +    }
    

    If the schema is complete, how do you end up with something undefined? I'm a bit confused how those two can co-exist. If that's a valid condition even with the @todo resolved it could use a more extensive comment.

Gábor Hojtsy’s picture

@catch:

#42/1: I don't think we want to encourage the use of ignore for any case, we want to encourage coming up with configuration formats that CAN be described with a schema in favor of those that cannot. Migrations don't want to make their configuration format schema compatible with a claim it would make hand-writing prohibitively more complex, which is the sole reason we are introducing this type. We can say eg. "Instead of using this type, we strongly suggest you use configuration structures that can be described with other structural elements of schema". Or something along those types. Which is a more elaborate way to say "avoid if at all possible" :D

#42/3: That a top level schema exists, which is what hasConfigSchema ensures (nothing more), does not mean all levels in the file have schema. So it may happen at any level that schema is missing, which is when we assign the Unknown type since we don't have a better idea. As for the SchemaIncompleteException business, the only situation we throw a SchemaIncompleteException ever is from mappings where a key attempted to be accessed did not exist in the parsed definition. That may be due to the key not even being in the data or the key not being defined in the schema. We do not throw that exception when a dynamic data type is not found or if you use a random name for a type that is not even defined... All those other cases end up in Unknown type tough. That needs to be cleaned up wholesale and not in scope here IMHO. This code is just moved in the patch and is not changed/updated in any way. Not sure how can it be made better, we can document the crappy in-between way of it now, but not sure how helpful that is?

Gábor Hojtsy’s picture

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

@catch: Rerolled with the suggested fixes. Changes are only in docs and a minor bit of schema type reorganisation so the file is better structured, so back to RTBC. Added a link to the schema docs page, so we can elaborate on any recommendations further there.

I also overhauled the issue summary of #2183983: Find hidden configuration schema issues which is the issue referenced in the @todo. We ran into limitations of possibly throwing that exception #2268975: Fix named schema elements missing when installing Drupal.

Gábor Hojtsy’s picture

Issue tags: +blocker

#2270815: Make schema testing code available as generic API is postponed on this one and that would allow my building of the debug tool for config schemas at #2266427: Add more info on schema validity to configuration inspector for debugging, so marking as a blocker if that means anything at this point :)

Gábor Hojtsy’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the extra comments. Committed/pushed to 8.x, thanks!

  • Commit 4451ee6 on 8.x by catch:
    Issue #2264179 by Gábor Hojtsy, vijaycs85, alexpott, benjy: Clarify use...
Gábor Hojtsy’s picture

Yay, thanks.

Gábor Hojtsy’s picture

Updated the docs for the new ignore type as well as the undefined type. (Also fixed a whole lot of issues with the schema docs where it became stale recently): https://drupal.org/node/1905070/revisions/view/7257181/7265669

Also added change notice at https://drupal.org/node/2271603.

Status: Fixed » Closed (fixed)

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