Consistently sort filter formats to simplify config exports

I think this is outdated now. There is a follow-up at 📌 Add orderby key to all sequences in core Active 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 - 🐛 Inconsistent behaviour in \Drupal\filter\FilterFormatInterface::setFilterConfig() Active

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 📌 Add orderby key to all sequences in core Active ! 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 📌 Add orderby key to all sequences in core Active 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.

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

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

+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 📌 Deprecate the trusted data concept in configuration Active and 🐛 Old post_update functions that ->save() config entities without calling ->trustData() first mask errors in other post_update functions Closed: outdated .

Looking at 🐛 Old post_update functions that ->save() config entities without calling ->trustData() first mask errors in other post_update functions Closed: outdated 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 📌 Deprecate the trusted data concept in configuration Active .

(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 → + 📌 [upstream] Update CKEditor 5 to 40.0.0 Needs review .)

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.

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