When confg->save() is called we call $this->sortByKey($this->data); this:

Ensures that re-inserted keys appear in the same location as before, in order to ensure an identical order regardless of storage controller. A consistent order is important for any storage that allows any kind of diff operation.

However is having some unforeseen consequences in #1735118: Convert Field API to CMI

If we create a text radio list with the following options:
Count field

Then the radios on the default setting screen will be:
Count radios

This definitely matters as the user should be able to define the list in whatever order they choose.

Looking at Zend config and Symfony config components neither of them change the key order. I think the current solution is like altering db_insert data using a hook which would be definitely be considered a bad idea. The config system should take arrays of data and save them - and do nothing to change the array. When a system requires this data again it should return it exactly as it was given.

Proposed solution

Remove config::sortByKey()

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Component: forum.module » configuration system
Issue tags: +Configuration system

Tagging...

swentel’s picture

Definitely a +1 from me. The CMI patch also had to change some existing asserts from assertIdentical to assertEqual to make the tests green. While that is not a criticial change, it still feels annoying.

alexpott’s picture

Patch removes the sortByKey function and changes the testDataKeySort to testDataKeyOrderPreservation.

Additionally had to change a config import test that imports config for ConfigTest config entites to have keys in the same order as the object properties. This highlights a potential issue with this patch - in that if the you import config and this process results in active config with a different key order than staging the config system will still think you have changes to make.

Maybe we should only sort root config keys but preserve order of an array contained within the config.

alexpott’s picture

Status: Active » Needs review

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 1785560_3_config_sort_order.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system

#3: 1785560_3_config_sort_order.patch queued for re-testing.

alexpott’s picture

The patch attached implements the suggestion that we should only sort root config keys but preserve order of an array contained within the config.

I think that this patch makes some sense as the order of the root keys belongs to Config but the data inside each of this keys including the order of an arrays really belongs to the module that uses it.

swentel’s picture

Makes even more sense, let's hope we don't a random failure now! :)

swentel’s picture

I used this patch on all Field API related tests where I reverted the 'hacks' in all simpletest files.

Without patch: 2363 passes, 13 fails
With patch: 2376 passes, 0 fails

So a big +1 from me. Won't RTBC it yet, guess we'll need more CMI people to look at this.

yched’s picture

I wonder if there would be a way to have alpha sorting for "variable-like collection of loosely related values" CMI files, but leave ordering completely up to the ConfigEntity for ConfigEntity files ?

I mean, if you take a "field instance" definition, the 'entity_type', 'bundle', 'field_name' keys are best grouped. The 'field_type' key is better off next to the 'settings' key. Much better for visual inspection. D7 exportables (views, panels...) would be tedious if ordering was strictly alphabetical.

But yeah, even for "flat collection of variables" files, an alpha order of 1st level keys makes sense, but shouldn't mess with the order within the values themselves if they happen to be arrays, since order may matter.

So :
- order 1st level keys alphabetically for non ConfigEntity files
- leave order completely up to the ConfigEntity for ConfigEntity files ?
Dunno if that's technically doable, though.

alexpott’s picture

After sleeping with patch from #7 in my head... I think we can justify the approach like this:

From the point of view of config storage (in some ways):

  • config objects are equivalent to database tables
  • config root / 1st level keys are equivalent to database columns
  • 2nd level arrays are just data

Therefore:

  • modules that use the config system should not care about the order of root / 1st level keys as this would be bad practice in SQL land too.
  • And the config system should not change the order of any data contained in a root / 1st level key because this would be like SQL changing data as you insert.
sun’s picture

Category: feature » task

The problem we have is that files are no longer identical (only same) without the sorting. This means you can get a bogus list of changes when staging/importing config.

Original issue: #1608912: (Re-)adding keys to configuration breaks the intended goal of being able to diff files

I'm inclined to agree with the case of Configurables mentioned in #10 — with regard to that, there's actually another unresolved issue:
#1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*

Therefore, one approach we could explore would be to combine the enforced clear() with the removal of key-sorting. That is, because the end result of doing both should technically save the keys in the order they are defined in a Configurable class and whenever the config file of a Configurable is re-saved/written, it is emptied out and written from scratch.

For static config files (settings), the order shouldn't really matter in the first place, I hope... Although there are use-cases like #1763754: [meta] Date and Time configuration, which potentially write arbitrary dynamic sub-keys (system.date:formats) into a static config file.

Lastly, closely related to the problem of potential diffs is also:
#1670370: Enforce writing of config changes during import, even if the module callback handled it already

yched’s picture

Category: task » feature

Well, precisely: in SQL, as the module who defines the table schema, you are allowed (and encouraged) to define your columns in an order that makes sense rather than alphabetically. The schema API doesn't decide of the order of my columns for me :-)

yched’s picture

Crosspost

alexpott’s picture

sun and yched and alexpott crossposted... :)

tim.plunkett’s picture

Category: feature » task
Issue tags: +VDC

If there ends up being consensus to remove it, can we at least make it opt-in or opt-on, and not just remove it? It helps bring sanity to views exports.

damiankloip’s picture

Here is our views issue linked to this: #1798026: Handler order is reset when a view is saved.

cosmicdreams’s picture

Issue tags: -Configuration system, -VDC

#7: 1785560_7_config_sort_order.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +VDC

The last submitted patch, 1785560_7_config_sort_order.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
1.46 KB

This is borrowed from the approach in #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config. It allows you to provide your own sorting callable, use the default, or opt-out altogether.

As such, it needs tests for providing your own sorting callable, and opting out :)

chx’s picture

Thanks for separating it out, I hoped it can just sneak in with the {system} removal, I still hope, if it doesn't, I can haz credit for it please?

tim.plunkett’s picture

FileSize
1.45 KB
+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -379,14 +386,18 @@ public function rename($new_name) {
+    return $value;

This is wrong, it should be $data. Typo on my part.

And yes, this is all chx's code.

sun’s picture

Status: Needs review » Needs work

No, we're walking in the utterly wrong direction.

Let's turn #12 into a patch (total removal) and combine it with the patch in #2 of #1601168-2: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*.

We'll see whether there may be any problems with that in staging scenarios. At least we'll have an actual and solid reason for re-considering any sorting -- if that happens at all.

This essentially means that the caller decides in which order keys are stored. Strict I/O, no magic.

chx’s picture

$config->save('sortme') is way shorter than $config->setData(sortme($config->get())) and much more readable too.

chx’s picture

Removing the default sorting from the patch and the sortByKey method completely is fine by me. Edit: no magic, indeed.

sun’s picture

Status: Needs work » Needs review
FileSize
4.68 KB

Attached patch implements #23:

1) Total removal of Config::sortByKey().

2) ConfigStorageController empties out the entire config object when saving a ConfigEntity.

3) Tests are adjusted to match these new expectations.

Status: Needs review » Needs work

The last submitted patch, drupal8.config-sort.24.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

This seems to suit VDC's use case. The issue @damiankloip linked passes with this also applied.

tim.plunkett’s picture

Status: Needs review » Needs work

Cross-post with the bot. Though that's an interesting failure.

sun’s picture

Status: Needs work » Needs review
FileSize
5.49 KB

Attached patch fixes that test failure.

chx’s picture

> $config->save('sortme') is way shorter than $config->setData(sortme($config->get())) and much more readable too.

Was this comment completely ignored? Any solution offered?

sun’s picture

Yes and no, it was too short and cryptic; impossible to make sense of such comments.

I'm strongly opposed to overloading ::save() with unrelated functionality. The sorting was a built-in operation previously, that's different.

The other problem is that it doesn't suffice for all possible sorting scenarios that people will reasonably have and run into.

What I can offer is $config->sort($key, $callback).

chx’s picture

#32 does not even work for the module case. I have posted a new version in #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config which is not even about sorts, just adds a call method so that you can add an arbitrary callback and keep chaining. And, it doesn't change get to a reference.

chx’s picture

  /**
   * Calls a PHP callable with the current data.
   *
   * This allows adding an arbitrary callback to a chain of config method
   * calls.
   *
   * @param callable $callable
   *
   * @return Drupal\Core\Config\Config
   */
  public function call($callable) {
    $this->data = $callable($this->data);
    return $this;
  }

Simple as that.

chx’s picture

Status: Needs review » Postponed

This should not hold up #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config but should be a followup to it and as they conflict, this is the smaller and easier to reroll. (Especially that an agreed upon solution doesnt yet even exist.)

sun’s picture

Status: Postponed » Needs review
FileSize
5.49 KB

I hope that ::call() method was a joke.

Let's go back to #30 and remove the sorting altogether. If anyone needs to any special sorting, or apply a "callback" to whatever, do it yourself.

This fixes the OP and thus helps the field and other conversion issues.

The new ::sort() method looked handy to me, but we should move that into a new issue and only do it when there are sufficient use-cases for it.

damiankloip’s picture

Yes, I think +1 for this, patch looks good. I will see what others think aswell :)

sun’s picture

#36: drupal8.config-sort.30.patch queued for re-testing.

andypost’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -274,6 +274,10 @@ public function save(EntityInterface $entity) {
     $this->preSave($entity);
     $this->invokeHook('presave', $entity);
 
+    // Clear out any possibly existing keys in an existing configuration object,
+    // so any potentially stale keys are removed.
+    $config->clear();

Let's discuss this change in #1601168-10: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*
Not sure that default implementation should clean-up values that could be added by the hook_presave()

sun’s picture

Status: Needs work » Needs review

I responded over there.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Let's get this done.

cosmicdreams’s picture

I'll test this tonight with the work I've been doing for date formats.

I read through the patch and it looked fine to me.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

This patch seems to be killing kittens.

It seems like the consensus reached here was to remove the sorting operation altogether, in favour of config-providing modules handling this themselves prior to calling $config->save(). That makes sense to me.

What doesn't make sense is why we're also putting orphaned logic here, especially when #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever* does not seem to have consensus around it.

Let's get a patch with just the sort removal, then I'm happy to commit it.

sun’s picture

Status: Needs work » Needs review

I can see how that might be confusing. It was explained in #12.

The essence:

The problem we have is that files are no longer identical (only same) without the sorting. This means you can get a bogus list of changes when staging/importing config.

Since keys rarely change in "static" config objects (i.e., module settings), we're making the assumption that removing the sorting won't cause troubles with those. However, "dynamic" config objects (i.e., all Configurables) are changing often and potentially fast, including their keys and sub-keys within. If they are not sorted anymore, it's guaranteed that will cause havoc. Therefore, we clear out any previously existing keys in the config object before setting/writing the new keys/data for config objects of Configurables.

alexpott’s picture

So I've talked to Sun about this on skype. So I'm going to try to justify why the clear() on configentity save belongs in this patch.

We are all agreed that the sort on save should die.

When we do not sort during a save configentity properties will be saved in the order they are declared in the object. I think this makes a great deal of sense. The class and the config YML match. Sorting was messing with this ... and so with this patch the order is in the hands of the configentity owner.

However, now that this is the case, we need to ensure that the class and the config YML match and (to quote Sun)

that can only be achieved and ensured by clearing out the config object before writing the new ConfigEntity property data

This is especially relevant during config import since (again from Sun)

config import mechanism cares about the key order. It ensures that input and output are identical.

Therefore I'm setting this patch back to RTBC since it still applies to HEAD and will request a retest.

alexpott’s picture

Issue tags: -Needs tests

Removing the "needs tests" since this applied to a much more complicated version of this patch"

alexpott’s picture

#36: drupal8.config-sort.30.patch queued for re-testing.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Since patch still passes and for reasons explained #45... setting back to rtbc

andypost’s picture

@alexpott I understand that configEntity should save only it's own properties but that's why we have #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*
I don't think we need to mix-up both issues in the same patch

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

"we need to ensure that the class and the config YML match"

Ok, let's get some tests for this, then. Then we could run them without the orphaned hunks of this patch and prove sun's assertion is correct, and the orphaned parts of this patch are actually required. Because otherwise it still sounds pretty theoretical to me, and I really don't want to hold up this entire simple patch on consensus building around how to properly deal with orphaned properties.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
4.74 KB

Here's a patch without this hunk

andypost’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Cross-post

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.76 KB
3.23 KB

#51 shows that we already have a test for orphaned key :) ... anyhow I really want to unblock the system list patch and the field CMI patch and its important that none of the other CMI patches rely on the in-built sort on save functionality so I propose that we commit a re-roll of #3 and move on.

#1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever* can deal with the orphan issue as far as I'm concerned. We don't have any committed config entities yet so I can not argue with @webchick's comment that at this point the config->clear() on configentity save is theoretical.

Status: Needs review » Needs work

The last submitted patch, 1785560_53.drupal8.config_sort_order.patch, failed testing.

alexpott’s picture

#53 failed due to a test that passes locally. Requesting re-test...

Drupal\system\Tests\File\RemoteFileUnmanagedDeleteRecursiveTest

The test did not complete due to a fatal error. Completion check UnmanagedDeleteRecursiveTest.php 68 Drupal\system\Tests\File\RemoteFileUnmanagedDeleteRecursiveTest->testSubDirectory()

alexpott’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Reviewed & tested by the community

So this patch essentially rolls back the patch from #1608912: (Re-)adding keys to configuration breaks the intended goal of being able to diff files (speaking of, the test method could be removed entirely, it doesn't test anything but native PHP array index association behavior.)

The fix from that issue had the following effect:

  1. All keys in the entire config object are re-sorted on every save.
  2. Sorting removed the "feature" of native array index associations. It inherently required us to add weight: 0 sub-nodes for collections that have an explicit, defined order.
  3. At the same time, ensuring an always consistent key order was a key enabler for diff operations between config objects, allowing you to see the actual change instead of a node/collection being removed at one position and re-inserted in a different position.

This is just to explain the current code/architecture and reasoning behind it, so we understand what we're changing here. I'm all-in for removing the sorting.

However, I think we should re-open #1608912: (Re-)adding keys to configuration breaks the intended goal of being able to diff files then, because the issue is essentially not fixed anymore (the exact patch over there is rolled back), and we should probably try to find an alternate solution for the issue then, as I believe that the ability to produce clean diffs for config objects is a fundamental facility, which not only affects developers working with $VCS but also a potential Config UI (e.g., Features module had this capability since the very beginning).

One alternate solution is to "re-instantiate" a "supposed to be same" key order in config objects by clearing them out altogether before writing new data into them; i.e., #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*. That is, however, for dynamic config objects (Configurables) only, since only those have an additional business logic layer wrapped on top of them, which is supposed to be used for any updates to a corresponding config object; having the resulting impact of always storing config object keys in the order the properties are defined on a classed ConfigEntity object.

I feel uncomfortable with not addressing the resulting consequences of this patch here, but I'm happy with unblocking other issues and moving forward. For this compromise, I'd want to increase the priority of #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*, but alas, it's filed as a major bug already.

The only possible tweak to this patch before commit would be to remove the test method entirely. We can still do that, unless a core maintainer commits this faster than we can. :)

andypost’s picture

So let's get #53 in, and continue with #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*

EDIT: crossposted
- for plain config file we can preserve an order with original file lines
-for config entities order of keys are declared within class

sun’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x. Thanks!

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