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.
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff.txt | 3.29 KB | Gábor Hojtsy |
#44 | 2264179.44.patch | 19.05 KB | Gábor Hojtsy |
#40 | 2264179.40.patch | 17.84 KB | alexpott |
#35 | interdiff.txt | 1.38 KB | Gábor Hojtsy |
#35 | 2264179-add-property-35.patch | 18.34 KB | Gábor Hojtsy |
Comments
Comment #1
benjy CreditAttribution: benjy commentedComment #2
chx CreditAttribution: chx commentedComment #3
alexpottCan we get a comment for this?
Comment #4
chx CreditAttribution: chx commentedWe 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.
Comment #5
vijaycs85isset() 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).
- 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.
IMO, if property can be anything, then that includes NULL as well.
Comment #6
vijaycs85Updating castValue to stop converting property to string.
Comment #8
vijaycs85Fixing test fails...
Comment #10
vijaycs851 fail is because of:
in core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php:257
Comment #11
vijaycs85Let's fix the test as we don't type cast to string, if no type provided.
Comment #12
alexpottThis looks excellent - with this we can now use config schema to mark something as a property and then have no other opinion about it.
Comment #13
chx CreditAttribution: chx commentedWe 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.
Comment #14
Gábor Hojtsy@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
Comment #15
Gábor HojtsyI 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.
Comment #16
Gábor HojtsyUpdated issue summary/title.
Comment #17
chx CreditAttribution: chx commentedComment #18
Gábor HojtsyHere 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.
Comment #19
alexpottThis seems like a sensible API change.
Comment #20
chx CreditAttribution: chx commentedI am certainly not in the position to approve this (vijaycs85 is, I'd guess) but
looks remarkably sane.
Comment #21
Gábor HojtsyWhile 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? :)
Comment #23
chx CreditAttribution: chx commentedI think Ignore should override validate much like the new Undefined / old Property does and return TRUE;
Comment #24
Gábor Hojtsy@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
Comment #26
Gábor HojtsyThe 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.
Comment #28
Gábor HojtsyThe 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.
Comment #29
Gábor HojtsyUntested 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
Comment #31
Gábor HojtsyThe 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.
Comment #32
Gábor HojtsyThere 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.
Comment #33
vijaycs85Thanks @Gábor Hojtsy. The new type makes it more clear now(Property vs undefined vs ignore). This is good to go.
Comment #34
Gábor Hojtsy@alexpott pointed out we should test type casting also ignores the Ignore type, which I did not add tests for. On that.
Comment #35
Gábor HojtsyTest coverage for saving config with ignore type, to ensure the data is not changed.
Comment #36
vijaycs85Looks good.
Comment #38
Gábor HojtsyI don't think this is related to this patch:
Comment #39
Gábor Hojtsy35: 2264179-add-property-35.patch queued for re-testing.
Comment #40
alexpottRerolled since #2266501: Array level schema errors are not reported, fix wrong schemas identified is in.
Comment #41
vijaycs85As per #36
Comment #42
catchShould 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.
s/uknown/unknown.
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.
Comment #43
Gábor Hojtsy@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?
Comment #44
Gábor Hojtsy@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.
Comment #45
Gábor Hojtsy#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 :)
Comment #46
Gábor HojtsyAlso a blocker for #2183957: Provide configuration schema for Migration module of course.
Comment #47
catchThanks for the extra comments. Committed/pushed to 8.x, thanks!
Comment #49
Gábor HojtsyYay, thanks.
Comment #50
Gábor HojtsyUpdated 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.