Config export key order is not predictable for sequences, add orderby property to config schema

Closed #2352705: [META] Introduce explicit order in config files in favor of this issue.

#2227731: Normalize configuration data during config writes is required to sort top level schema keys.

What we do when the order of itens is important and must not change ?

For user role permissions we have a patch: https://www.drupal.org/node/2409129

First attempt.

Rerolling

Rerolling again.

Corrected.

Removing sprint weekend tag!!
As suggested by @YesCT

@deepakaryan1988 The tag was added January 17, 2015 during the actual global sprint weekend. See https://groups.drupal.org/node/447258
Please leave the tag on issues that were actually worked on during that event.

I think you might have confused a message from May about the sprint weekend tag, that it should not be added to events from May. The tag is for issues worked on during https://groups.drupal.org/node/447258 by people attending sprint weekend sprints.

@YesCT sorry for this!

Surely this should be Needs Work as there is no patch passing tests?

You can always mark it needs work yourself.
And don't call me Shirley.

This came up again in #2850601: ContentModeration workflow type plugin incorrectly preserves bundle keys on sorting and does not sort entity types. I think this is one of the most common kinds of bugs in config implementations and very easy to get wrong. And even after it's fixed, fixing it still can require upgrade paths and add noise to people's config diffs. So definitely agree that this is a major bug.

Here's an approach that is more storage focused but less of an API change than @webflo's previous work. This is entirely additive.

Re-scoping the issue to just deal with sequences - will file a followup to tackle Mapping. This is much more complex because we probably should follow the order as defined by schema - BUT this could result in massive change. Luckily root keys in config entity schema already do this (i think).

Added follow-up for maps #2852557: Config export key order is not predictable, use config schema to order keys for maps

Added CR https://www.drupal.org/node/2852566

Some more test coverage just to ensure we don't mess anything up when the sequence contains something different from scalar values.

Having this is for sequences is a huge step forward. Test coverage looks good.

1. +++ b/core/lib/Drupal/Core/Config/Schema/Sequence.php
   @@ -16,6 +18,17 @@ class Sequence extends ArrayElement {
   +    if (!isset($this->definition['orderby'])) {
   +      $this->definition['orderby'] = 'none';
   +    }
   

   I'm not convinced about setting this default here. In D9 we should pick a default - either key or value (probably value) but supporting none seems unnecessary. All config sequences should be sorted. We might need to add more complex sorts - ie by a value in a mapping contained with the sequence but none is not really an option.

2. +++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
   @@ -210,6 +211,18 @@ protected function castValue($key, $value) {
   +        switch ($element->getDataDefinition()['orderby']) {
   

   Array access to data definition is deprecated - let's add a getter to the Sequence class.

Thinking about how we can avoid using the ArrayAccess BC layer - some related issues: #1928868: Typed config incorrectly implements Typed Data interfaces and #2350597: Extract a common DataContainerInterface for lists and complex data

Addressing #47 and #48. As far as I can see this approach does not cause any issues the issues linked in #48.

Filed a followup to use orderby in all of core - #2855675: Add orderby key to all sequences in core

Using the additional class makes sense. Its a way for a better future.

+++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
@@ -396,6 +396,76 @@ public function testConfigSaveWithSchema() {
+    // Tests sorts do never destroy complex values.

s/do//

Fixed #52 - thanks @quietone

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -46,7 +46,7 @@ mapping:
   -  definition_class: '\Drupal\Core\TypedData\ListDataDefinition'
   +  definition_class: '\Drupal\Core\Config\Schema\SequenceDataDefinition'
   

   I did a double-take at this hunk, but of course SequenceDataDefinition is extending ListDataDefinition, so all fine there.

2. +++ b/core/lib/Drupal/Core/Config/Schema/Sequence.php
   @@ -29,4 +29,20 @@ protected function getElementDefinition($key) {
   +   *   Describes how the sequence should be sorted. Either 'value', 'key' or
   +   *   NULL. NULL means that the schema does not describe how the sequence
   
   +++ b/core/lib/Drupal/Core/Config/Schema/SequenceDataDefinition.php
   @@ -0,0 +1,24 @@
   +   *   Describes how the sequence should be sorted. Either 'value', 'key' or
   +   *   NULL. NULL means that the schema does not describe how the sequence
   

   "Either" can't be used with a list of three things. For that matter, the sentence does not have a subject or a verb. I can fix this on commit. This would work:

   May be 'value' (to sort by value), 'key' (to sort by key), or NULL (if the schema does not describe how the sequence should be sorted).

   I hadn't actually expected a value sort would be supported, but it sounds from the issue comments that there are usecases for that. I also briefly asked myself (as I do with any integer or string parameter values with special meaning) whether these should be constants, but since they also are string values that get added to the schemata, it's clearer to just use the strings.

3. +++ b/core/lib/Drupal/Core/Config/Schema/SequenceDataDefinition.php
   @@ -0,0 +1,24 @@
   + * A typed data definition class for defining sequences in configuration.
   

   This doesn't follow our verb-first standard, but I checked and the class it's extending from doesn't either, so eh.

4. +++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
   @@ -210,6 +211,18 @@ protected function castValue($key, $value) {
   +          case 'key':
   +            ksort($value);
   +            break;
   +          case 'value':
   +            sort($value);
   +            break;
   

   Pretty sure our standards require an empty line after a break statement. I can fix this on commit.

5. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -396,6 +396,76 @@ public function testConfigSaveWithSchema() {
   +   * Test configuration sequence sorting using schemas.
   

   Tests.

6. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -396,6 +396,76 @@ public function testConfigSaveWithSchema() {
   +    // Value sort does not preserve keys - this is intentional.
   +    $data = [
   +      'value_sort' => [1 => 'b', 2 => 'a'],
   +      'no_sort' => [1 => 'b', 2 => 'a'],
   +    ];
   

   Ah, interesting. So this would be to avoid the scenario where in that Workflow issue some serial keys got saved to the config weirdly?

7. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -396,6 +396,76 @@ public function testConfigSaveWithSchema() {
   +      'complex_sort_value' => ['b' => ['foo' => '1', 'bar' => '1'] , 'a' => ['foo' => '2', 'bar' => '2']],
   +      'complex_sort_key' => [['foo' => 'b', 'bar' => 'b'] , ['foo' => 'a', 'bar' => 'a']],
   ...
   +    $this->assertSame([['foo' => '1', 'bar' => '1'], ['foo' => '2', 'bar' => '2']], $this->config('config_schema_test.schema_sequence_sort')->get('complex_sort_value'));
   +    $this->assertSame([['foo' => 'b', 'bar' => 'b'], ['foo' => 'a', 'bar' => 'a']], $this->config('config_schema_test.schema_sequence_sort')->get('complex_sort_key'));
   

   Ah wow. Not only does the value sort not preserve serial keys, it does not preserve any keys at all, even strings keys. I did not expect that. (Edit: removed nonsensical sentence; the patch uses plain old sort()). Anyway, I think we definitely need to document that because otherwise this could lead to data loss if people don't realize the value sort is intended to not preserve keys.

NW for last point; nits could be fixed too.

Based on point 7, I'd rewrite point 2 as:

May be 'key' (to sort by key), 'value' (to sort by value, discarding keys), or NULL (if the schema does not describe how the sequence should be sorted). Only the top level of the array is sorted. Top-level keys are discarded when using 'value' sorting.

This issue reminded me of why I'd come to avoid using sort(). The PHP docs say:

Be careful when sorting arrays with mixed types values because sort() can produce unpredictable results.

I realized I don't actually know if that means "unexpected, but consistent" or "possibly non-deterministic". I'd hope the former, in which case I guess it may not matter because what does matter regardless is that the order is consistent. But given that a schema can be mixed data types with various depths of mappings or flat mixed arrays together in the same, is this a concern?

Edit: Maybe a couple more test cases where the data types are mixed?

Changes from:
#54.2,.4,.5

Here is the new patch, but still we need more test cases.

Maybe a couple more test cases where the data types are mixed?

NR for other things.

Sorry, NW was set before i submitted the post :( Setting it to NR

Re value sorting and not preserving keys. This behaviour is vital. If we preserved keys then we'd get lots and lots of arrays where the numeric keys had been converted to strings and the order in which you configured things would matter. Basically breaking the entire point of this change. Unfortunately PHP does not make it easy to determine when an array is associative or not. I considered added a non destructive value sort but rejected this as over-engineering. If you care about the keys do a key sort. If you have a true non-associative array do a value sort.

Also we sequences we're lucky re the type thing - we are guaranteed that all the values are of the same type because it is defined in the sequence and enforced in \Drupal\Core\Config\StorableConfigBase::castValue() before we do any sorting - so fortunately we don't have to worry about that. And I don't think we need more tests because we can't actually mix the data types.

Re #54.3 Maybe we need to fix our coding standard - verbs just don't work for 90% of our class doc blocks. Classes tend to be nouns.

I realised we can do without the method duplication on Sequence and SequenceDataDefinition.

@himanshu-dixit sorry I didn't build on your patch but it also made a breaking change to core/config/schema/core.data_types.schema.yml #54.1 wasn't saying the change was wrong - just that it was surprising to @xjm.

Updated the CR to be more explicit about the associative / non-associative thing.

When this is committed we need to update https://www.drupal.org/docs/8/api/configuration-api/configuration-schema... and we probably ought to create a whole new page about sorting of configuration since it is a key topic wrt to reliable and sane config deployments.

Also we sequences we're lucky re the type thing - we are guaranteed that all the values are of the same type because it is defined in the sequence and enforced in \Drupal\Core\Config\StorableConfigBase::castValue() before we do any sorting

Can we add that to the docs (inline and/or docblock)? It's helpful information.

For #63, I meant maybe an inline comment actually directly above the sort() in the switch statement, since that's the kind of thing people might scan codebases for and try to "fix" etc. Not sure if it's worth mentioning in the docblock itself really.

Let's file a followup docs issue also for #62.

I discussed the possibility of making the orderby a callback where it'd perform a sort by calling the callback. Originally I thought we might just do this for everything so orderBy should take values of sort and ksort but I think there is still value in doing the simple declarative thing in schema. This means that an alternative system can validate config without having a running code base.

Not sorting callbacks means that if you have advanced cases like module_config_sort() I think these have to remain in code. Which, in turn, means we might have to reconsider the 'none' option. Or perhaps call it something other than 'none'. But we can defer these choices till we try to do #2855675: Add orderby key to all sequences in core because that'll highlight these issues. We know we're not wrong by implementing value and key sorts and it is nice that people don't have to think about which PHP sort function to call.

This looks really good now.

+++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
@@ -210,6 +212,27 @@ protected function castValue($key, $value) {
+              // The PHP documentation notes that "Be careful when sorting
+              // arrays with mixed types values because sort() can produce
+              // unpredictable results". Fortunately we have already cast all
+              // the values to the same type using configuration schema.

The word "Fortunately" is unnecessary and "we" did not do anything. I'd say instead:

There is no risk here because \Drupal\Core\Config\StorableConfigBase::castValue() has already cast all values to the same type using the configuration schema.

I can update that on commit, but won't probably commit until tomorrow. @himanshu-dixit, maybe you could make that update as well? I think it's okay to leave it RTBC if you agree with the change since it's a small clarification rather than new documentation.

Here is the new patch updating documentation.

RTBC+

The docs followup issue for #62 did not seem to exist yet, so I filed #2857879: Update Configuration system documentation on sorting.

@yukare, regarding:

What we do when the order of itens is important and must not change ?

In that case your implementation should explicitly enforce the ordering that is correct on save. Followups for this might add support for custom orderby callbacks as well, but for now, it's best to pass in NULL so there is no sorting by the SequenceDataDefiniton and make sure your config is saved in a consistent order that will not accidentally change.

As an API addition, this goes into the development branch, so committed to 8.4.x. I also published the CR. Thanks everyone!

Attributing the contribution to GSoC.

+++ b/core/modules/config/tests/config_schema_test/config/schema/config_schema_test.schema.yml
@@ -297,3 +297,41 @@ test.double_brackets.breed:
+    complex_sort_value:
+      type: sequence
+      orderby: value
+      sequence:
+        - type: mapping
+          mapping:
+            foo:
+              type: string
+            bar:
+              type: string
+    complex_sort_key:
+      type: sequence
+      orderby: key
+      sequence:
+        - type: mapping
+          mapping:
+            foo:
+              type: string
+            bar:
+              type: string

I was coming here to ask why the behavior for non-primitive sequences was not defined. Because neither the CR nor the IS explain this. But I'm happy to see that there appears to be test coverage for that case :)

(Coming from #2874633: [PP-1] Update CDN module for Drupal 8.4.x: 'orderby' in config schema for sequences.)

Actually, there is an important question that is not answered by the CR or the IS, at least not AFAICT. Quoting #2874633-5: [PP-1] Update CDN module for Drupal 8.4.x: 'orderby' in config schema for sequences:

+++ b/config/schema/cdn.mapping.schema.yml
@@ -27,6 +27,7 @@ cdn.mapping.auto-balanced:
+      orderby: value

@@ -43,6 +44,7 @@ cdn.mapping.complex:
+      orderby: value

Actually, these should not be sorted/ordered… the order does matter in this case, and it cannot be ordered by key (there isn't any). In fact, the key is numerical, hence indicating the order already.

So these should be key, not value. I think that that must be the general rule for non-associative arrays where the order matters?

Reopening this issue to get an answer to that question documented in the CR, because more modules will struggle with that.

@Wim Leers hmmm this is a good question. Actually in this instance the best practice is to have a weight key and sort by something that does not change. This means that if things are re-ordered when you review the config change the only thing you have to review is the weight key changes. However maybe we need to add "in the order added" type of "sort" so at least the current behaviour of cdn could be documented and we could document that this is bad practice and recommend ways of fixing it.

However maybe we need to add "in the order added" type of "sort" so at least the current behaviour of cdn could be documented and we could document that this is bad practice and recommend ways of fixing it.

+1 to both of those.

By the way, an example of exactly the same problem/requirement can be found in ckeditor.schema.yml: the Toolbar configuration mapping contains a sequence of rows, which contains a sequence of buttons groups, which contains a sequence of buttons. None of them have "weight" or "priority"; the order is relevant there, too.

So this is not a problem that's exclusive to contrib. And probably there are even more like these in other places in core.

Moved #76—#80 into a follow-up issue: #2885368: Config export key order for sequences: "orderby" does not support cases where the order actually matters.

Was there a changelog created for this change? Also, I'm not seeing anything about an 'orderby' key in this documentation page: https://www.drupal.org/docs/8/api/configuration-api/configuration-schema...

Would someone familiar with this issue and its solution at the very least update the documentation page?

I see the property is already in use by the content_moderation module in /content_moderation/config/schema/content_moderation.schema.yml and it should be documented.