Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #10
Problem/Motivation
- Sequence is a bit of a weird type data construct. It implements neither
ListInterface
norComplexDataInterface
but it allows arbitrary keys - The former makes it currently impossible to validate using the typed data validation API
Proposed resolution
- Let ArrayElement implement ComplexDataInterface. This allows you to fetch a specific item from this array.
- Therefore let both Mapping and Sequence implement ComplexDataInterface. Both conceptually allow (arbitrary) keys and are complex by that definition
- Fix validation for Typed config elements, reusing (and maybe extending) TypedData validation
- Prove that validation of typed config works with the adaption of sequence
Comment | File | Size | Author |
---|---|---|---|
#102 | interdiff-1928868.txt | 4.1 KB | dawehner |
#102 | 1928868-102.patch | 30.26 KB | dawehner |
#99 | interdiff-1928868.txt | 980 bytes | dawehner |
#99 | 1928868-99.patch | 28.24 KB | dawehner |
#97 | interdiff-1928868.txt | 4.24 KB | dawehner |
Comments
Comment #1
fagoOh, here a first start not doing much yet.
Comment #2
Gábor HojtsyFirst off, great cleanup proposals :)
As for the patch, the use of 'any' is interesting. Is that introduced in #1913328: Provide general list and map classes? The 'undefined' type is in system.data_types.schema.yml so if we don't use it, it should be removed from there too. Also, there is test coverage for undefined types since #1905230: Improve the typed data API usage of configuration schema so those tests will need to be updated.
As for the proposals, I'm a bit afraid if this one:
Given the extent of custom types there will need to be, especially around views, I think they would likely look very odd to show up as high level Typed Data elements.
Comment #3
fagoCreated #1928938: Improve typed data definitions of lists
Comment #4
Gábor HojtsyCrosspost?
Comment #5
fagoad crosspost:
Indeed - sry (I should wait for results and check them ;-)
For having dynamic references like
views.display.[%parent.display_plugin]
to be valid typed data I'd do the same as we'd do for bundle-specific entity-references - have a generic reference in the metadata, but more metadata once metadata is retrieved from the object (which has contextual information).For example,
views.display.[%parent.display_plugin]
seems to be a display plugin, so its general type should be "display_plugin" or whatever the valid type for it is. Then, in getPropertyDefinitions() of the views.display object we have more contextual information and can return a more detailled type depending on the metadata of the "views" object or the "display" object - so it knows the display object is of type "display_plugin:foo" and can use that to sayviews.display.[%parent.display_plugin]
is of type "display_plugin:foo".While my example was towards specifying more detailed types, you obviously can use the same approach for generally adding more metadata depending based upon contextual metadata, e.g. adding additional constraints, additional settings, ... However, the metadata returned shouldn't be conflicting, e.g. it would not make sense to have the type be 'entity' without context and with context it's suddenly 'string'.
Comment #6
Gábor HojtsyYeah that sounds like it would complicate the schema definition, since you'd need to define the more generic type and the inherited sub-types somehow(?)
Comment #7
fagoYeah, inherited sub-types would be computed by the classes only. I think the schema definitions could stay as they are as they make it possible to define that without declaring classes - but what gets exposed as typed-data should probably follow the pattern outlined - e.g. create it with the parent type and compute better metadata later on. Question is whether a suiting parent type could be determined from the pattern?
edit: If the pattern is already resolved when config-schema gets processed to typed data definitions we have no problem at all anyway.
Comment #8
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
The summary may need to be updated with information from comments.
Comment #9
Gábor HojtsyMore concrete code based suggestions would be great! Also given the limited interest in the internals of the schema system as well as our other high priorities that are actually user facing, I'm not sure we (as in people working with the D8MI core team) will be able to get to this before July 1st and it is not possible to do much about this afterwards.
Comment #10
Gábor HojtsyFYI unless Jose has time/inclination for this next week in Dublin, I don't see any way of this happening in Drupal 8. There is an 11 day window where such changes are accepted in core, and there is no actual patch yet.
Comment #10.0
Malerio CreditAttribution: Malerio commentedupdate summary to template and with comment.
Comment #11
dixon_I might have a look at this, because while doing #2002138: Use an adapter for supporting typed data on ContentEntities I ran into problems trying to untie config schema from
TypedDataInterface
.@Gabor It's definitely very late to do this, but I see this as clean-up and/or conversion to the latest incarnation of the Typed Data API. While there might be minor changes to how you'd extend config schema implementations (if someone ever will) there will be no changes to how they work for module developers that wants to implement a schema for their config stuff. So I'm hopeful we'll get this in.
Not assigning to myself yet. It might take a couple of days until I find time to put aside for this.
Comment #12
dlu CreditAttribution: dlu commentedMoved to configuration system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).
Comment #13
xjmIs this even still an issue following #2002138: Use an adapter for supporting typed data on ContentEntities? Tentatively demoting to normal and setting postponed for more info.
Comment #14
Gábor HojtsyWell, there would be many things to reconcile. Typed data has map and list types now, which it did not have back at the time. Schemas have a parallel implementation, they have mappings and sequences. There are also some concerns about different validation implementations, etc. There is nothing in this code that makes it not work, its just several things implemented custom.
Comment #15
Gábor Hojtsy@xjm asked me to clarify. Config schemas reuse typed data classes for strings, integer, and so on. At the time config schema was written, there was no list and associative mapping type in typed data, so schemas have their own. Then later on these were introduced in typed data, but the custom schema implementations for these types have not been removed. That is at least one possibility where this issue would be valid.
I think @fago's point of view is that since the derivative types (basically all configuration schema definitions in core) are not valid typed data types, the use of typed data in this system is more smoke and mirrors than useful.
Comment #15.0
Gábor Hojtsychange dot list
Comment #16
Gábor HojtsyUnpostponing as per above.
Comment #17
Jose Reyero CreditAttribution: Jose Reyero commentedYes, I think this makes sense, and I'm planning to work on this task provided we agree on how to do it, one step at a time. This is my take on how to make typed data and typed config more consistent:
These are the trivial changes:
- 1.1 (WIP) #2271967: Fix and use TypedConfigManagerInterface/TypedConfigManager
- 1.2 Use DataDefinition objects instead of arrays for typed config
- 1.3 Add a TypedDataManagerInterface so TypedDataManager can be extended and stuff depending on it reused. (Please let validation out of that for it not to bee too complex)
A bit more complex ones:
- Reuse some new data types form typed data: ItemList, Map, Any.
- Have TypedConfigManager extending TypedDataManager or at least implementing a common ancestor / interface (It basically needs to override discovery, and maybe validation)
Other (harder) issues:
- Discovery. Typed data depends on annotations while typed config is extendable through the schema. This is why we cannot just reuse TypedDataManager to produce typed config objects.
- Validation. Dependency Injection in badly broken in TypedData when using validations. So it cannot be reused for other stuff and we should fix this before any other movement. See:
So I'm not sure we can -nor we want- really merge both but we could really make some progress about consistency. Also I'm not doing gigantic D8 patches anymore so this would need to be one step at a time. Waiting for feedback on this one.
Comment #18
fagoAny custom validation logic should not go into validate(), but be provided by a validation constraint plugin. Howsoever, I don't think those can be already dependeny injected, but this is where it we should make it work for validation then.
Data types are provided via the plugin system, so one can implement a derive to provide back multiple code-discovered data types.
Then I think we should look into providing proper data types for all data types used by typed config, and either contribute them directly to typed-data or just have a "config_foo" type registered. When parsing the schema "foo" types could be converted to "config_foo" then.
Comment #19
Jose Reyero CreditAttribution: Jose Reyero commented@fago,
You're right about discovery, we could extend that, though I feel it's too late for big refactorings in D8, isn't it?
About Typed Data, if we want to make it fully reusable, we shouldn't allow into it such specific dependencies (TypedConfigManager). So if we cannot make it work with DIC, maybe we should just drop that 'validate' methods... Really not much intereset in reworking schema elements into something that at the end cannot use the full features of TypedData...
Anyway, I'm planning to work on the 'trivial changes' mentioned in #17, but I don't think much more for D8 core...
Comment #20
fagoIf it's just moving around internal stuff and does not impact the API average module developers would use, i.e. config schema, I do not think it's a problem.
Comment #21
Jose Reyero CreditAttribution: Jose Reyero commented@fago,
It's not really the code the scaring part.
Do you really think we can easily add something like "plugin definitions in YAML files"? (Or "Annotations" overridden by "Configuration"), that is what it actually is.
Really, it may not be that hard on the technical side, but I don't have the time to follow up on the possible discussion that may trigger.
Comment #22
Jose Reyero CreditAttribution: Jose Reyero commentedHey, I've just found YamlDiscovery and YamlDiscoveryDecorator, so forget my last words, we may not be that far after all :-)
Comment #23
Gábor HojtsyI've been thinking about validation a lot in #2183983: Find hidden configuration schema issues, see from comment #29 especially. In short, schema mistakes are so easy to make (and dependent data with non-installed modules is possible) that its near impossible to require 100% valid schemas. So we need a resilient type system (which we have now), but we also need a way to collect all errors in a distributed way from the tree from the types (which we don't have now). The errors mappings found are swallowed up and the errors we find in tests / casting are due to a parallel custom implemented outside system (in fact two). Also we would need several error levels, which may or may not be why I think Jose wants to inject the constraints. We need to be able to have errors and warnings at least. Examples of error: data has a key but schema is not defined, data has a key but schema does not match (eg. integer instead of list). Example of warning: schema has a key that data does not contain.
In short, we would need to somehow find a way to delegate the validation to the elements. But that would mean some typed config specific validation in typed data classes which does not sound very good.... Not sure how to best do this... See the linked issue where we need typed config specific error reporting about the nested key name for example.
Comment #24
Jose Reyero CreditAttribution: Jose Reyero commented@Gábor,
I think if we move this one a little bit (make typed config valid typed data) we may be able to use the TypedData validation.
Btw, I have some patch almost ready, kind of TypedConfigManager extends TypedDataManager, just waiting for #2271967: Fix and use TypedConfigManagerInterface/TypedConfigManager
Comment #25
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated summary with proposed resolution.
Comment #26
Jose Reyero CreditAttribution: Jose Reyero commentedThe next patch should be a big step ahead towards consistency: #2277945: Typed configuration implements TypedDataInterface improperly
Comment #27
fagoTyped data validation does not feature different error levels, but else this seems to be doable to me. You could easily add in respective default constraints for config related data types. When/where would the warnings be displayed in contrast to the errors?
I'm not sure that would be really needed, but I do know the config schema to less. I mean there could be data types for the primary config schema types, while there could be one or two "extensible" data types, like the typed data "map" plus an additional setting from where to retrieve the map definition.
Comment #28
fagoLooking at core.data_types.schema.yml, I think technically it would work nicely if the types having classes would be proper data types as used by the typed data API, while others could be all a config_mapping type plus a setting pointing to the mapping definition, e.g. "theme_settings".
However, the problem I'd see with that is that the typed config system would have to know whether a type is a data type in the TypedData API sense or a pointer to another mapping definition to prevent clashes in type names. To fix that, it would probably require a config schema change, e.g.
-> something like
Comment #29
Jose Reyero CreditAttribution: Jose Reyero commentedI'd say 'warnings' and 'errors' are a more 'UI' kind of concept. I mean typed data validation just gets you the constraint violations, right? Then it should be up to the invoking code to decide whether that violations are a warning or an error that may differ deppending on the context on which it is invoked.
For config validation, possibly, all TypedData constraint violations should be errors, while other more specific constraints introduced by the config system itself may produce either warnings or errors.
The question: Is there any way to get the Constraint object from the ConstraintViolation or should we be using ConstraintViolation:$code to add some logic which decides the output (warning or error) depending on the kind of constraint violation?
About specific config schema data types, and consolidating them with typed data plugins I think we should be opening a different thread for that. Anyway you are right raising the issue of how we use 'type' in configuration schema because that is a problematic concept.
As I see it, schema extension types may still be valid typed data types (though they are defined in the schema as opposed to annotations) but maybe we could replace the 'type' property by something like 'extends',
Comment #30
Gábor Hojtsy@fago: so far typed config is resilient to all kinds of issues, so if an element has no data defined, it will not bother with it. That would be a warning but not a runtime showstopper. Runtime showstoppers include when an element has a type of int and is a list of strings instead. The former would be displayed in debugging contexts, tests, etc. the later would probably halt some operations with typed config because it does not make sense to attempt to work with such combinations.
Comment #31
Jose Reyero CreditAttribution: Jose Reyero commentedComment #32
Jose Reyero CreditAttribution: Jose Reyero commentedComment #33
Jose Reyero CreditAttribution: Jose Reyero commentedPlease take a look at #2300829: Clean up LocaleConfigManager / TypedConfigManager features and cache duplications
Comment #34
mgiffordComment #35
Gábor HojtsyAny actionable subissues here to work on? :) All the child issues are now resolved :)
Comment #36
Jose Reyero CreditAttribution: Jose Reyero commentedI think most of it is done, though I still see two actionables here, from the issue description
- Add a TypedDataManagerInterface in order to be able to pass around either TypedConfgManager or TypedDataManager. That one should be extended by TypedConfigManagerInterface
- Fix validation for Typed config elements, reusing (and maybe extending) TypedData validation
Not sure we still want the second one, since config validation has gone its own way, though I think the first one may be worth trying and easy enough.
Comment #37
Gábor HojtsyJose: would be great if you can help with that one then :) Thanks for the summary!
Comment #38
Jose Reyero CreditAttribution: Jose Reyero commentedPatch for TypedDataManagerInterface here, #2488568: Add a TypedDataManagerInterface and use it for typed parameters
Comment #39
XanoTypedDataInterface
references this issue. Its::createInstance()
method takes a data definition as its first parameter and the docblock says the parameter is\Drupal\Core\TypedData\DataDefinitionInterface
, but the parameter itself is not type hinted. What exactly blocks the introduction of a type hint there?Comment #40
Gábor HojtsyI don't believe anything blocks that.
Comment #41
xjmAt this point we would need to postpone improving this validation to be consistent to 8.1.x or later.
Comment #42
xjmAdding several related issues about validation. Some are about validating imports whereas others are about validating form submissions or CRUD through the API/REST/etc. We might want to make an overall plan to sort out the big picture.
Comment #43
dawehnerLet's unpostpone that, getting the ball rolling here isn't a bad idea
a) Make typed config type data aware of the needed objects
b) Create the data definitions with the used typed data manager
c) Add an example constraint to the schema.
d) Make mapping and sequence complex data and list (ideally those two classes though don't exist in the first place)
With that traversed and top level constraints work basically.
Comment #44
dawehnerIt kinda works in theory, but the simplicity of
\Drupal\Core\Config\Schema\Mapping::set
causes some issues when you want to change some data.Comment #46
fagoI took a look at what's necessary to get this fixed. I think first of, we need to ensure the Typed Data API works - then validation should work as well. Thus, I added some test-coverage for that. Seems like we need to pass through metadata first. Mapping::getElementDefinition() and getProperties() have that already, so that should be doable just fine.
I'm not so sure about Sequence::getElementDefinition() as has a $key parameter. When I understand correctly, the schema of a sequence item might vary based upon the value (thanks to replacement properties). However, we'd need a generic schema for all items here. Not sure we have one though?
That said, sequences are problematic in general as it does not really map to lists. Lists in typed data are un-keyed, but sequences aren't. This makes me think a sequences should be just complex data as well - it's complex data where we do not know the keys up-front, but that should be ok.
Attached patch adds some more test-coverage (still with lists) and removes the changes related to injected typed data manager as those lead to exceptions for me (this is hard enough already, not? ;).
ad:
I fear this would be an unacceptable API change for d8, thus this needs to wait until d9 now.
Comment #48
Gábor Hojtsy@fago: thanks for looking into this. It would be nice to find some backwards compatible way to solve this if at possible in Drupal 8. There is no requirement for API signatures to look ideal ;)
Re the sequence typing, we do have a "type template" so to speak for items in a sequence in general, if there are no dynamic parts to it then its even a concrete type, but often it is just a template
Comment #49
fagoRight. Not ideal either, but fine ;)
Yep, but if I understand correctly the "type template" does not come with any metadata attached, right?
Related, I noted some missing docs while doing the test coverage. Opened #2677672: Improve docs of getDataDefinition for lists and complex data
Comment #50
Gábor HojtsyYeah there is no metadata, there is only the type template information and the data that decides on which type the template resolves to.
Comment #51
fagook, worked a bit more on this. Good news is I think this is fixable quite fine, bad news is that this would require some small API changes. The API changes are small and would only affect people who would use the typed config objects based upon the Typed Data API interface, so I do not think they are problematic but helpful - i.e. it's API correction.
Issues:
Typed config data definitions are passing through the configuration data type into DataDefinition, i.e. DataDefintion->getDataType() tells you 'string', 'text' or 'config_object'. That's actually a bug as those data type are no TypedData data types, so it's wrong. We could try to register all those config data types, but that would lead to tons of conflicts and tons of unneeded registered data types in the system. Instead, I'd suggest to register on 'config' data type, which has the configuration type's definition array below settings. That means, the config data type goes from $config_element->getDataDefiniton->getDataType() to $config_element->getDataDefiniton->getSetting('type'). I found only one internal call to this, so I doubt anyone is using that. But that's the most problematic change imo.
Sequence implements ListInterface, although it doesn't make sense as it has keys. As there are quite some methods around it also I kept that for BC and made it implement ComplexDataInterface in addition - what makes sense. It's a bit confusing that it has both, but I documented that ListInterface is only there for BC.
Given the potential advantages of this (tokens of config, REST and normalizers for config, better form generation for config (we are working on typed data widgets for Rules) I think this small API correction is totally worth it.
In order to highlight that the API-change is more a correction, I re-categorize this as bug.
WIP patch attached, but I'd love to get feedback!
Comment #52
fagoComment #54
dawehnerIt is great that someone with actual knowledge about that stuff works on this!
I'm wondering whether its okay to skip notification with the point of simply not needing it for config in general, because its more simple?
For sequences this should be sort be able to figure out, right?
It is a bit odd that the type config manager would have this responsibility.
unneeded, IMHO
Comment #58
tstoecklerI would need to look at some of the code in context, before giving a more detailed review, but here are some thoughts from just looking at the patch:
Seems like you've conceptually renamed
Element
toConfig
and made it a proper data type plugin. If so, we should properly deprecateElement
.Doesn't seem to be used. Also elsewhere in the patch...
Yeah, implementing
ListInterface
really is completely wrong forSequence
. So why are you introducing that in the first place?This makes the current patch almost won't fix to me.
ComplexDataInterface
andListInterface
simply different semantics, and we don't share the same semantics in config land. That's why I introducedTraversableTypedDataInterface
in the first place. So we should be improving that IMO, and not reverting back to our previous mistakes.I generally hate that key with a passion, but it's A) nice to be able to just use that here and have it work and B) that entities then are no longer the only example of this in core. That makes it an actual (albeit weird) API. So kudos, for bringing that into the mix for typed config, I would have never thought of that!
Wow, this would really be a game changer.
Comment #59
dawehnerAfter reading the response from tstoeckler I started to think a bit more about it again. As he wrote, sequence is not a list, but its a traversable, so why not let Sequence implement that.
I don't know whether this patch is in scope of this issue, but it solves the problem of config validation. I added test coverage for every level.
Some feedback would be great!
Comment #61
dawehnerSeriously testbot. I opened #2755069: Check in \Drupal\Tests\Listeners\DrupalStandardsListener that each test class has a @group annotation for the missing
@group
statement.Comment #62
Wim LeersThis looks like it probably makes sense in the world of Typed Data. Sadly, I don't know anything about Typed Data, so I lack the necessary context to provide a review that's actually helpful. Sorry.
I APPROVE
(despite the 'meh', which we will need to discuss in person at some point)
Comment #64
tstoecklerAgain, will have to study the surrounding code to make a proper assessment of the patch. But from looking at the patch, this is a lot closer to what I was thinking, thanks! Would be great to hear @fago's thoughts about this.
Wow, that's terrible, but totally not your fault. See #2420067: Promote usage of TraversableTypedDataInterface for cleaning that up.
Comment #65
dawehnerThank you for your feedback @Wim Leers and @tstoeckler!
Yeah, it seems to make more sense.
I'm glad you haven't seen this interdiff yet.
Comment #67
dawehnerThis addresses just the test failures.
Comment #69
dawehnerSome more fixes
Comment #71
dawehnerFixed the remaining failures
Comment #72
fagoThanks for working on this + great to see a green patch! It looks already pretty good.
yes, that seems reasonable. Seems silly to take care of it when we do not need it.
Here my review:
Missing docblock
hm, I'm so sure about that. It's true that there is no clear fit for lists or complex data, but having neither drastically limits the usefulness of the implementation. Code operating with the data based on the typed data API, would simply not deal with it. :/ For example, the serialisation module would simply ignore it I assume
The best they could do with, would be a list. But then, we cannot make it a list without loosing the keys.
I think we could make it ComplexData, but without having any pre-defined property names ,it does not really meet the expectations here either. Seems like we miss a way to properly denote that this is keyed, but ordered collection.
Given __sleep() is already implemented, this seems to be unused?
Beginning of the sentence seems to be flawed? At least I cannot parse it :)
Comment #73
fagoAfter wrapping my head on the whole data structures topic again for a while, I think that sequences should be implemented as complex data having an implementation respecting order. See https://www.drupal.org/node/2420067#comment-11406345. Best, we clarify the unclear points around that in some separate issues also, but that should not hold this issue up.
Comment #74
fagook, worked on this a bit more. Attached patch
- converts sequecnes to complex data (see comment above)
- undo some now uncessary changes
- clean up some additional added test exceptions
- improve test coverage to add some generic typed data api usage docs.
What's not covered still:
- Ensuring there is metadata for contained properties etc. available. This is a detail and is best covered in a follow-up imo.
- Addressing the "data type issue" as described in #51
Comment #76
fagoComment #77
fagoelaborating on #51:
This is bit hairy. The TypedDataInterface does actually not say that an object has to work with data types registered in the typed data manager service only. But as TypedConfig has its own manager, it makes sense to have it's own type system also. So this seems to be actually valid, but what are the consequences?
- When Typed-Config is exposed to some APIs expecting typed data, those APIs will use a different manager and fail to look up some types. It's not totally obvious that some data might have types, that are not registered. But as no one actually guarantees that this is the case it seems ok and that code would have to catch those cases.
- Still, code working based on the Typed Data API can reasonably work with Typed Config as is, as with the changes of this patch it implements the interfaces correctly and data objects implement the respective interfaces for primitives like string, integer or complex data.
- Validation, as tested in this patch, already shows that the Typed Data implementation of Typed Config can be very useful, and I think other stuff like serialization or the token replacements API would work just fine with it also.
Given that, I think we should keep the current behaviour and have the Typed Config use different data types as regular Typed Data.
That said, I think the patch is ready once it's green again. (Update: Green :)
Comment #78
dawehnerThat is super weird ... for me a sequence doesn't have a main property name or could ever have one. Are we sure we don't want to introduce its own one?
... but well,
MapDataDefinition
provides a minimal implementation ofComplexDataDefinitionInterface
Oh wow I didn't know config could potentially provide computed properties :) I mean conceptually it would not work as on runtime we don't use typed config. I think skipping support for this check seems better.
This use statement is not needed
Also updated the issue summary to be sort of up to date.
Comment #80
fagoAs discussed with dawehner, we want to rely on DataContainerInterface for sequences instead. Thus postponed on #2350597: Extract a common DataContainerInterface for lists and complex data.
Comment #82
dawehnerSee #2350597-42: Extract a common DataContainerInterface for lists and complex data why I continue on this issue again.
Note: This is no longer a problem, we have a sequence specific definition class now.
This tries to get the ball rolling again.
Comment #83
dawehnerI tried to update the issue summary to be more uptodate with the current patch.
Comment #84
Berdirdo we want to go through the trouble of making it optional with a fallback?
wondering if that should be part of the trait even? but we can do that elsewhere.
this assumes DataDefinitionInterface has this method, but you just add it to the base class?
Should we add it to the interface or should we make this conditional on having that method?
we might want slightly more valid values than hum and meh :)
when/how are we going to do that?
is that because of the injections?
Comment #85
dawehnerThank you for the review berdir!
Why not, seriously, let's just do that, its not worth thinking about it :)
Well it saves some code in other places I guess.
I just added the condition for now.
I expanded the allowed llamas a bit.
I think this should be fixed now by expanding the test coverage a bit.
Yeah, the object no longer compares that easy.
Comment #87
dawehnerValidating llamas is hard.
Comment #89
dawehner#87 was the wrong patch (some other branch got merged in)
Comment #90
BerdirThis looks good to me.
Not sure if we need a change record for this, While this technically allows to validate config, it sounds like a crazy amount of work to actually ensure we have everything properly validated (new meta issue?), so I don't think it's something we want to promote already?
Comment #91
alexpottI can't see an use of the class resolver.
Comment #92
alexpottNice.
Comment #93
Berdir@alexpott: That confused me as well for a moment, but the trick is that TypedConfigManager extends \Drupal\Core\TypedData\TypedDataManager, which defines and uses $this->classResolver. This was added in #2197029: Allow to inject dependencies into validation constraints and because TypedConfigManager does not call the parent constructor, we probably didn't notice or need htis.
But now that we can validate config, we need that too, for \Drupal\Core\TypedData\TypedDataManager::getValidator()
Comment #94
alexpottThe second sentence here doesn't make sense to me. Are we trying to say that we can't treat them as lists because they have named keys. But do they really? Sometimes they do and sometimes they don't and we don't differentiate.
Comment #95
dawehnerWhat about replacing
with
?
Comment #96
Wim LeersROFL
s/to/do/
More importantly: why not do:
?
Wouldn't that be clearer?
This is difficult to parse:
What about this instead:
This is because sequences MAY have named keys, which is not supported by ListInterface.
?
If it's a string, then why is the variable name
$object
?(Perhaps this makes sense from a Typed Data POV — I'm not familiar enough with it!)
s/typed data api/Typed Data API/
Comment #97
dawehnerWell, there is no parent here which we could call.
It is the thing you want to validate. ¯\_(ツ)_/¯ we can call how we want it to be called.
Comment #98
Wim Leers#97 RE #96.3: I also don't know what to call it.
#97 didn't address #94/#97.2 yet. Other than that, I think this looks good.
Comment #99
dawehneroops I failed on using vim copy commands.
Comment #100
Wim LeersComment #101
tstoecklerSo we currently do not validate an entire mapping or sequence on its own. If that's already possible I think we should add tests for that, as well. Especially for sequences this will really valuable, so you can validate the keys, as well. For mappings, I guess the schema structure is already sufficient validation in itself, but for sequences I think this would be quite nice.
Comment #102
dawehnerComment #103
tstoecklerNeat, that looks great. Thanks! I didn't review the entire patch in detail, but re-RTBCing per #100.
Comment #104
dawehnerOH where did my comment went. I wanted to write that its an excellent idea to test the entire tree, rather than just some specific elements.
Comment #105
alexpott#104 and
Seem to be in agreement that more testing would be good. No?
Comment #106
dawehner@alexpott
Right, well, I hope #102 added this additional test coverage :)
Comment #107
tstoecklerYes, from my point of view test coverage is sufficient now at least.
Comment #108
alexpottCommitted d31759a and pushed to 8.4.x. Thanks!
Fix on commit.
I agree with @Berdir in #90
Let's file the meta issue to explore actually implementing config validation.
Comment #110
dawehnerYeah. Thank you for everyone who was involved in the process. I'm so glad we made some progress here. I'm currently creating the following up meta issue to add the constrains.
Comment #111
dawehnerHere is the follow up: #2869792: [meta] Add constraints to all config entity types
Comment #113
xeM8VfDh CreditAttribution: xeM8VfDh commentedI'm curious about the change made to
core/core.service.yml
in the commit that resolved this issue. I opened a separate issue for it here: https://www.drupal.org/node/2915332#comment-12295557I posting here as well merely to get your alls opinion, particularly that of @alexpott.
The addition of
@validation.constraint
as a dependency in theconfig.typed
block is problematic in that the former has aparent
attribute, and configurations withparent
attributes cannot be instantiated at runtime during the bootstrapping/ContainerBuilder process (see the first conditional regardingDefinitionDecorator
inContainerBuilder::createService()
). So, whenconfig.typed
is instantiated during this process, the app chokes and fails to spin up, since thevalidation.constraint
is not accessible. This results in no requests (web, Drush, etc) can be fulfilled, basically bricking the application.My Solution Turned Out To Be
I'm wondering
,
Was the commit that resolved this issue intending to make
config.typed
inaccessible at build time?Comment #114
larowlanThere is still a todo in the code referencing this issue, I opened a followup
Comment #115
Wim LeersThis is now being used in #2969065: Use typed config validation constraints for validation of cdn.settings simple config and a rough outline for how core could use this is provided at #2164373-16: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs ….
Comment #116
Chi CreditAttribution: Chi commentedOne more todo followup #3092651: Add missing type hint to TypedDataInterface::createInstance() method.