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

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

Status:Active» Needs work
StatusFileSize
new1.52 KB

Oh, here a first start not doing much yet.

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.

Crosspost?

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

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

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.

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.

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.

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.

Issue summary:View changes

update summary to template and with comment.

I might have a look at this, because while doing #2002138: Use adapters for supporting typed data 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.

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

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

Is this even still an issue following #2002138: Use adapters for supporting typed data? Tentatively demoting to normal and setting postponed for more info.

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.

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

Issue summary:View changes

change dot list

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

Unpostponing as per above.