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:
Then the radios on the default setting screen will be:
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()
Comment | File | Size | Author |
---|---|---|---|
#59 | drupal8.config-sort-remove.59.patch | 3.42 KB | sun |
#53 | 1785560_53.drupal8.config_sort_order.patch | 3.23 KB | alexpott |
#53 | 30-53-interdiff.txt | 3.76 KB | alexpott |
#51 | drupal8.config-sort.50.patch | 4.74 KB | andypost |
#36 | drupal8.config-sort.30.patch | 5.49 KB | sun |
Comments
Comment #1
alexpottTagging...
Comment #2
swentel CreditAttribution: swentel commentedDefinitely a +1 from me. The CMI patch also had to change some existing asserts from
assertIdentical
toassertEqual
to make the tests green. While that is not a criticial change, it still feels annoying.Comment #3
alexpottPatch removes the
sortByKey
function and changes thetestDataKeySort
totestDataKeyOrderPreservation
.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.
Comment #4
alexpottComment #6
swentel CreditAttribution: swentel commented#3: 1785560_3_config_sort_order.patch queued for re-testing.
Comment #7
alexpottThe 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.
Comment #8
swentel CreditAttribution: swentel commentedMakes even more sense, let's hope we don't a random failure now! :)
Comment #9
swentel CreditAttribution: swentel commentedI 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.
Comment #10
yched CreditAttribution: yched commentedI 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.
Comment #11
alexpottAfter 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):
Therefore:
Comment #12
sunThe 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
Comment #13
yched CreditAttribution: yched commentedWell, 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 :-)
Comment #14
yched CreditAttribution: yched commentedCrosspost
Comment #15
alexpottsun and yched and alexpott crossposted... :)
Comment #16
tim.plunkettIf 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.
Comment #17
damiankloip CreditAttribution: damiankloip commentedHere is our views issue linked to this: #1798026: Handler order is reset when a view is saved.
Comment #18
cosmicdreams CreditAttribution: cosmicdreams commented#7: 1785560_7_config_sort_order.patch queued for re-testing.
Comment #20
tim.plunkettThis 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 :)
Comment #21
chx CreditAttribution: chx commentedThanks 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?
Comment #22
tim.plunkettThis is wrong, it should be $data. Typo on my part.
And yes, this is all chx's code.
Comment #23
sunNo, 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.
Comment #24
chx CreditAttribution: chx commented$config->save('sortme') is way shorter than $config->setData(sortme($config->get())) and much more readable too.
Comment #25
chx CreditAttribution: chx commentedRemoving the default sorting from the patch and the sortByKey method completely is fine by me. Edit: no magic, indeed.
Comment #26
sunAttached 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.
Comment #28
tim.plunkettThis seems to suit VDC's use case. The issue @damiankloip linked passes with this also applied.
Comment #29
tim.plunkettCross-post with the bot. Though that's an interesting failure.
Comment #30
sunAttached patch fixes that test failure.
Comment #31
chx CreditAttribution: chx commented> $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?
Comment #32
sunYes 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)
.Comment #33
chx CreditAttribution: chx commented#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.
Comment #34
chx CreditAttribution: chx commentedSimple as that.
Comment #35
chx CreditAttribution: chx commentedThis 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.)
Comment #36
sunI 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.
Comment #37
damiankloip CreditAttribution: damiankloip commentedYes, I think +1 for this, patch looks good. I will see what others think aswell :)
Comment #38
sun#36: drupal8.config-sort.30.patch queued for re-testing.
Comment #39
andypostLet'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()
Comment #40
sunI responded over there.
Comment #41
alexpottLooks good. Let's get this done.
Comment #42
cosmicdreams CreditAttribution: cosmicdreams commentedI'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.
Comment #43
webchickThis 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.
Comment #44
sunI can see how that might be confusing. It was explained in #12.
The essence:
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.
Comment #45
alexpottSo 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)
This is especially relevant during config import since (again from Sun)
Therefore I'm setting this patch back to RTBC since it still applies to HEAD and will request a retest.
Comment #46
alexpottRemoving the "needs tests" since this applied to a much more complicated version of this patch"
Comment #47
alexpott#36: drupal8.config-sort.30.patch queued for re-testing.
Comment #48
alexpottSince patch still passes and for reasons explained #45... setting back to rtbc
Comment #49
andypost@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
Comment #50
webchick"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.
Comment #51
andypostHere's a patch without this hunk
Comment #52
andypostCross-post
Comment #53
alexpott#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.
Comment #55
alexpott#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()
Comment #56
alexpott#53: 1785560_53.drupal8.config_sort_order.patch queued for re-testing.
Comment #57
sunSo 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:
weight: 0
sub-nodes for collections that have an explicit, defined order.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. :)
Comment #58
andypostSo 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
Comment #59
sunFull revert of #1608912: (Re-)adding keys to configuration breaks the intended goal of being able to diff files
As explained in #57, the test method is obsolete.
Comment #60
catchCommitted/pushed to 8.x. Thanks!