Problem/motivation

This is an implementation side issue following conversation in #1648930: Introduce configuration schema and use for translation, more details there.

Proposal

Based on prior work on all of the other issues, this is a proposal for a schema format (metadata) that has the following characteristics:

  • A schema format inspired by the pre-existing Kwalify format so the core format is "not invented here" and possibly existing kwalify tools could be reused to some extent. See: http://www.kuwata-lab.com/kwalify/ruby/users-guide.01.html and https://drupal.org/node/1648930#comment-6834008 for more information.
  • It is self-contained and fully extendable, meaning that the data types are contained in the schema itself (including the most basic ones) and any module can add any number of types, that can be based on pre-existing ones.
  • It reuses TypedData API, as the schema itself contains the mapping from data types to TypedData classes. It would reuse existing types and will need a few more defined as a part of the configuration classes (Drupal/Core/Config) but also any module can add their own, just declaring them on the schema.
  • Since new types are defined in the schema itself, it needs no hooks, nor they can be built using the typed_data() core factory. Adding a new factory for them is quite straight forward though and just needs some "class TypedConfigManager extends TypedDataManager".

This is how it looks like, note we need only one schema.yml file per module that can be imported into the config system and therefore, can be altered by contrib modules just by editing the configuration. The system.schema.yml file defines the base types (and other system level complex schema elements):

# Basic scalar data types from typed data.
boolean:
  label: 'Boolean'
  class: '\Drupal\Core\TypedData\Type\Boolean'
email:
  label: 'Email'
  class: '\Drupal\Core\TypedData\Type\Email'
integer:
  label: 'Integer'
  class: '\Drupal\Core\TypedData\Type\Integer'
string:
  label: 'String'
  class: '\Drupal\Core\TypedData\Type\String'
uri:
  label: 'Uri'
  class: '\Drupal\Core\TypedData\Type\Uri'

# Basic data types for configuration.
undefined:
  label: 'Undefined'
  class: '\Drupal\Core\Config\Schema\Property'
mapping:
  label: Mapping
  class: '\Drupal\Core\Config\Schema\Mapping'
sequence:
  label: Sequence
  class: '\Drupal\Core\Config\Schema\Sequence'

# Default mapping for unknown types or types not found.
default:
  type: undefined
  label: 'Unknown'

# Simple extended data types:

# Human readable string that must be plain text and editable with a text field.
label:
  type: string
  label: 'Label'

# Internal Drupal path
path:
  type: string
  label: 'Path'

# Human readable string that can contain multiple lines of text or HTML.
text:
  type: string
  label: 'Text'

# Complex extended data types:

# Mail text with subject and body parts.
mail:
  type: mapping
  label: "Mail"
  mapping:
    "subject":
      type: text
      label: "Subject"
    "body":
      type: text
      label: "Body"

# Schema for configuration files of system module:

system.site:
  type: mapping
  label: 'Site information'
  mapping:
    "name":
      label: "Site name"
      type: label
    "mail":
      label: "Site mail"
      type: email
    "slogan":
      label: "Site slogan"
      type: text
    "page":
      type: mapping
      mapping:
        "403":
          type: path
        "404":
          type: path
        "front":
          type: path
          label: "Front page path"
    "admin_compact_mode":
      type: boolean
    "weight_select_max":
      type: integer

system.maintenance:
  type: mapping
  label: 'Maintenance mode'
  mapping:
    "enabled":
      type: boolean
      label: "Put site into maintenance mode"
    "message":
      type: text
      label: "Message to display when in maintenance mode"

Another example from image module:

# Image module schema: image.schema.yml

# Data types for image module.
image.size:
  type: mapping
  mapping:
    "width":
      type: integer
      label: "Width"
    "height":
      type: integer
      label: "Height"

# Image styles (multiple).
# Plugin \Drupal\image\Plugin\Core\Entity\ImageStyle
image.style.*:
  type: mapping
  label: "Image style"
  mapping:
    "name":
      type: string
    "label":
      type: label
    "effects":
      type: sequence
      sequence:
        - type: mapping
          mapping:
            "name":
              type: string
            "data":
              type: image.effect.[ %parent.name]
            "weight":
              type: integer
            "ieid":
              type: string

# Image effects plugins: image.effect.%
# These are used in image styles.
image.effect.image_crop:
  type: image.size
  label: "Image crop"
  mapping:
    "anchor":
      label: "Anchor"

image.effect.image_resize:
  type: image.size
  label: "Image resize"

image.effect.image_rotate:
  type: mapping
  label: "Image rotate"
  mapping:
    degrees:
      type: integer
      label: 'Rotation angle'
    bgcolor:
      label: 'Background color'
    random:
      type: boolean
      label: 'Randomize'

image.effect.image_scale:
  type: image.size
  label: "Image scale"
  mapping:
    "upscale":
      type: boolean
      label: "Upscale"

image.effect.image_scale_and_crop:
  type: image.size
  label: "Image scale and crop"

# Schema for configuration files of image module.
image.settings:
  type: mapping
  mapping:
    "preview_image":
      type: string
      label: "Preview image"

Here is the full class/interface diagram as of #44

Kwalify (along with Rx) was suggested by Damien Tournoud at #1648930: Introduce configuration schema and use for translation back in 2012 July, https://drupal.org/node/1648930#comment-6189800

API additions

The new API function config_typed() can be used to get typed data information about the configuration key passed. For example the following code will return a typed data array:

  $definition = config_typed()->getDefinition('system.maintenance');

The structure of the array is as follows:

  $definition = array(
    'label' => 'Maintenance mode',
    'class' => '\Drupal\Core\Config\Schema\Mapping',
    'mapping' => array(
      'enabled' => array(
        'label' => 'Put site into maintenance mode',
        'type' => 'boolean',
      ),
      'message' => array(
        'label' =>  'Message to display when in maintenance mode',
        'type' => 'text',
      ),
    ),
  );
Files: 
CommentFileSizeAuthor
#52 1866610.47.config-schema.patch40.56 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 48,773 pass(es). View
#47 1866610.47.config-schema.patch54.5 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 49,414 pass(es), 60 fail(s), and 29 exception(s). View
#45 Kwalify schema object model.png73.05 KBheyrocker
#44 1866610.44.config-schema.patch40.79 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 49,300 pass(es). View
#44 39-44-interdiff.txt7.99 KBalexpott
#40 config_schema-39.diff.patch20.04 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config_schema-39.diff.patch. Unable to apply patch. See the log in the details link for more information. View
#39 config_schema-39.patch40.72 KBJose Reyero
PASSED: [[SimpleTest]]: [MySQL] 50,717 pass(es). View
#13 config_schema-13.patch40.51 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 50,465 pass(es). View
#13 interdiff.txt6.09 KBeffulgentsia
#11 config_schema-11.patch39.48 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#3 config_schema-01.patch39.52 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint, +language-config

Tagging it up.

Gábor Hojtsy’s picture

Posted this request for reviews in the Core group (also should be on Drupal Planet): http://groups.drupal.org/node/273268

Jose Reyero’s picture

Status: Active » Needs review
FileSize
39.52 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

This is the patch which:
- Provides the basic data types in system.schema.yml
- Provides the schema for a few modules (image, system, user...)
- Reuses the TypedData manager and factory, adds a new SchemaDiscovery.
- Adds a very basic API and one test. This is how the API looks like:

// Get configuration as typed data.
config_typed()->get('image.style.large');
// Get schema for any 'configuration name'
config_typed()->getDefinition('image.style.large');

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-config

The last submitted patch, config_schema-01.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review

#3: config_schema-01.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +language-config

The last submitted patch, config_schema-01.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +epic

Tagging as epic.

Gábor Hojtsy’s picture

Category: feature » task

Recategorize as task as per the other issues.

Gábor Hojtsy’s picture

Cross-quoting @heyrocker from #1648930-346: Introduce configuration schema and use for translation:

I would like to establish a deadline for deciding how we're going to proceed with this. If we don't, we are going to go back and forth on these decisions in a circle until feature freeze comes and then we're screwed. My feeling is we will probably need about a month to implement any new proposal and get it to RTBC, so I'd like to lay down January 15 as our deadline or deciding what to do. If we haven't agreed on any new proposal, then we reroll the original RTBC patch from #285 and get it in. We have to get this done, and right now I don't see any light at the end of the tunnel which scares the crap out of me.

I very much agree we need to get out of our lack of agreement here, because we are dealing with things we absolutely much do even if only to avoid sizeable regressions in Drupal 8.

In light of this, reviews pro or against the above proposals are more than welcome.

Gábor Hojtsy’s picture

Posted this call for feedback on the Core group (also going to Drupal Planet): http://groups.drupal.org/node/275008

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
39.48 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Rerolled for HEAD changes.

Status: Needs review » Needs work

The last submitted patch, config_schema-11.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.09 KB
40.51 KB
PASSED: [[SimpleTest]]: [MySQL] 50,465 pass(es). View

Fixed whitespace errors and code for HEAD changes to TypedData.

effulgentsia’s picture

Priority: Normal » Critical

Swapping criticals with #1865300: Introduce configuration definition file format and API for reading it because this patch is farther along.

effulgentsia’s picture

I spent a few hours looking over this patch, and I must say, I like it. Initially, I did not like the extra verbosity and depth of:

image.style.%:
  type: map
  label: "Image style"
  mapping:
    "name":
      type: str
    "label":
      type: label
    "effects":
      type: seq
      sequence:
        - type: map
          mapping:
            "name":
              type: str
            "data":
              type: image.effect.[%parent.name]
            "weight":
              type: weight
            "ieid":
              type: str

vs the flattened version of the above that I suggested in #1865300: Introduce configuration definition file format and API for reading it, which would be more like:

image.style.%:
  "":
    label: "Image style"
  "name":
    type: str
  "label":
    type: label
  "effects.%.name":
    type: str
  "effects.%.name":
    type: image.effect.[%parent.name]
  "effects.%.weight":
    type: weight
  "effects.%.ieid":
    type: str

I.e., the first code snippet has 21 lines and extra indentation with the mapping keyword contributing to both, whereas the second code snippet has 15 lines and less indentations, because the config key structure is flattened with a "." delimiter, and thereby, there's no need for a mapping keyword.

However, in looking at the two side by side, I can't really claim that the 2nd is all that much easier on the eyes than the 1st is, and meanwhile, the benefit of the 1st being based on Kwalify, which is used by other projects, is documented, and has tools, might actually outweigh the need for extra lines and indentation.

As noted in the issue summary, this isn't pure Kwalify though. It extends it by:
- Making 'type' extensible rather than limited to the list in http://www.kuwata-lab.com/kwalify/ruby/users-guide.01.html#schema-rules.
- And related to above, makes the first level of the schema file the type name (e.g., image.style.%), whereas a pure Kwalify schema file would only contain what is inside one of those blocks.

I think this extension is a very clever way of implementing Drupal's need to support plugins, but I don't know if this effectively negates the ability to use existing Kwalify tools.

As to the patch itself, there are some things we'll want to refine (e.g., caching of SchemaDiscovery), but overall I think the implementation and integration with TypedData is quite nice, and I'm happy to see wildcard and plugin linking fully working.

So, my current recommendation is to abandon #1865300: Introduce configuration definition file format and API for reading it in favor of this, but I'm curious what others think.

beejeebus’s picture

Status: Needs review » Needs work

initial incomplete review.

- config_definition() is called, but not defined. is that a typo in the function name, or a to-be-added function? and i guess that means we need tests for that bit.

- should Parser be container aware, or have the service objects 'config.typed' and 'config.definition'(?) injected?

- same for Element, looks like it could use the 'config.typed' service

+ $this->assertTrue(count($effects) == 1, 'Got an array with effects for image.style.large data');
+ $ieid = key($effects->getValue());

- i don't know if this is required to use the TypedData API, but can we avoid fake arrays, and just use a real API? do callers have to treat this data as a not-quite-usable-as-an-array array? i may be missing something, but the classes would read a lot easier if we could use method names we choose, rather than the stupid you get with ArrayAccess. and the calling code could avoid "it's an array. it's an object. IT'S ALL THE THINGS" code like the above

Gábor Hojtsy’s picture

Title: Yet another schema format for Drupal configuration (Based on kwalify) » Introduce Kwalify-based schema format for configuration

@chx agreed on #1865300: Introduce configuration definition file format and API for reading it, so retitling this as *the* proposal.

Gábor Hojtsy’s picture

Some environment analysis on Kwalify:

1. Seems like the last Kwalify release was in 2010-07-18 and http://www.kuwata-lab.com (where it is being released) does not seem to produce new releases of any of their projects either since 2012 February (with releases appearing often before). (However we are not using any of the software from there!)

2. Do we know that this is actually used elsewhere?

3. Kwalify (along with Rx) was suggested by Damien Tournoud as well at #1648930-13: Introduce configuration schema and use for translation on the original issue back in 2012 July.

4. Kwalify's license is MIT as per http://www.kuwata-lab.com/kwalify/, question is if this applies to the software only as shipped there or the format as well. Not sure if the license can apply to the format. The patch only mentions Kwalify twice where the types are defined. Really the implementation is very custom to Drupal but the format is taken and forked.

Jose Reyero’s picture

re @effulgentsia #15,
Thanks for the reroll, much cleaner now

I don't know if this effectively negates the ability to use existing Kwalify tools.

Not for the extended types, though they could be used by pre-parsing the schema into kwalify (all extended types can be "reduced" to a kwalify type).
About format and wildcards, we could use any character now for wildcards (replace % by *) since this is independent of file names.

re @beejeebus #16,
The Parser class (which is the only one calling config_definition) could be dropped, we don't really need it.
Or if we want to keep it, replace config_definition() by config_typed()->getDefinition()

can we avoid fake arrays, and just use a real API?

I think ArrayAccess is a useful way to be able to iterate over any kind of "multiple" element (Mapping, Sequence). Otherwise we need to make the difference between Mapping and Sequence on any code iterating typed configuration.
I don't think typed data provides any very usable option here to replace this.

re @Gábor Hojtsy #18
Though I'd share your concerns about maintainability of Kwalify, we don't really use the library, just borrow some ideas about the format so I don't think that's a major concern.
I don't know whether it's used elsewhere but AFAIK it is the only viable existing option for providing an schema for YAML (Rx just won't work with simplified yaml parsers like Symfony's). So well, we may be the first 'major' project building on it.
About this and legal issues, I've dropped a note to Kwalify's author, just to know his take on the matter, I'll report here if I get a reply.

Jose Reyero’s picture

Issue summary: View changes

Updating schema examples.

Jose Reyero’s picture

Assigned: Unassigned » Jose Reyero

Assigning this issue to me.

Gábor Hojtsy’s picture

Do we have / can we maintain a list of tasks to do still on this issue?

effulgentsia’s picture

Re #18, I wonder if we'd be better served by using JSON Schema as a base rather than Kwalify. It seems like JSON Schema is getting a bit more traction in the broader web community these days. Of course, we're using YAML, not JSON, but in many ways, YAML is not all that different from JSON, so I think a lot of JSON Schema's concepts, terminology, and rules can carry over pretty directly to a YAML representation.

I suggest people here read up on http://json-schema.org/ and the 2 examples linked to from there (http://json-schema.org/example.html and http://json-schema.org/example2.html).

I don't think it would require that much of a change from what's already been figured out here based on Kwalify. Here's what I came up with as the JSON Schema equivalents of the issue summary's image.style.% and image.effect.image_crop portions of image.schema.yml:

image.style.%:
  type: object
  title: "Image style"
  properties:
    "name":
      type: string
    "label":
      $ref: "URI-OF-SYSTEM-SCHEMA#label"
    "effects":
      type: object
      additionalProperties:
        type: object
        properties:
          "name":
            type: string
          "data":
            $ref: "{???}#image%2Eeffect%2E{parent.name}"
          "weight":
            $ref: "URI-OF-SYSTEM-SCHEMA#weight"
          "ieid":
            type: string

image.effect.image_crop:
  extends: 
    $ref: "#image%2Esize"
  title: "Image crop"
  properties:
    "anchor":
      title: "Anchor"

Here's the basic differences from the Kwalify format:

  • "string" instead of "str"
  • "object" instead of "map"
  • "properties" instead of "mapping"
  • "additionalProperties" instead of "sequence" (for string indexed items, like effects). There's also "items" if the items are numerically indexed.
  • "title" instead of "label"
  • Instead of custom types, you can reference other schemas completely via $ref or extend them via extends.$ref.

Other than the wider usage of JSON Schema, the big benefit I see to it over Kwalify is that it includes the $ref concept in its spec. However, as can be seen from the above example, there's some challenges with it:

  • If referencing a fragment from another document, we need to address that document via a URI. So if image.schema.yml wants to reference the "label" fragment of system.schema.yml, it needs to do it via a URI, so we need to come up with URIs for addressing Drupal schemas. That's not a bad thing, just something for us to figure out.
  • Fragment values use JSON Pointer notation. Yay that there's a spec for that rather than us inventing a Drupalism. But, the spec treats "." as navigating the structure. In our case, we have "." in our key names (e.g., image.size), so it needs to be URI encoded (hence, $ref: "#image%2Esize" which is less readable than $ref: "#image.size").
  • Braces in URIs for holding variables to expand is also part of the spec (yay!) and is something we're also doing in our MODULE.routing.yml files (yay!). However, the spec only explicitly discusses URI placeholder expansion for URIs used in the hyperlinking portion of the spec, not as $ref values, from what I can tell. So the example above of $ref: "{???}#image%2Eeffect%2E{parent.name}" might not be completely valid, though it's at least based very closely on existing specs.
  • Also from the above example, there's the {???} part. The issue here is that {parent.name} not only controls the fragment we want, but also indirectly the module that defines that fragment, and therefore the initial part of the URI too. But it does so in a non obvious way. We have issues open to address that (#1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term)), but even without that, we could potentially define an abstract URI that could contain the union of all fragments from individual URIs. In effect, #13 does that merging already behind the scenes, just without a URI to identify it.

In summary, I think these URI issues are solvable, and we're better served by leaning on a more robust and widely used spec. @Jose and others: what do you think?

Gábor Hojtsy’s picture

@effulgentsia: well, so Dries pushed back on us because the schema system was duplicating things found elsewhere in Drupal and was looking too complex. That was the Drupal specific schema, which was still very simple. The Kwalify based schema is a bit of a jump for more complexity and the JSON schema seems to me yet another considerable jump up in complexity, so in the name of reusing industry standards we are getting further away from how I interpreted Dries' concerns. I think given the very limitd time we have and Dries' very specific feedback, we should consult him as to his strong feelings either way. Based on my recollection of the reaction from both webchick and Dries, even the way simpler schema was frightening for DX, so without their direct feedback I see little point in spending more time making it even more complex :/ :/ Can you help with getting that feedback?

Jose Reyero’s picture

@effulgentsia,
I don't see any real advantage in json schema, and it's really more complex. But again, if you think it's worth the work, you can create one more thread to research & develop it.
I think Kwalify-like is really more suited to our case, which is yaml, not json. However if you have specific suggestions borrowed from json schema, well, this is based on but doesn't need to be Kwalify, so open to hear that ones. However simply replacing tags like "properties instead of mapping", I don't think that really adds any value for us.

@Gábor Hojtsy,
Plans for this patch: Consolidate and simplify.
1. So far this has been some proof of concept of the schema being suitable and extendable, which doesn't mean we need to extend it too much in this patch, so we may drop some of the extended types here for the first stage to be shorter and easier to understand.
2. This one introduces too the 'translatable' flag and two translatable types (label, text). Not sure this is the place to introduce them, maybe they should wait for some follow up 'configuration translation' patch.
3. There are still some issues with the TypedData integration: Our typed data classes don't implement real validation as Kwalify does. We may want to drop that part (validations) for now.
4. About the basic types, in order to make this still simpler, maybe we should go for typed data names (string) which are already in Drupal instead of Kwalify ones (str) which given your concerns in #18 that make sense to me, we may not get too much value from real Kwalify compatibility (Also I haven't hear back from Kwalify's author).

Gábor Hojtsy’s picture

As a reality check and so I'm not putting words in Dries' mouth, here is what he stated on #1648930: Introduce configuration schema and use for translation a month ago:

I spent some more time on this patch. It's really hard and have been breaking my head on it for a while. After having spent 3+ hours on it, I still don't feel that I fully grok this approach. As a result, I'm struggling to provide good direction for the time being.

Here is my current stream of thoughts: I understand that this is important but I'm nervous about introducing a new custom file format that is pretty hard to grok. I still struggle with the fact that it only marginally improves the translator experience and that it is still an incomplete solution. I'm also not interested in having core automatically generate simple forms from the meta files; I don't want to introduce another form API, at least not in Drupal 8. It's too late for that.

It feels like we should have some time to continue to figure this out. I encourage people to explore simpler solution similar to what chx is working on (just fixing the translation regression) as well as effulgentsia (simplified file format). I think both are interesting evolvements that give us a point of comparison.

I'm not giving up on this issue. I'll continue to study this issue and will think about it more. In the mean time, I encourage people to continue to work on this issue and the other issues listed above.

Once again I think its *extremely important* we get his feedback ASAP on the kwalify format and especially on the JSON-schema format before we spend more time on this. He was vary of the complexity of the prior format which was much simpler. (Granted, he was also vary of it being custom to Drupal, so maybe a format that is more widely used could be tolerated more even if more complex). Anyway, all we are left now is guessing and we are making our approach more and more involved with each step, so I think its time we step back and get a Dries' view again. It is pointless to waste more time on this and get this refused as well.

webchick’s picture

Dries has some time on Thursday to review this, not sure about before.

Gábor Hojtsy’s picture

Assigned: Jose Reyero » Dries

Assigned to Dries for feedback.

webchick’s picture

For reference/comparison, here are the same examples in the issue summary from #1648930-298: Introduce configuration schema and use for translation. (Ignore the + signs)

modules/system/meta/system.site.yml

+name:
+  .label: 'Site name'
+  .type: text
+mail:
+  .label: 'Site mail'
+slogan:
+  .label: 'Site slogan'
+  .type: text
+page:
+  .label: 'Default pages'
+  403:
+    .label: 'Default 403 (access denied) page'
+  404:
+    .label: 'Default 404 (not found) page'
+  front:
+    .label: 'Default front page'

modules/system/meta/system.maintenance.yml

+enabled:
+  .label: 'Put site into maintenance mode'
+  .type: boolean
+message:
+  .label: 'Message to display when in maintenance mode'
+  .type: text

modules/image/meta/image.effect.image_crop.yml (one file per effect, others are very similar)

+width:
+  .label: 'Width'
+  .type: integer
+height:
+  .label: 'Height'
+  .type: integer
+anchor:
+  .label: 'Anchor'

modules/image/meta/image.style.%.yml

+name:
+  .label: 'Machine name'
+label:
+  .label: 'Label'
+  .type: text
+effects:
+  .label: 'Style effects'
+  .list:
+    .label: 'Image effect'
+    weight:
+      .label: 'Weight'
+      .type: integer
+    ieid:
+      .label: 'IEID'
+    name:
+      .label: 'Machine name'
+    data:
+      .include: 'image.effect.[%parent.name]'
+      .label: 'Data'
Dries’s picture

Assigned: Dries » Unassigned

After looking into this more, I'm comfortable to proceed with the Kwalify-based approach. I look forward to an RTBC patch. :)

Jose Reyero’s picture

Assigned: Unassigned » Jose Reyero

Thanks @Dries.

So we'll try to move forward with this one, assigning it back to me.
The plan is basically: to consolidate and simplify this code as outlined in #24

Gábor Hojtsy’s picture

1. So far this has been some proof of concept of the schema being suitable and extendable, which doesn't mean we need to extend it too much in this patch, so we may drop some of the extended types here for the first stage to be shorter and easier to understand.
2. This one introduces too the 'translatable' flag and two translatable types (label, text). Not sure this is the place to introduce them, maybe they should wait for some follow up 'configuration translation' patch.
3. There are still some issues with the TypedData integration: Our typed data classes don't implement real validation as Kwalify does. We may want to drop that part (validations) for now.
4. About the basic types, in order to make this still simpler, maybe we should go for typed data names (string) which are already in Drupal instead of Kwalify ones (str) which given your concerns in #18 that make sense to me, we may not get too much value from real Kwalify compatibility (Also I haven't hear back from Kwalify's author).

1. I'm not sure which extended types do you mean?

2. I think introducing that here would be good/important. We don't need to break this up even more. #1648930: Introduce configuration schema and use for translation had a pretty extensive scope with integration code in locale, etc. We'll need that too eventually, so not breaking things out more is useful for progress here IMHO. The current scope seems about fine.

3. @fago et.al. are still working on typed data validations, better not duplicate work => removal sounds good :)

4. I agree using Drupal type names would make this *much* simpler to adapt (using "custom" type names looked pretty bad for me); that would break direct kwalify compatibility but keep the kwalify spirit at least :)

attiks’s picture

@Gábor Hojtsy: just to be clear, you mean this issue isn't going to add any validation?

Gábor Hojtsy’s picture

@attiks: yes, validating CMi data files based on their schema would be out of scope for this patch IMHO. The feature freeze is in one month and we need to concentrate on the translation use case that we wanted to get this in for. Others can then work on other things.

effulgentsia’s picture

I agree with #33, but I would go even further and recommend that this issue not even add the translation use case to its scope, but simply clean up the basic API and implementation of the #13 patch. And then have a follow up focused on how best to integrate identification of translatability. Just my 2 cents as a way to keep each step easy to review and move through the core process quickly.

effulgentsia’s picture

To clarify, this is what I suggest removing from #13 and punting to a followup:

+++ b/core/lib/Drupal/Core/Config/Schema/Element.php
@@ -0,0 +1,52 @@
+  /**
+   * Checks whether this element is translatable.
+   *
+   * @return bool
+   */
+  public function isTranslatable() {
+    return !empty($this->definition['translatable']);
+  }
...
+++ b/core/modules/system/config/system.schema.yml
@@ -0,0 +1,101 @@
+# Simple scalar data types (Drupal)
+label:
+  type: str
+  label: 'Label'
+  length:     { max: 255, min: 0 }
+  translatable: true
+path:
+  type: str
+  label: 'Path'
+  class: '\Drupal\Core\TypedData\Type\String'
+text:
+  type: str
+  label: 'Text'
+  translatable: true
+weight:  ¶
+  type: int
+  label: 'Weight'
+  range: { max: 255, min: -255 }

But that's just a suggestion: I leave to Gabor and Jose to decide if you like it.

Gábor Hojtsy’s picture

Title: Introduce Kwalify-based schema format for configuration » Introduce Kwalify-inspired schema format for configuration

Based on changes discussed above "Kwalify-inspired" is a much more correct explanation for the proposed changes. Proper references help manage expectations.

heyrocker’s picture

Issue tags: +Configuration system
heyrocker’s picture

I had somehow gotten unfollowed from this issue so I'm just catching up here. I agree with Gabor in #31 about the scope of what we should and should not approach here. I'm also pretty happy with this format, I personally find it easier to read than prior attempts. About to get on a flight but will be reviewing this soon.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
40.72 KB
PASSED: [[SimpleTest]]: [MySQL] 50,717 pass(es). View

Changes:
Replaced generic name placeholder '%' by '*'
Replaced 'seq' by 'sequence' and 'map' by 'mapping'
Removed translatable property, reordered schema files.
Element extends ContextAwareTypedData instead of TypedData
Using type email
Renamed basic types from qualify names to typed data.

Jose Reyero’s picture

Assigned: Jose Reyero » Unassigned
FileSize
20.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config_schema-39.diff.patch. Unable to apply patch. See the log in the details link for more information. View

Interdiff for previous one.
Unassigning temporarly, pretty busy this week.

Status: Needs review » Needs work

The last submitted patch, config_schema-39.diff.patch, failed testing.

Gábor Hojtsy’s picture

#40 was not meant to go to testing I guess (it's an interdiff), so #39 is it. @heyrocker, @effulgentsia, others? Any concerns, suggestions, issues left?

Gábor Hojtsy’s picture

Also @Jose, can you update the issue summary to reflect the current format / API? :) That would help reviews a lot. Thanks!

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.99 KB
40.79 KB
PASSED: [[SimpleTest]]: [MySQL] 49,300 pass(es). View

Patch that:

  • Converts test to DrupalUnitTestBase - test now takes only 1 second
  • Changes "Definition of" to "Contains" (latest standards)
  • Fixes whitespace issues
heyrocker’s picture

Status: Needs review » Needs work
FileSize
73.05 KB

In the process of reviewing, created the attached class diagram in tribute to Gabor. Adding to summary as well. This is based on #44.

heyrocker’s picture

Issue summary: View changes

Updated issue summary.

heyrocker’s picture

Issue summary: View changes

Updated issue summary.

heyrocker’s picture

Issue summary: View changes

Adding object diagram

heyrocker’s picture

Issue summary: View changes

Updated issue summary.

heyrocker’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue summary: View changes

Updating for current schema type info and better formatted summary.

Gábor Hojtsy’s picture

Issue summary: View changes

Document API.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Updated issue summary with current schema types and config_typed() example. I think this looks good to go since Alex Pott cleared this up, Greg reviewed and no objections were raised :)

aspilicious’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
54.5 KB
FAILED: [[SimpleTest]]: [MySQL] 49,414 pass(es), 60 fail(s), and 29 exception(s). View
+++ b/core/lib/Drupal/Core/Config/Schema/Property.phpundefined
@@ -0,0 +1,22 @@
diff --git a/core/lib/Drupal/Core/Config/Schema/Schema.php b/core/lib/Drupal/Core/Config/Schema/Schema.php

diff --git a/core/lib/Drupal/Core/Config/Schema/Schema.php b/core/lib/Drupal/Core/Config/Schema/Schema.php
new file mode 100644

Empty file added ?

I didn't read everything but we added an empty file. As it didn't contain any documentation I presume we can remove it, so I did. If not we should document it inside the file :).

I also fixed 3 newline on end of file issues.

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI, -sprint, -language-config, -epic

The last submitted patch, 1866610.47.config-schema.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +D8MI, +sprint, +language-config, +epic

#47: 1866610.47.config-schema.patch queued for re-testing.

Gábor Hojtsy’s picture

Entity translation fails (due to permissions) seem totally unrelated.

Status: Needs review » Needs work

The last submitted patch, 1866610.47.config-schema.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
40.56 KB
PASSED: [[SimpleTest]]: [MySQL] 48,773 pass(es). View

The fails were indeed unrelated but were coming from the patch :D @aspilcious only mentioned things he did that should have made the patch a little bit smaller. It become 14k bigger :D There were accidentally added unrelated entity translation test stuff in there :D Removed those two files. Otherwise no changes.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All righto :)

Gábor Hojtsy’s picture

Assigned: Unassigned » Dries

This is among the issues blocking D8/D7 progress, so trying to expedite commit with assigning to Dries who already spent considerable time with this.

Gábor Hojtsy’s picture

#52: 1866610.47.config-schema.patch queued for re-testing.

webchick’s picture

The CMI YAML files + schema YAML files, for comparison.

core/modules/contact/config/contact.category.feedback.yml:

id: feedback
label: Website feedback
recipients: []
reply: ''
weight: '0'

core/modules/contact/config/contact.settings.yml:

default_category: feedback
flood:
  limit: '5'
  interval: '3600'
user_default_enabled: '1'

core/modules/contact/config/contact.schema.yml

# Configuration schema: contact.schema.yml

# Schema for contact category (multiple)
contact.category.*:
  type: mapping
  mapping:
    "id":
      type: string
    "label":
      type: string
    "recipients":
      type: sequence
      sequence:
        - type: email
    "reply":
      type: string
    "weight":
      type: integer

# Module settings
contact.settings:
  type: mapping
  mapping:
    "default_category":
      type: string
      required: yes
    "flood":
      type: mapping
      mapping:
        "limit":
          type: integer
        "interval":
          type: integer
      "user_default_enabled":
        type: boolean
Gábor Hojtsy’s picture

#52: 1866610.47.config-schema.patch queued for re-testing.

Dries’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Fixed

After many hours of looking into this, I decided to commit this. I can't think of a way to simplify this. Committed to 8.x. :)

larowlan’s picture

Title: Introduce Kwalify-inspired schema format for configuration » Needs Change notification: Introduce Kwalify-inspired schema format for configuration
Status: Fixed » Active
Issue tags: +Needs change record

Can we get a change notification for this one?
Thanks

plach’s picture

Title: Needs Change notification: Introduce Kwalify-inspired schema format for configuration » Change notifice: Introduce Kwalify-inspired schema format for configuration
Gábor Hojtsy’s picture

Title: Change notifice: Introduce Kwalify-inspired schema format for configuration » Introduce Kwalify-inspired schema format for configuration
Status: Active » Fixed

Added http://drupal.org/node/1905120 for change notice but the real meat is in the new docs at http://drupal.org/node/1905070 (which I just linked from there :).

YesCT’s picture

Issue tags: -Needs change record

looks like a really good start

Gábor Hojtsy’s picture

Opened #1905152: Integrate config schema with locale, so shipped configuration is translated to resurrect the translatable property that was removed late in the process here and resurrect all the missing functionality that was not added here but was part of #1648930: Introduce configuration schema and use for translation. Since that functionality had no debates in that issue either, I think its fine to leave the 350+ comments behind there too.

fago’s picture

Great to see this land - thanks to everyone who made that possible! :-)

I took a short look at it from a typed-data API perspective and opened #1905230: Improve the typed data API usage of configuration schema

Gábor Hojtsy’s picture

Issue tags: -sprint

Removing from sprint.

Gábor Hojtsy’s picture

Added a tracking META issue at #1910624: [META] Introduce and complete configuration schemas in all of core to track introducing and completing schemas for core. Opened the first issue for views. More issues need to be opened and wired in there. Also wrote this post to encourage people to help with conversions: http://hojtsy.hu/blog/2013-feb-07/learn-drupal-8-now-helping-make-it-mor...

Gábor Hojtsy’s picture

FYI (especially @effulgentsia), the Views team finds that having all schema descriptions in one file might be way too big to handle, discussing allowing more files to define schemas (but not necessarily one-by-one per CMI key / internal type). See comments on #1910606: Improve the configurations schemas for Views significantly.

Status: Fixed » Closed (fixed)

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

YesCT’s picture

Gábor Hojtsy’s picture

As indicated earlier the schema is becoming more complex with real life use. For example #1972816: Add support for %parent.%parent in config schema dynamic type names is submitted as a bug to introduce more complex dynamic type references to avoid schema repetitions.

Gábor Hojtsy’s picture

Issue summary: View changes

Fix code parser.

Leeteq’s picture