Updated: Comment #10

Problem/Motivation

  • Sequence is a bit of a weird type data construct. It implements neither ListInterface nor ComplexDataInterface 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
CommentFileSizeAuthor
#102 interdiff-1928868.txt4.1 KBdawehner
#102 1928868-102.patch30.26 KBdawehner
#99 interdiff-1928868.txt980 bytesdawehner
#99 1928868-99.patch28.24 KBdawehner
#97 interdiff-1928868.txt4.24 KBdawehner
#97 1928868-97.patch28.22 KBdawehner
#89 1928868-85.patch28.23 KBdawehner
#87 interdiff-1928868.txt2.43 KBdawehner
#87 1928868-87.patch88.77 KBdawehner
#85 interdiff-1928868.txt3.73 KBdawehner
#85 1928868-85.patch28.21 KBdawehner
#82 1928868-82.patch27.86 KBdawehner
#78 interdiff.txt1.19 KBdawehner
#78 1928868-78.patch28.55 KBdawehner
#76 d8_typed_config.patch.interdiff.txt1.93 KBfago
#76 d8_typed_config.patch28.87 KBfago
#74 d8_typed_config.patch.interdiff.txt18.63 KBfago
#74 d8_typed_config.patch27.38 KBfago
#71 interdiff.txt1.28 KBdawehner
#71 1928868-71.patch28.49 KBdawehner
#69 interdiff.txt3 KBdawehner
#69 1928868-69.patch27.22 KBdawehner
#67 interdiff.txt14.14 KBdawehner
#67 1928868-67.patch24.81 KBdawehner
#65 interdiff.txt1.18 KBdawehner
#65 1928868-65.patch14.96 KBdawehner
#61 interdiff.txt449 bytesdawehner
#61 1928868-61.patch14.81 KBdawehner
#59 1928868-59.patch14.79 KBdawehner
#51 d8_typed_config_fix.interdiff.txt19.36 KBfago
#51 d8_typed_config_fix.patch20.54 KBfago
#46 d8_typed_config_fix.interdiff.txt5.33 KBfago
#46 d8_typed_config_fix.patch14.81 KBfago
#44 interdiff.txt5.64 KBdawehner
#44 1928868-44.patch13.48 KBdawehner
#43 1928868-43.patch8.09 KBdawehner
#1 d8_config_schema.patch1.52 KBfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Status: Active » Needs work
FileSize
1.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

Xano’s picture

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

Gábor Hojtsy’s picture

I don't believe anything blocks that.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Task
Status: Needs work » Postponed

At this point we would need to postpone improving this validation to be consistent to 8.1.x or later.

xjm’s picture

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

dawehner’s picture

Status: Postponed » Active
FileSize
8.09 KB

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

dawehner’s picture

Status: Active » Needs review
FileSize
13.48 KB
5.64 KB

It kinda works in theory, but the simplicity of \Drupal\Core\Config\Schema\Mapping::set causes some issues when you want to change some data.

Status: Needs review » Needs work

The last submitted patch, 44: 1928868-44.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
14.81 KB
5.33 KB

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

   * @todo When \Drupal\Core\Config\TypedConfigManager has been fixed to use
   *   class-based definitions, type-hint $definition to
   *   DataDefinitionInterface. https://www.drupal.org/node/1928868

I fear this would be an unacceptable API change for d8, thus this needs to wait until d9 now.

Status: Needs review » Needs work

The last submitted patch, 46: d8_typed_config_fix.patch, failed testing.

Gábor Hojtsy’s picture

@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

fago’s picture

There is no requirement for API signatures to look ideal ;)

Right. Not ideal either, but fine ;)

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

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

Gábor Hojtsy’s picture

Yeah there is no metadata, there is only the type template information and the data that decides on which type the template resolves to.

fago’s picture

Title: Make typed config valid typed data » Typed config incorrectly implements Typed Data interfaces
Status: Needs work » Needs review
FileSize
20.54 KB
19.36 KB

ok, 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!

fago’s picture

Category: Task » Bug report

Status: Needs review » Needs work

The last submitted patch, 51: d8_typed_config_fix.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

It is great that someone with actual knowledge about that stuff works on this!

  1. +++ b/core/lib/Drupal/Core/Config/Schema/Mapping.php
    @@ -31,4 +33,24 @@ protected function getElementDefinition($key) {
    +    $this->value[$property_name] = $value;
    +    // @todo notify?
    +    return $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?

  2. +++ b/core/lib/Drupal/Core/Config/Schema/Sequence.php
    @@ -34,4 +39,90 @@ protected function getElementDefinition($key) {
    +  public function getItemDefinition() {
    +    // @todo.
    +  }
    

    For sequences this should be sort be able to figure out, right?

  3. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
    @@ -113,6 +120,10 @@ public function buildDataDefinition(array $definition, $value, $name = NULL, $pa
    +    // @todo: Add property definitions for all known properties from the mapping
    +    // and sequence keys.
    

    It is a bit odd that the type config manager would have this responsibility.

  4. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -12,6 +12,9 @@
     use Drupal\Core\StringTranslation\TranslatableMarkup;
    +use Symfony\Component\Validator\Constraints\Choice;
    +use Symfony\Component\Validator\Constraints\EqualTo;
    +use Symfony\Component\Validator\Constraints\GreaterThan;
     
    

    unneeded, IMHO

The last submitted patch, 1: d8_config_schema.patch, failed testing.

The last submitted patch, 43: 1928868-43.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tstoeckler’s picture

Status: Needs review » Needs work

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

-abstract class Element extends TypedData {
+abstract class Element extends Config {

Seems like you've conceptually renamed Element to Config and made it a proper data type plugin. If so, we should properly deprecate Element.

+use Drupal\Core\TypedData\MapDataDefinition;
+

Doesn't seem to be used. Also elsewhere in the patch...

+ *
+ * @todo for Drupal 9: Do not implement ListInterface any more as lists are
+ * just ordered sets without keys, while Sequences have (unknown) keys.
  */
-class Sequence extends ArrayElement {
+class Sequence extends ArrayElement implements ListInterface {

Yeah, implementing ListInterface really is completely wrong for Sequence. So why are you introducing that in the first place?

+interface TypedConfigInterface extends TraversableTypedDataInterface, ComplexDataInterface {

This makes the current patch almost won't fix to me. ComplexDataInterface and ListInterface simply different semantics, and we don't share the same semantics in config land. That's why I introduced TraversableTypedDataInterface in the first place. So we should be improving that IMO, and not reverting back to our previous mistakes.

+      'unwrap_for_canonical_representation' => TRUE,

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!

+      constraints:
+        NotNull: []

Wow, this would really be a game changer.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.79 KB

After 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!

Status: Needs review » Needs work

The last submitted patch, 59: 1928868-59.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.81 KB
449 bytes
Wim Leers’s picture

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

+++ b/core/modules/config/tests/config_test/config/install/config_test.validation.yml
@@ -0,0 +1,7 @@
+llama: meh

I APPROVE

(despite the 'meh', which we will need to discuss in person at some point)

Status: Needs review » Needs work

The last submitted patch, 61: 1928868-61.patch, failed testing.

tstoeckler’s picture

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

diff --git a/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
index b765b05..cf4b3fc 100644
--- a/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
+++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
@@ -142,7 +143,9 @@ protected function validateNode(TypedDataInterface $data, $constraints = NULL, $
 
     // If the data is a list or complex data, validate the contained list items
     // or properties. However, do not recurse if the data is empty.
-    if (($data instanceof ListInterface || $data instanceof ComplexDataInterface) && !$data->isEmpty()) {
+    if ((($data instanceof ComplexDataInterface || $data instanceof ListInterface) && !$data->isEmpty()) ||
+      ($data instanceof TraversableTypedDataInterface)
+    ) {
       foreach ($data as $name => $property) {
         $this->validateNode($property);
       }

Wow, that's terrible, but totally not your fault. See #2420067: Promote usage of TraversableTypedDataInterface for cleaning that up.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.96 KB
1.18 KB

Thank you for your feedback @Wim Leers and @tstoeckler!

But from looking at the patch, this is a lot closer to what I was thinking, thanks!

Yeah, it seems to make more sense.

Wow, that's terrible, but totally not your fault. See #2420067: Promote usage of TraversableTypedDataInterface for cleaning that up.

I'm glad you haven't seen this interdiff yet.

Status: Needs review » Needs work

The last submitted patch, 65: 1928868-65.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
24.81 KB
14.14 KB

This addresses just the test failures.

Status: Needs review » Needs work

The last submitted patch, 67: 1928868-67.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.22 KB
3 KB

Some more fixes

Status: Needs review » Needs work

The last submitted patch, 69: 1928868-69.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.49 KB
1.28 KB

Fixed the remaining failures

fago’s picture

Thanks for working on this + great to see a green patch! It looks already pretty good.

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?

yes, that seems reasonable. Seems silly to take care of it when we do not need it.

Here my review:

  1. +++ b/core/lib/Drupal/Core/Config/Schema/Mapping.php
    @@ -26,4 +28,24 @@ protected function getElementDefinition($key) {
    +  public function getProperties($include_computed = FALSE) {
    

    Missing docblock

  2. +++ b/core/lib/Drupal/Core/Config/Schema/Sequence.php
    @@ -11,7 +13,7 @@
    -class Sequence extends ArrayElement {
    +class Sequence extends ArrayElement implements TraversableTypedDataInterface {
    

    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.

  3. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -2,11 +2,16 @@
    +  use DependencySerializationTrait;
    

    Given __sleep() is already implemented, this seems to be unused?

  4. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -142,7 +143,10 @@ protected function validateNode(TypedDataInterface $data, $constraints = NULL, $
    +    // Note: Most traversable, doesn't have a concept of empty/not empty, but
    

    Beginning of the sentence seems to be flawed? At least I cannot parse it :)

fago’s picture

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

fago’s picture

ok, 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

Status: Needs review » Needs work

The last submitted patch, 74: d8_typed_config.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
28.87 KB
1.93 KB
fago’s picture

elaborating on #51:

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.

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

dawehner’s picture

Issue summary: View changes
FileSize
28.55 KB
1.19 KB
  1. +++ b/core/config/schema/core.data_types.schema.yml
    @@ -46,7 +46,7 @@ mapping:
       class: '\Drupal\Core\Config\Schema\Sequence'
    -  definition_class: '\Drupal\Core\TypedData\ListDataDefinition'
    +  definition_class: '\Drupal\Core\TypedData\MapDataDefinition'
    

    That 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 of ComplexDataDefinitionInterface

  2. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
    @@ -161,4 +163,28 @@ public function isNullable() {
    +      $definition = $this->getElementDefinition($name);
    +      if ($include_computed || !$definition->isComputed()) {
    

    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.

  3. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -2,11 +2,15 @@
     
    +use Drupal\Core\DependencyInjection\DependencySerializationTrait;
    +
    

    This use statement is not needed

Also updated the issue summary to be sort of up to date.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fago’s picture

Status: Needs review » Postponed

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Status: Postponed » Needs review
FileSize
27.86 KB

See #2350597-42: Extract a common DataContainerInterface for lists and complex data why I continue on this issue again.

That 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 of ComplexDataDefinitionInterface

Note: This is no longer a problem, we have a sequence specific definition class now.

This tries to get the ball rolling again.

dawehner’s picture

Issue summary: View changes

I tried to update the issue summary to be more uptodate with the current patch.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
    @@ -45,13 +46,18 @@ class TypedConfigManager extends TypedDataManager implements TypedConfigManagerI
         $this->alterInfo('config_schema_info');
         $this->moduleHandler = $module_handler;
    +    $this->classResolver = $class_resolver;
       }
    

    do we want to go through the trouble of making it optional with a fallback?

  2. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -340,4 +342,14 @@ public function toArray() {
    +   * {@inheritdoc}
    +   */
    +  public function __sleep() {
    +    // Never serialize the typed data manager.
    +    $vars = get_object_vars($this);
    +    unset($vars['typedDataManager']);
    +    return array_keys($vars);
    +  }
    +
    

    wondering if that should be part of the trait even? but we can do that elsewhere.

  3. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
    @@ -117,7 +117,10 @@ public function createDataDefinition($data_type) {
         $class = $type_definition['definition_class'];
    -    return $class::createFromDataType($data_type);
    +    $data_definition = $class::createFromDataType($data_type);
    +    $data_definition->setTypedDataManager($this);
    +
    

    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?

  4. +++ b/core/modules/config/tests/config_test/src/ConfigValidation.php
    @@ -0,0 +1,68 @@
    +    if ($object !== 'meh') {
    

    we might want slightly more valid values than hum and meh :)

  5. +++ b/core/tests/Drupal/KernelTests/Config/TypedConfigTest.php
    @@ -0,0 +1,125 @@
    +    // @todo: Make metadata about contained properties available and add test
    +    // coverage here.
    +
    

    when/how are we going to do that?

  6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTypedDataDefinitionTest.php
    @@ -123,7 +123,7 @@ public function testEntityReferences() {
         $reference_definition2 = $this->typedDataManager->createDataDefinition('entity_reference');
         $this->assertTrue($reference_definition2 instanceof DataReferenceDefinitionInterface);
    -    $this->assertEqual($reference_definition2, $reference_definition);
    +    $this->assertEqual(serialize($reference_definition2), serialize($reference_definition));
    

    is that because of the injections?

dawehner’s picture

Thank you for the review berdir!

do we want to go through the trouble of making it optional with a fallback?

Why not, seriously, let's just do that, its not worth thinking about it :)

wondering if that should be part of the trait even? but we can do that elsewhere.

Well it saves some code in other places I guess.

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?

I just added the condition for now.

we might want slightly more valid values than hum and meh :)

I expanded the allowed llamas a bit.

when/how are we going to do that?

I think this should be fixed now by expanding the test coverage a bit.

is that because of the injections?

Yeah, the object no longer compares that easy.

Status: Needs review » Needs work

The last submitted patch, 85: 1928868-85.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
88.77 KB
2.43 KB

Validating llamas is hard.

Status: Needs review » Needs work

The last submitted patch, 87: 1928868-87.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.23 KB

#87 was the wrong patch (some other branch got merged in)

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/core.services.yml
@@ -331,9 +331,11 @@ services:
+    arguments: ['@config.storage', '@config.storage.schema', '@cache.discovery', '@module_handler', '@class_resolver']

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -45,13 +46,18 @@ class TypedConfigManager extends TypedDataManager implements TypedConfigManagerI
+   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface $class_resolver
+   *   (optional) The class resolver.
...
+  public function __construct(StorageInterface $configStorage, StorageInterface $schemaStorage, CacheBackendInterface $cache, ModuleHandlerInterface $module_handler, ClassResolverInterface $class_resolver = NULL) {
...
+    $this->classResolver = $class_resolver ?: \Drupal::service('class_resolver');

I can't see an use of the class resolver.

alexpott’s picture

+++ b/core/modules/system/config/schema/system.schema.yml
@@ -7,6 +7,8 @@ system.site:
+      constraints:
+        NotNull: []

Nice.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
@@ -161,4 +163,25 @@ public function isNullable() {
diff --git a/core/lib/Drupal/Core/Config/Schema/Sequence.php b/core/lib/Drupal/Core/Config/Schema/Sequence.php

+++ b/core/lib/Drupal/Core/Config/Schema/Sequence.php
@@ -10,6 +10,12 @@
+ * Note that sequences implement the typed data ComplexDataInterface (via the
+ * parent ArrayElement) rather than the ListInterface. This is, as sequences
+ * have named keys, what is not covered by lists. From the typed data API
+ * perspective sequences are handled as ordered mappings without metadata about
+ * existing properties.

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

dawehner’s picture

What about replacing

This is, as sequences
 * have named keys, what is not covered by lists.

with

This is, as sequences
 * can have named keys, what is not covered by lists.

?

Wim Leers’s picture

I expanded the allowed llamas a bit.

Validating llamas is hard.

ROFL


  1. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
    @@ -161,4 +163,25 @@ public function isNullable() {
    +    // Config schema elements to not make use of notifications. Thus, we skip
    +    // notifying parents.
    

    s/to/do/

    More importantly: why not do:

    $notify = FALSE;
    return parent::set($property_name, $value, $notify);
    

    ?

    Wouldn't that be clearer?

  2. +++ b/core/lib/Drupal/Core/Config/Schema/Sequence.php
    @@ -10,6 +10,12 @@
    + * parent ArrayElement) rather than the ListInterface. This is, as sequences
    + * have named keys, what is not covered by lists. From the typed data API
    

    This is difficult to parse:

    This is, as sequences have named keys, what is not covered by lists.

    What about this instead:
    This is because sequences MAY have named keys, which is not supported by ListInterface.

    ?

  3. +++ b/core/modules/config/tests/config_test/src/ConfigValidation.php
    @@ -0,0 +1,68 @@
    +   * @param string $object
    +   *   The string to validate.
    ...
    +   * @param string $object
    +   *   The string to validate.
    ...
    +   * @param string $object
    +   *   The string to validate.
    

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

  4. +++ b/core/tests/Drupal/KernelTests/Config/TypedConfigTest.php
    @@ -0,0 +1,126 @@
    +   * Tests config validation via the typed data api.
    

    s/typed data api/Typed Data API/

dawehner’s picture

Wouldn't that be clearer?

Well, there is no parent here which we could call.

f 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!)

It is the thing you want to validate. ¯\_(ツ)_/¯ we can call how we want it to be called.

Wim Leers’s picture

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

dawehner’s picture

#97 didn't address #94/#97.2 yet. Other than that, I think this looks good.

oops I failed on using vim copy commands.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/config/tests/config_test/config/schema/config_test.schema.yml
@@ -158,3 +158,33 @@ config_test.foo:
+    cat:
+      type: mapping
+      mapping:
+        type:
+          type: string
+          constraints:
+            Callback:
+              callback: [\Drupal\config_test\ConfigValidation, validateCats]
+        count:
+          type: integer
+          constraints:
+            Callback:
+              callback: [\Drupal\config_test\ConfigValidation, validateCatCount]
+    giraffe:
+      type: sequence
+      sequence:
+        type: string
+        constraints:
+          Callback:
+            callback: [\Drupal\config_test\ConfigValidation, validateGiraffes]

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

dawehner’s picture

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Neat, that looks great. Thanks! I didn't review the entire patch in detail, but re-RTBCing per #100.

dawehner’s picture

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

alexpott’s picture

#104 and

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,

Seem to be in agreement that more testing would be good. No?

dawehner’s picture

@alexpott
Right, well, I hope #102 added this additional test coverage :)

tstoeckler’s picture

Yes, from my point of view test coverage is sufficient now at least.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d31759a and pushed to 8.4.x. Thanks!

diff --git a/core/lib/Drupal/Core/Config/Schema/ArrayElement.php b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
index 3e4ba7e..b38a94b 100644
--- a/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
+++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
@@ -177,7 +177,7 @@ public function set($property_name, $value, $notify = TRUE) {
    * {@inheritdoc}
    */
   public function getProperties($include_computed = FALSE) {
-    $properties = array();
+    $properties = [];
     foreach (array_keys($this->value) as $name) {
       $properties[$name] = $this->get($name);
     }

Fix on commit.

I agree with @Berdir in #90

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?

Let's file the meta issue to explore actually implementing config validation.

  • alexpott committed d31759a on 8.4.x
    Issue #1928868 by dawehner, fago, Gábor Hojtsy, Jose Reyero, tstoeckler...
dawehner’s picture

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

dawehner’s picture

Status: Fixed » Closed (fixed)

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

xeM8VfDh’s picture

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

I posting here as well merely to get your alls opinion, particularly that of @alexpott.

The addition of @validation.constraint as a dependency in the config.typed block is problematic in that the former has a parent attribute, and configurations with parent attributes cannot be instantiated at runtime during the bootstrapping/ContainerBuilder process (see the first conditional regarding DefinitionDecorator in ContainerBuilder::createService()). So, when config.typed is instantiated during this process, the app chokes and fails to spin up, since the validation.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

  1. restore backup
  2. update Libraries Module from 8.x-3.x-dev (2016-Nov-13) to latest 8.x-3.x-dev
  3. Get Pantheon support to restart my webstie container to get around Fatal Error cause by step above (more)
  4. run update.php for good measure
  5. update core to 8.4.0

I'm wondering

,
Was the commit that resolved this issue intending to make config.typed inaccessible at build time?

larowlan’s picture

There is still a todo in the code referencing this issue, I opened a followup

Wim Leers’s picture

Chi’s picture