Problem/Motivation

Configuration object wonderful.settings should be able to be like:

node:
  article:
    status: TRUE
    wonderful: FALSE
  page:
    status: FALSE
    wonderful: FALSE
user:
  user:
    status: TRUE
    wonderful: TRUE

and schema:

wonderful.settings:
  type: sequence
  label: 'Entity type'
  sequence:
    - type: sequence
      label: 'Bundle'
      sequence:
        - type: mapping
          label: 'Wondeful settings'
          mapping:
            status:
              type: boolean
              label: 'Status'
            wonderful:
              type: boolean
              label: 'Is it wonderful?'

Proposed resolution

Fix it so the above works. Otherwise we have to add a key like this:

entities:
  node:
    article:
      status: TRUE
      wonderful: FALSE
    page:
      status: FALSE
      wonderful: FALSE
  user:
    user:
      status: TRUE
      wonderful: TRUE

and schema:

wonderful.settings:
  type: mapping
  label: 'Entities'
  mapping:
    entities:
    type: sequence
    label: 'Entity type'
    sequence:
      - type: sequence
        label: 'Bundle'
        sequence:
          - type: mapping
            label: 'Wondeful settings'
            mapping:
              status:
                type: boolean
                label: 'Status'
              wonderful:
                type: boolean
                label: 'Is it wonderful?'

Remaining tasks

Review patch.

User interface changes

none

API changes

none

Files: 
CommentFileSizeAuthor
#2 1-2-interdiff.txt4.32 KBalexpott
#2 2248709.2.patch5.98 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2248709.2.patch. Unable to apply patch. See the log in the details link for more information. View
#1 2248709.test-only.1.patch1.84 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,571 pass(es), 6 fail(s), and 6 exception(s). View

Comments

alexpott’s picture

Status: Active » Needs work
FileSize
1.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,571 pass(es), 6 fail(s), and 6 exception(s). View

Here's a patch that makes Drupal\config\Tests\DefaultConfigTest fail - got to love that test!

alexpott’s picture

Issue summary: View changes
FileSize
5.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2248709.2.patch. Unable to apply patch. See the log in the details link for more information. View
4.32 KB

Working patch attached. Fixes this by pushing Mapping::get() up to ArrayElement::get() so that both Sequence and Mapping can sharing the ability to traverse config files if they are the root type. The documentation sets out the interesting interface / inheritance going on :)

Leaving at needs work because testbot is borked.

sun’s picture

I'm confused. The summary talks about a sequence, but the example shows a mapping.

A sequence:

key:
  - value1
  - value2
  - ...

A mapping:

key:
  subkey1: value1
  subkey2: value2
  ...

A mapping already works at the top-level.

A top-level sequence does not make sense to me. Much rather, it's a sign that configuration objects might be structured too granularly and the sequence should actually be located and merged into another object.

alexpott’s picture

Status: Needs work » Needs review

Bots are back

alexpott’s picture

@sun a mapping is where you know every key - if the top level is a list of entities available and then the next key are their bundles and then there is mapping then the top two levels are sequences - see #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration for an example of this. Or even rest.settings - the top level resources key is completely superfluous the only reason it exists is because schema can not traverse config properly if the top level is a sequence.

The last submitted patch, 1: 2248709.test-only.1.patch, failed testing.

sun’s picture

It's a double-edged sword:

By allowing this, we essentially assume that developers have a solid plan about their config requirements ahead of time and for the entire future. — Once you put a sequence at the top-level of (in particular) a $module.settings config object, it will not be safe to add a new top-level key to the config + schema later on. As in the examples here, the sequence key names can be any string that possibly exists.

I doubt that even core has enough of a plan to be really sure. It's perfectly possible that rest.settings needs some additional settings at some point later on. At that point, you either need a more complex module update or migration process, or you're forced to create and use a separate config object, which would have been unnecessary if there wasn't a sequence at the top-level and which is bad for performance.

Even though it could be supported on technical grounds, it is good that we don't support it in my book, since it prevents people from running into a major trap.

alexpott’s picture

@sun but otoh if a developer knows that the want to the root type to a sequence they can do this - and config will work just fine - until you try to create a schema for it. So saying we don't support it is not true.

A mapping is just a special form of sequence - in terms of the data structure they are both arrays.

alexpott’s picture

For example: content_translation.settings in HEAD works this way already. The top level key is a sequence and this only works because no one has tried to write a schema for it.

sun’s picture

Great, so we already have the problem in HEAD:

What if a necessary change for Content Translation module has to add a setting to content_translation.settings?

We'll either have to rewrite content_translation.settings to use a sub-key for the sequence (API-breaking change), or we will have to use a new config object separate from content_translation.settings, even though generic module settings are supposed to live in a $module.settings config object and even though the entire problem could have been avoided by not using a sequence at the top-level in the first place.

I understand that it's technically possible, but not everything that is technically possible is a good idea. :-)

alexpott’s picture

Fair enough but what if the configuration object was called content_translation.entities

alexpott’s picture

Basically, I just don't get why we just think we know best and restrict people in this way. Especially when they're not actually restricted.

sun’s picture

Right, that's why I said that it's a double-edged sword:

If we do this, then there's a good chance that many developers will run into the trap.

If we don't do this, then we're essentially baby-sitting developers. (and yes, without an explicit protection/exception in the existing config schema validation in Config::save(), that baby-sitting is not complete)

IMHO, baby-sitting is a good idea here, since it's going to be exceptionally hard to recover from such a mistake later on.

alexpott’s picture

But there is no way of baby sitting developers here - in data terms there is not difference between a keyed sequence and a map. Unless we require schema. And people have been against that forever.

alexpott’s picture

Also I think it our job to design robust APIs that make it difficult break Drupal and easy to use. I don't think it is our job to prevent people making mistakes because (a) that is our opinion - do we really know better? (b) can lead to interesting new ideas (c) is it really the Drupal way to limit what people can do?

Gábor Hojtsy’s picture

We don't require schema, but its true that some schema limitations mandate some data structures if you do use schema, eg. a subkey with an identifier value to be used to identify dynamic data types (eg. views plugin ids). I don't have a strong feeling either way whether we should babysit shortsighted developers not using a mapping at the top. I think shortsighted developers will not care about schemas in the first place, and then config is a free form tree, so they will do whatever they like. So not really concerned that we could box them in somehow with an optional system...

Actual code review:

  1. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
    @@ -114,4 +115,40 @@ public function getIterator() {
    +   * All configuration objects at their root are either of the type
    +   * sequence or mapping which both extend ArrayElement. Enable traversal of the
    +   * schema by excepting a dot delimited key to access nested values, for
    +   * example, 'page.front'.
    

    The first sentence is about config objects, the second is about the method which is very confusing. I only figured this out by reading the patch.

  2. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
    @@ -114,4 +115,40 @@ public function getIterator() {
    +   * This implements the get() method on the follow schema classes:
    

    following?

  3. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
    @@ -114,4 +115,40 @@ public function getIterator() {
    +   * To conform with the respective interfaces:
    +   * - \Drupal\Core\TypedData\ComplexDataInterface
    +   * - \Drupal\Core\TypedData\ListInterface
    

    This is confusing, why not implement that interface?

  4. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
    @@ -114,4 +115,40 @@ public function getIterator() {
    +      throw new SchemaIncompleteException(String::format("The configuration property @key doesn't exist.", array('@key' => $property_name)));
    ...
    +        throw new SchemaIncompleteException(String::format("The configuration property @key does not exist.", array('@key' => $property_name)));
    

    The two messages could use the same English. I know this is pre-existing in the moved code, but since we are doing this...

Gábor Hojtsy’s picture

BTW I looked at config translation and there does not seem to be an assumption about a top level mapping implemented/hardcoded there.

anavarre queued 2: 2248709.2.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2248709.2.patch, failed testing.

Gábor Hojtsy’s picture

#2212069: Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated practically mandates that config objects are sequencesmappings, it will add a langcode key on top level config objects. If they are defined as a sequence and the top level type is not a string, that will break schema validation.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.