Text Editor plugins are coupled to editor_form_filter_format_form_alter()

+1!

My guess (without actually looking at editor plugins) is that all of their $form_state->getValue calls were relative to the editor form itself, and not just it's own form valudes

This is true. See \Drupal\ckeditor\Plugin\Editor\CKEditor::settingsFormSubmit():

  public function settingsFormSubmit(array $form, FormStateInterface $form_state) {
    // Modify the toolbar settings by reference. The values in
    // $form_state->getValue(array('editor', 'settings')) will be saved directly
    // by editor_form_filter_admin_format_submit().
    $toolbar_settings = &$form_state->getValue(array('editor', 'settings', 'toolbar'));

    // The rows key is not built into the form structure, so decode the button
    // groups data into this new key and remove the button_groups key.
    $toolbar_settings['rows'] = json_decode($toolbar_settings['button_groups'], TRUE);
    unset($toolbar_settings['button_groups']);

    // Remove the plugin settings' vertical tabs state; no need to save that.
    if ($form_state->hasValue(array('editor', 'settings', 'plugins'))) {
      $form_state->unsetValue(array('editor', 'settings', 'plugin_settings'));
    }
  }

Does this mean we can't do this until D9? I think we can predict what catch will say about that :)

How about we do something like:

1. Use reflection to detect whether the editor plugin itself implements the deprecated settingsFormSubmit(), or the new submitConfigurationForm().
2. Let the calling code behave differently according to that.

1. +++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
   @@ -234,20 +234,15 @@ public function settingsForm(array $form, FormStateInterface $form_state, Editor
   -  public function settingsFormSubmit(array $form, FormStateInterface $form_state) {
   -    // Modify the toolbar settings by reference. The values in
   -    // $form_state->getValue(array('editor', 'settings')) will be saved directly
   -    // by editor_form_filter_admin_format_submit().
   -    $toolbar_settings = &$form_state->getValue(array('editor', 'settings', 'toolbar'));
   -
   +  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
        // The rows key is not built into the form structure, so decode the button
        // groups data into this new key and remove the button_groups key.
   -    $toolbar_settings['rows'] = json_decode($toolbar_settings['button_groups'], TRUE);
   -    unset($toolbar_settings['button_groups']);
   +    $form_state->setValue(['toolbar', 'rows'], json_decode($form_state->getValue(['toolbar', 'button_groups']), TRUE));
   +    $form_state->unsetValue(['toolbar', 'button_groups']);
    
        // Remove the plugin settings' vertical tabs state; no need to save that.
   -    if ($form_state->hasValue(array('editor', 'settings', 'plugins'))) {
   -      $form_state->unsetValue(array('editor', 'settings', 'plugin_settings'));
   +    if ($form_state->hasValue('plugins')) {
   +      $form_state->unsetValue('plugin_settings');
        }
      }
   

   I'd like to see this patch without these changes, because that would prove it doesn't break BC.

   Alternatively, make this change, but then we'll need to add test coverage for this, probably to \Drupal\Tests\editor\Unit\EditorBaseTest.

2. +++ b/core/modules/editor/editor.module
   @@ -214,6 +215,11 @@ function editor_form_filter_admin_form_ajax($form, FormStateInterface $form_stat
   +  if ($editor_plugin = $form_state->get('editor_plugin')) {
   

   Isn't this always going to be true, because of line 175, which always does $form_state->set('editor_plugin', $plugin);?

Thanks! Very close to RTBC.

Just one small thing left:

+++ b/core/modules/editor/tests/src/Unit/EditorBaseTest.php
@@ -48,6 +49,47 @@ public function testBc() {
+    // settingsForm() is deprecated in favor of buildConfigurationForm().
...
+    // settingsFormValidate() is deprecated in favor of
+    // validateConfigurationForm().
...
+    // settingsFormSubmit() is deprecated in favor of submitConfigurationForm().

These comments are the same as the original ones.

They should say something like settingsForm() uses SubFormState and is deprecated in favor of buildConfigurationForm() which uses FormState, the BC layer ensures they are have the same results.

Repeat for all three.

This should also update the CR at https://www.drupal.org/node/2819753.

🥳 Thanks, Tim! 🙏

Addressed my own nits in #13.

+++ b/core/modules/editor/src/Plugin/EditorBase.php
@@ -34,6 +35,9 @@ public function getDefaultSettings() {
+      if ($form_state instanceof SubformStateInterface) {
+        $form_state = $form_state->getCompleteFormState();
+      }

😎

Committed ef01478 and pushed to 8.8.x. Thanks!

Can we get those change record updates please (or the tag removed).

Thanks!

Change record updated: https://www.drupal.org/node/2819753/revisions/view/11495020/11528210