Consistently sort filter formats to simplify config exports

This issue was throwing me for a loop as to why the filter configuration was always different on different env's. I applied the patch to 8.6.7 and it fixed the issue.

Just updated to 8.7 and patch #18 applied cleanly.

The issue #2852557: Config export key order is not predictable, use config schema to order keys for maps adds the ability to add a sorting key in the configuration schema. Filter configuration could be a good test case for the issue. It would also mean this patch is no longer required.

Re-roll for 8.8.x

This fixes the order of filter formats in profiles/demo_umami/config/install/filter.format.full_html.yml.

Looks like this needs a re-roll for 9.1.10.

Reroll attempt for new 9.1.10 release

Add missing updated file

+++ b/core/modules/filter/filter.install
@@ -0,0 +1,18 @@
+function filter_update_8101() {

I think this can be a post_update hook. If not, 8101 is not the right number as this will go into 9.3.x now.

Updated the patch in #31 addressing the changes suggested in #32 i.e., moved the update_8101() hook in filter.install to post_update_Name hook in filter.post_update.php, thanks!

This seems to need a re-roll for 9.3.x/9.4.x because the patch does not apply anymore.

Here is a rerolled version of the patch for 9.3.x/9.4.x - WARNING: It does not contain the changes in the core/tests/fixtures/config_install/multilingual.tar.gz and core/tests/fixtures/config_install/testing_config_install.tar.gz files that were included in the older patches!

Those files were the cause for the problems applying the patches in 9.3.x - but unfortunately I do not have any idea what has to be done or what has been done in this issues that changed those files.

@deviantintegral: Maybe you can give me some insights, so I can rework the rerolled patch to contain those changes as well again (if really needed).

Therefore this patch is not listed in the files table for now!

Is this issue/patch obsolete, now that the underlying issue is addressed in 9.3?

I think this is outdated now. There is a follow-up at #2855675: Add orderby key to all sequences in core that appears to address some lingering edge cases.

@bradjones1 I think this is still an issue - that filters are not consistently sorted. This issue is trying to fix the current sorting in implementation in the Filter system and the other one is a generic issue for all sequences in config. I think that filter sorts when operating on a filter format have to be by weight whereas in the config this is different.

I think this issue should leave filter plugin collection sorting alone as that does not feel like it should change. But what we could investigate here is disconnecting config sorting from the plugin sorting and using the orderby config schema capabilities.

Huge thread about this over at https://drupal.slack.com/archives/C01GWN3QYJD/p1694532185920599, 1 hour after #43 was posted!

Related: https://www.drupal.org/node/2852566

Hiding all the files as the MR is the way forward here.

Applied the issue summary template.

Finally worked out why this is actually occurring. It's because \Drupal\Core\Plugin\DefaultLazyPluginCollection::getConfiguration() preserves ordering so that \Drupal\filter\Entity\FilterFormat::preSave() sort just doesn't work as expected. This functionality was added in #2062367: Prevent PluginBags from reordering the export based on their sort.

I think sorting added in the MR is desirable. It'll mean less configuration change when re-ordering filter plugins and make it easier to see what is changing. This does point to an alternate fix by adding a flag to \Drupal\Core\Plugin\DefaultLazyPluginCollection::sort() to reset the \Drupal\Core\Plugin\DefaultLazyPluginCollection::$originalOrder to the new order.

The alternate fix would result in the config order changing when the a plugin is enabled or disabled - which makes the diff really hard to read so I think the fix here is correct. The remaining question is what to do about the sorting in the preSave(). I think we can remove it as it is not doing what we want - I think we might want to ensure that the FilterCollection is re-initialised after saving.

Okay well doing #49 reveals another bug in Filter formats - a different behaviour if the plugin collection is initialised on the entity and you call \Drupal\filter\FilterFormatInterface::setFilterConfig(). I think we should leave removing the sort in preSave() to a follow-up - #3392978: Inconsistent behaviour in \Drupal\filter\FilterFormatInterface::setFilterConfig()

This MR is basically just one line added in core/modules/filter/config/schema/filter.schema.yml: orderby: key:

in context:

    filters:
      type: sequence
      orderby: key
      label: 'Enabled filters'
      sequence:
        type: filter

This is very much the goal of #2855675: Add orderby key to all sequences in core! So I am very excited to see this green.
The rest of the MR is basically fixing the default to adhere to this, and is a taste of what kind of changes are to be expected on a site when the update hook runs. So the changes are sorting in config files, changes in test files to assert the correct order, an update hook and a test for it.
It all looks good to me and proves that #2855675: Add orderby key to all sequences in core may have to be a meta with many children.
Given that site builders will most likely see a diff in their config after this change (for the last time) I went ahead and created a change record so with that I think this has everything now and is RTBC.

The CR looks good, I left a little more context (mentioning explicitly config export YAMLs, and linking to the commit hash in the MR that shows an example of what kind of changes could be expected).

https://www.drupal.org/node/3393754/revisions/view/13267842/13268066

Updated IS with some of same indications from the CR.

+1 for this, was just wondering why this is not using ConfigEntityUpdater?

AFAICT this should work fine:

function filter_post_update_sort_filters(&$sandbox = []) {
  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
  $config_entity_updater->update($sandbox, 'filter_format', function (FilterFormat $format): bool {
    $sorted_filters = $filters = array_keys($format->get('filters'));
    sort($sorted_filters);
    return $sorted_filters !== $filters;
  });
}

@Wim Leers yeah so I did this because ConfigEntityUpdater's save does not result in sorting because the data is "trusted". There is a tension here - see #3347842: Deprecate the trusted data concept in configuration and #2989256: Old post_update functions that ->save() config entities without calling ->trustData() first mask errors in other post_update functions.

Looking at #2989256: Old post_update functions that ->save() config entities without calling ->trustData() first mask errors in other post_update functions the update function I've written falls into this trap and uses the fact we're not trusting the data :) so we need to fix that.

I think even though we've got the orderBy thing in config schema we need to do the ordering on the entity so it will occur regardless of whether we trust the data. If we don't then anytime a config entity is saved with the trusted data flag set we'll end up changing the order. Fun.

Back to rtbc, the discussion at drupalcon we had about the trusting data and why this can't use the config Updater suggested by Wim makes sense to me.

@borisson_ funnily enough now the update does use ConfigEntityUpdater because we work out that due to trusted data we need to sort in the preSave the correct way.

Attached is a patch for anyone looking for this fix on 10.1.x.

Following those issues now 👍 —thanks for the links, @alexpott in #55 🤓

Agreed with RTBC. This is a clear improvement, and should not be blocked on solving #3347842: Deprecate the trusted data concept in configuration.

(In fact, the current solution reminds me about @catch's repeated recommendations/requests to not write update/post-update hooks, but instead make config changes in presave hooks. This is very similar to that. (See #3393557-20: [upstream] Update CKEditor 5 to 40.0.0 + #3393557: [upstream] Update CKEditor 5 to 40.0.0.)

Nice to see this finally fixed. Adding a release note snippet because this might affect config exports after sites have upgraded.

Committed and pushed 5f533ca660 to 11.x and fee08681da to 10.2.x. Thanks!

Also published the change record and tweaked it slightly.