Create a schema for "stylescombo" which provides a better config diff

This totally makes sense! I see no reason not to do this, it's better on all fronts. The current code is pre-D8 thinking.

Thanks for filing this :) Any chance you can work on a patch? I'll provide reviews!

This configuration is likely only ever updated through the UI. The UI will remain the same. We've changed the structure of configuration in the past. What matters most is an update path, which is totally feasible.

So: I'm not concerned :)

I checked https://www.drupal.org/core/d8-bc-policy, and it doesn't mention configuration or configuration schema at all. Besides, we've done this sort of thing in the past. And much more complex ones. For example: #2308745: Remove rest.settings.yml, use rest_resource config entities. So I'm 99% certain this will be fine.

One suggestion. Instead of:

mapping:
          style:
            type: string
            label: 'Style'
          label:
            type: label
            label: 'Label'

using this would be clearer I think:

mapping:
          selector:
            type: string
            label: 'Style selector'
          label:
            type: label
            label: 'Style label'

Great work here!

1. +++ b/core/modules/ckeditor/ckeditor.install
   @@ -0,0 +1,27 @@
   +    if ($styles !== NULL && $editor->get('editor') === 'ckeditor') {
   

   I think it'd be more logical to break out of this loop early when the editor is not ckeditor?

2. +++ b/core/modules/ckeditor/ckeditor.install
   @@ -0,0 +1,27 @@
   +      $generate_method = new ReflectionMethod(StylesCombo::class, 'generateStylesSetSetting');
   +      $generate_method->setAccessible(TRUE);
   

   Nice :)

3. +++ b/core/modules/ckeditor/config/schema/ckeditor.schema.yml
   @@ -48,5 +48,21 @@ ckeditor.plugin.stylescombo:
   +          name:
   ...
   +          element:
   ...
   +          attributes:
   
   +++ b/core/modules/ckeditor/tests/src/Functional/CKEditorStylesComboAdminTest.php
   @@ -91,7 +107,18 @@ public function testExistingFormat() {
   +        'name' => 'Title',
   +        'element' => 'h1',
   +        'attributes' => ['class' => 'title']
   ...
   +        'name' => 'Callout',
   +        'element' => 'p',
   +        'attributes' => ['class' => 'callout']
   

   This matches the terminology in http://docs.ckeditor.com/#!/api/CKEDITOR.config-cfg-stylesSet — so +1! :)

4. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/StylesCombo.php
   @@ -36,14 +37,10 @@ public function getFile() {
   -    $config = [];
   ...
   -    if (!isset($settings['plugins']['stylescombo']['styles'])) {
   -      return $config;
   -    }
   ...
   +    return [
   +      'stylesSet' => isset($settings['plugins']['stylescombo']['styles']) ? $settings['plugins']['stylescombo']['styles'] : [],
   +    ];
   

   This is a behavior change that we can avoid.

   i.e. if there was no stylescombo setting, then we would return the empty array. With these changes, that's no longer the case.

   Let's bring back the old behavior, to minimize change.

5. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/StylesCombo.php
   @@ -163,4 +178,28 @@ protected function generateStylesSetSetting($styles) {
   +  protected function generateStyleSetString($style_set) {
   

   Let's make this method static, because it does not depend on any object property.

6. +++ b/core/modules/ckeditor/src/Tests/Update/StylesComboSchemaUpdateTest.php
   @@ -0,0 +1,67 @@
   +  public function testSchemaUpdate() {
   +    $this->runUpdates();
   +
   +    $this->assertStylesComboSettings('test_editor', [
   

   This is asserting the structure after the update — great! But it'd be good to also assert the structure before the update. i.e. to assert that it's a string.

CKEditor has been removed from core, CKEditor 4 is removed from Drupal Core in 10.0.0