Config export key order is not predictable, use config schema to order keys for maps

FYI until we get to 8.4, you might be interested in this sandbox module workaround. It forces CM to compare configs deep-sorted, which eliminates this kind of false diff.

Before I try to reroll the last patch, is thi issue still relevant now that we have this ? https://www.drupal.org/node/2852566

Switching to official Vienna tag.

The module in #18 seems to address the particular issues we have been seeing with config diffs. Specifically, we have been seeing unexpected config differences with filter.format.* and search_api.index.* files, which is disconcerting to see post-deployment.

Although the module helps us get around this issue, it would be great to see it addressed in the core configuration system.

One thing worth mentioning about the module in #18 is that while it will prevent false diffs from showing up in drush and the UI, the relocated configuration keys will still be relocated during exports as the module doesn't prevent functionally identical configuration entities from showing up in exports.

While we considered using the module in our codebase, we ultimately did not because of this limitation.

@bobbygryzynger If you're problem is about the order of the keys inside the "filters" key in filter.format.*, it's not a mapping but a sequence. Then the right issue is #2855675: Add orderby key to all sequences in core. While waiting for this issue to be fixed, you can add this simple hook implementation to a custom module:

/**
 * Implements hook_config_schema_info_alter().
 */
function my_module_config_schema_info_alter(&$definitions) {
  // Make sure that the order of filters in the config export file is always
  // the same, to avoid "false positive" differences.
  $definitions['filter.format.*']['mapping']['filters']['orderby'] = 'key';
}

Then, if you save your filter format again and export the config, you'll get the filters in alphabetical order in the export file.
The same kind of trick might work for search_api.index.* if it's also a sequence problem.

Well done @GaëlG! #28 absolutely saved my project!

Linking in the issue summary to contrib module code that tries to address config sorting/normalization and that might be reduced/replaced by a solution in core.

first attempt at re-rolling this.
There is one migrate test I didn't know what to do for, I guess it will come back here.
Most likely newer tests will also fail.
Lets see what the testbot thinks about it...

I used several commits for the re-roll because I had to update 383 default and optional config files and then the real changes would be lost. (like it was for me when going through the patch from 2017 which didn't apply at all any more)

Adding Config Split to the list of modules that do this in contrib.
And this solution (or rather this idea in a different solution) will be added to config_normalizer 2.x since Config Split is not meant to be an API module.
The approach there would automatically benefit from the work here.

I will have to check the very numerous test fails again later. I suspect that most of them can be solved with updated expectations for the order of config items. But maybe there are some other ones that do something strange.

So someone else can fix some of the tests in the mean time.

All the 383 config updates were of course scripted with the service I just added to config split. I can share that here for next time this needs to be re-rolled. Or we create a sub-issue to add it to core as well so that comitters can run it to make sure config is always sorted? But then we should really also expose this service to contrib. But that is all out of scope here I guess.

Re #40 if I remember correctly there is an existing test that checks that installed config for all modules can be immediately exported without changes, so in theory no script is necessary as that test will catch any issues.

Fixed some of the simpler test expectations where the config order has changed.

Nice! we are making progress!
RE #41: Thanks for the info about the test.

The changes from alexpott in 9eac1a72 made me check what my script messed up.

I found that sometimes on mappings the mapping is null.
The types of these mappings are:

field.formatter.settings.*
field.value.*
field.field_settings.*
media.source.*
 

I guess those should be followups?

ok disregard the comment with the mappings not having mappings. @alexpott was correct and it was because I didn't have all the modules installed.

However, I discovered that the tests that install everything and then export and import again (like ConfigImportAllTest) surfaced that the classes responsible for a config object also have to follow the schema order when they save the config (since they get treated as trusted data). That was not the case for blocks image styles and media entity types.
I am wondering if instead of this requirement we just sort the config also if the data is trusted is not the less disruptive solution.

I created a change notice, https://www.drupal.org/node/3230199
But it should maybe be updated? I don't really know what should be in it..
The most visible change is that some of the config is sorted.

Should we add an update hook so that all config gets re-sorted and then you export it sorted and are good from then on?

could still use review for what we have so far..

A point I'm curious about is, how does sorting interact with Drupal\Core\Config\StorageTransformEvent events? Can event subscribers assume the config they receive is sorted? Are they responsible for maintaining expected/correct sorting in any changes they make? Is config re-sorted post-event?

Should we add an update hook so that all config gets re-sorted and then you export it sorted and are good from then on?

That sounds like a good step, but any way we go about this, it looks like devs or admins are going to have to deal with one last round of just the challenges this issue will help avoid in future. We can't very well update the sync storage, which may be under version control. So the next time a dev exports config, the sync storage diff will include both the valid changes made since the last export plus spurious ones from the new sorting.

Then, when config is staged, the snapshot storage comes into play since the next diff is between sync and snapshot. Should we re-sort any existing snapshots in an update? By definition, a snapshot captures state at a given point in time, so retroactively changing it feels at the very least counterintuitive. But if we don't, the next time a site admin re-exports config and runs synchronization they'll get spurious diffs.

Maybe release notes could include suggestions for how to minimize turbulence? Possibly, a recommendation to export and sync config before updating, so that the next round of diffs will contain only the sorting changes?

RE #48
Excellent questions! However, they hint at a solution that is out of scope here:
How it interacts with storage transformations: It doesn't directly, event subscribers are expected to sort the config themselves if they change the order of keys. The problem is that currently without this patch there is no way to do that. Currently Config Entities (mostly) sort their config, the problem is that they do that freely and independently of anything else so without checking every entity there is no way for a module changing the config arrays in the storage transformation event to do so without causing a unnecessary diff. This is primarily the reason why I am interested in this patch and why I re-rolled it in the first place.
Re-sorting it automatically at the end could/should be a followup to this issue. I was experimenting with a contrib module that would do that but it faces the same issue as mentioned above and will only work when this issue here is fixed.

Yes the update hook is a good idea, and it would not lead to any unnecessary diffs in the Drupal UI/drush in a normal workflow.
One would do the normal steps:
in a development environment where you update the codebase:

1. update code (introduces the sorting and the update hook)
2. run update hooks (sorts config)
3. export config (exports sorted)
4. commit to git (this is where you get the big diff)

in all other environments

1. git pull (gets new code and sorted config in the sync directory)
2. run update kooks (sorts active config in this database)
3. import config (both active and sync config are sorted, no extra diff)

This is the standard workflow and the only place where things change is in git when you export the config, but this also happens for all other update hooks that change config in some way.
The sync storage is updated when you import the config so in the first environment you can just add an import to the steps and in the other ones the import happens as part of the normal process.
Not following the steps mentioned above will always be problematic regardless of this patch, so these concerns are not specific to this issue and are rather a documentation and education and best practice issue.

But yes we can add a specific recommendation to the change notice to export config before the update. I don't know in what circumstance I wouldn't start an update with a site that is in sync with the sync storage, but it doesn't hurt to recommend it especially.

RE #49: Thanks for linking to the other issue. Yes there will be other cases too, after all there is the option to type parts of a schema as ignored.

I am not sure everyone always follows the recommended workflow as noted in #50. I think we might need a bit more info on this in the change notice and perhaps an expanded release note snippet that explains exactly what should be done as part of this update to get the sorted config written to disk correctly.

I guess this is also the time to ask: is all our config schema now in the order we actually want it to be? e.g. "region" and "weight" are commonly used keys in different config, but they are not consistent in their ordering between different types of config. If we are to fix this, now would seem to be the time to do it as we are already re-sorting a lot of config.

RE #51: Thanks for the comment.
1) Oh, the recommended workflow just prevents you from getting spurious diffs. No matter what you do after the drupal code with this in is running the active config will be sorted. So if you try to import unsorted config it will seem like it is importing it but then you will get the same change next time you attempt import the config. This is already happening now with some of the config (not all), for example with config entities, without this patch, just try and shuffle some keys around of a block placement in the sync directory. All import attempts will result in always wanting to re-import the same change.
The solution is to just export the config as always. This will make the yaml files sorted and you are all good.

2) Yes now would be a good thing to do that. I would hate for this to de-rail the issue, though, because I don't actually care about the order as long as there is a predictable order and a fairly straight forward way of sorting the config without having to actually save the config. (which at least for mappings is the case after this issue is merged, albeit with some tricks).
So if someone feels strongly about the order of the schema then they should propose a sort order and we can re-sort and re-adjust the tests for the re-sorted expectations. There is the limitation with inherited schemas though: you can not sort things to the bottom that your schema inherited.

OK, I guess as already stated this is no different to any other config change in a release, just that this will likely cause some config diffs for every site, as long as the release notes and change notice cover that I think it's OK.

I looked through the existing "configuration system" issues and don't see anyone calling for an explicit change to the order of anything. I guess that ultimately the order doesn't really matter as the exported config is only read by humans as part of a diff, the actual order is unimportant - only the fact that it shouldn't arbitrarily change is important, and we're fixing that (or most of that) here.

I guess next step after this would be to get #2855675: Add orderby key to all sequences in core done in 9.3.x as well if possible.

Accidentally reverted the change to the release note snippet in the IS.

From an upgrade path perspective I think this is fine - it just means a big config diff. Best way to minimise that is to try to update core and contrib module separately.

I've updated the change record with more specific instructions for site owners and contrib developers.

Patch itself looks ready to go to me. I think it's OK if #2855675: Add orderby key to all sequences in core happens in a later release, the git diff is not the worst thing in the world, and it will result in far less confusing diffs over time, even if we make the core changes in two separate releases.

MR needs a rebase though.

This will be a fabulous change. The current behavior has been a source of much head-scratching and annoyance over the years. 😉

MR has been rebased, so this is ready for re-review. I'm looking now. Hope to re-RTBC shortly.

The code changes are relatively small. I had a few things that I didn't fully understand, but I'm just dipping my toes into the water here, and don't have time for a deep dive. The whole MR diff is enormous, and there's no way I can carefully review every change, but I spot checked a few and it looked like the changes are indeed just reordering things (as expected) and not changing any values (yay). I was happy to find testConfigSaveMappingSort() buried in the haystack, so it looks like we are testing / enforcing this new sorting behavior.

Since @catch thought this was RTBC before, the bot is happy, and some quick validation / sanity checking on the MR is coming up good, I'm going to go ahead and restore this to RTBC. 🎉

Thanks everyone who helped make this happen! 🙏
-Derek

I just went through the automated changes to the default config and checked where the amount of lines added don't match up with the amount of lines removed.
I found that on tours there is in general a line more because the routes are on a new line.
So:

routes:
  - route_name: system.admin

becomes:

routes:
  -
    route_name: system.admin

But that is how the yaml gets formatted now when it is exported, so it is a welcome change.

I also found a couple of comments that were stripped, I restored them with the last commit, leaving the issue as RTBC because that would otherwise have been a needs work and then would go back to RTBC.

Committed e40f6d1 and pushed to 9.3.x. Thanks!

adding link to CR in release note snippet

Sorry for asking this here, but
how to understand now (with that new order numbers), what was changed in user.role.*.yml files during GIT merging/reviving changes?

The problem is that if that YML files have a lot role's permissions (lines), like ~500, then if I change just 1 permission for a role (add or delete, for example), I see in git diff marked as deleted ~ 400 lines and as added ~ 400 lines - since orders changed globally for the whole file and made a mess?

In case you need to git-merge changes in user.role.*.yml from several developers ... it's impossible now.

I have seen the advice to use dyff tool, but it is the same result just with other "output formatting".

Thanks!

Hello,
I am going to upgrade to Drupal 9.3.3. Do you think post-update that system_post_update_sort_all_config can be skipped because it makes changes for more than 2000 configuration files at once. In the process of work on the site in the future, more of the configs will be edited and order changes will be automatically triggered with config export command. In this way the changes will be smoother and the probability of something going wrong from many conflicts will be reduced.
So basically, is the sort of configuration according to its schema vital for exported configurations or it can happen in the time.

I think something related still happens to us in our projects.

After a Drupal or module update, it happens very often that when I do a cex I see a lot of changes. But they are not changes that we have actually made. For example, changes of the position in the yml.

Example screenshot: