Enforce order of CKEditor 5 plugin settings in config export (as well as other sequences)

Tested locally and the tests do fail without the changes.

I want to say the failures in #16 are random. Believe something is going on with Javascript tests but I'm wrong this can be moved back.

So reviewing the failures I think they are random javascript ones.

Doc nit!

+++ b/core/modules/ckeditor5/ckeditor5.module
@@ -656,3 +657,17 @@ function _ckeditor5_theme_css($theme = NULL): array {
+  ksort($settings['plugins']);

Could this and the other ksort get a comment acknowledging that the sorting does not impact the overall functionality, but is done for better DX + test writing?

I could see this confusing contributors who see this without having worked on this issue, with them mistakenly thinking the sort order is relevant to the functionality.

We can enforce the order of the plugins via config schema. I.e.

    plugins:
      type: sequence
      label: 'Plugins'
      orderby: key
      sequence:
        type: ckeditor5.plugin.[%key]

This will ensure that the order is always the expected order no matter where the write comes from.

+++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
@@ -393,6 +393,12 @@ public function computeSmartDefaultSettings(?EditorInterface $text_editor, Filte
+    // Match the config export order for better DX. This also enables
+    // contributed modules to extend this core test.
+    $settings = $editor->getSettings();
+    ksort($settings['plugins']);
+    $editor->setSettings($settings);

I don't get the comment here - this is not a core test to extend.

Ran the tests locally and verified they do fail without the full patch.

The failure in #28 appears to be random javascript failure.

+1 for RTBC

There's a change to a comment I'd like to see and I'd be good to RTBC after that.

+++ b/core/modules/ckeditor5/ckeditor5.post_update.php
@@ -65,3 +65,27 @@ function ckeditor5_post_update_image_toolbar_item(&$sandbox = []) {
+    // Nothing to do if there are is only one plugin with settings, or none.

Change to:
// Nothing to do if there are fewer than two plugins with settings.

Made the change.

It was so minor so wouldn't be offended if it's not counted for credit haha

This looks good then, orderby is a good way to do it, and went through the symfony docs to confirm that. While I didn't confirm if the orderby: ~ additions are needed, but I do think they're helpful as it provides context for which items need ordering vs which don't. I'm going to RTBC under with the hope that the one line doc change doesn't cause test failures somehow.

One ultranit, I think we can just remove the two characters on commit?

+++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
@@ -497,6 +497,10 @@ public function test(string $format_id, array $filters_to_drop, array $expected_
 
+    // Save this to ensure the config export order is applied. O
+    // @see \Drupal\Core\Config\StorableConfigBase::castValue()
+    $updated_text_editor->save();

What is the O?

orderby: ~

I think this is OK, but maybe we should have a follow-up to apply it to the rest of core?

@catch,
Made changes as per comment #38.
Needs Review.

Patch #39 includes removals that were not requiested. In #38 the orderby: ~ was accompanied by "I think this is OK" - i.e. it doesn't need to be removed.. @catch also said it was fine to remove those two stray characters on commit.

#35 is still something I'm comfortable RTBCing, provided those two characters are removed on commit, but I'm also wary of a not-the-most-recent patch being what gets committed. I'm happy to RTBC if someone either reposts #35 or an updated patch that is #35 without the two extra chars.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x.

Needs a 9.4.x version if we want to commit it there.

This seems to be consistently failing on 9.5.x too.

There are differences between the drupal-9.4.0.filled.standard.php.gz on Drupal 9.5.x and 10.1.x that have me wondering how tests passed on 9.5 at all.

10.x includes an editor.editor.basic_html that includes

[settings] => Array
        (
            [toolbar] => Array(// toolbar stuff)
                

            [plugins] => Array
                (
                    [ckeditor5_heading] => Array(etc..)
                 )

On the 9.5, the same config has an empty plugins array

 [plugins] => Array
                (
                )

So the

 $this->assertNotSame($plugin_settings_before, $plugin_settings_after);

in testUpdatePluginSettingsSortPostUpdate shouldn't work since empty === empty.

I feel like this may not be the whole story unless 9.5 has been failing for the past 4 days since this was committed? It does look like there are intentional differences between the fixtures #3306545: Replace ckeditor with ckeditor5 in the 9.4.x database dumps in Drupal 10.0.x & 10.1.x. For the test to work in 9x presumably we'd need to first convert the editor to CK5?

Oops. Reverted!

Should this go back into Needs work?

For the things in #52

Agree with #55. This is too late now for 9.5.0, as a task it is ineligible for commit in a patch release, and so we will just have to live with it. A reminder that we should test patches on all branches before commit, especially majors where things might have diverged even if cherry pick is clean!