Updated: Comment #10

Problem/Motivation

Right now, typed config implements the typed data interfaces but does not properly implements its API. Let's use this issue to work over that and fix things based upon
#1913328: Provide general list and map classes and
#1905230: Improve the typed data API usage of configuration schema
Regarding the data definitions I mostly see 'mapping' and 'sequence' as problematic, as they are non-valid top-level definition keys. Per-type specific keys are supposed to go below 'settings' and should be documented at the type's class. This could be moved during processing though.
Sequences seems to be equivalent to lists but use a totally different notation. We need to align it with the typed-data list notations. However, I think the way sequences are specified matches with what others suggested doing for typed data as well (and I agree with), so maybe we should re-work it for typed data first.
Issues I see with the current code:

  • Used type names, i.e. as returned from getType() have to be registered via the plugin discovery.

The way I see this could work:

  • Use the typed config manage to pre-process config schema definitions to valid typed data definitions. That's partly done already, but we need to make sure that we end up with data definitions matching the docs. Also the custom merging of type-definitions and data-definitions as done in the TypedConfigElementFactory right now could happen there.
  • Best, this processing of the definition would be available as its own helper function also.
  • Make the classes extend the new general Map and List classes as suiting (or not).
  • Make sure the classes properly implement the interfaces - I mostly see all validate() implementations violating the interface. Maybe we can re-used typed data tests for the map and ItemList classes.

Proposed resolution

This will be a set of patches, one step at a time, based on the previous list.

  • Use DataDefinitionInterface for typed config and make TypedConfigManager extend TypedDataManager reusing some of the implementation.
  • Add a TypedDataManagerInterface in order to be able to pass around either TypedConfgManager or TypedDataManager. That one should be extended by TypedConfigManagerInterface
  • Reuse some more TypedData classes for TypedConfig (Any, Map, List...). Maybe we can replace TypedConfig ones completely, maybe not, not sure yet.
  • Fix validation for Typed config elements, reusing (and maybe extending) TypedData validation

A patch replacing type 'undefined' to 'any' was propose but the 'undefined' type is in system.data_types.schema.yml

Files: 
CommentFileSizeAuthor
#1 d8_config_schema.patch1.52 KBfago

Comments

fago’s picture

Status:Active» Needs work
StatusFileSize
new1.52 KB

Oh, here a first start not doing much yet.

Gábor Hojtsy’s picture

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

Not sure how types like "views.filter.[table]-[field]" end up in the end, but for making the result a valid type we'd either need sure to register it or make a generic type with parameters, e.g. type 'views_filter' with settings 'table' and 'field'.

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.

fago’s picture

Gábor Hojtsy’s picture

Crosspost?

fago’s picture

ad 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 say views.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'.

Gábor Hojtsy’s picture

Yeah 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(?)

fago’s picture

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

dcam’s picture

http://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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

Malerio’s picture

Issue summary:View changes

update summary to template and with comment.

dixon_’s picture

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.

dlu’s picture

Component:base system» configuration system

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

xjm’s picture

Status:Needs work» Postponed (maintainer needs more info)

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Issue summary:View changes

change dot list

Gábor Hojtsy’s picture

Issue summary:View changes
Status:Postponed (maintainer needs more info)» Needs work

Unpostponing as per above.

Jose Reyero’s picture

Yes, 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:

abstract class TypedData implements TypedDataInterface, PluginInspectionInterface { {
---
  public function validate() {
    // @todo: Add the typed data manager as proper dependency.
    return \Drupal::typedDataManager()->getValidator()->validate($this);
  }
  ...
}

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.

fago’s picture

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

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

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.

Jose Reyero’s picture

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

fago’s picture

You're right about discovery, we could extend that, though I feel it's too late for big refactorings in D8, isn't it?

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

Jose Reyero’s picture

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

Jose Reyero’s picture

Hey, I've just found YamlDiscovery and YamlDiscoveryDecorator, so forget my last words, we may not be that far after all :-)

Gábor Hojtsy’s picture

I'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.

Jose Reyero’s picture

@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

Jose Reyero’s picture

Updated summary with proposed resolution.

Jose Reyero’s picture

fago’s picture

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

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.

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.

fago’s picture

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

type: theme_settings

-> something like

  type: mapping
  mapping_schema: theme_settings
Jose Reyero’s picture

Typed data validation does not feature different error levels...

I'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',

Gábor Hojtsy’s picture

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

Jose Reyero’s picture

Related issues:
Jose Reyero’s picture

mgifford’s picture

Gábor Hojtsy’s picture

Any actionable subissues here to work on? :) All the child issues are now resolved :)

Jose Reyero’s picture

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

Gábor Hojtsy’s picture

Jose: would be great if you can help with that one then :) Thanks for the summary!

Jose Reyero’s picture