CKEditor5PluginConfigurableInterface should extend PluginFormInterface and ConfigurableInterface instead of adding ::settingsForm()

I dug into this, first concluded it does not make sense, and then eventually after spending a few hours on a PoC, I concluded it totally makes sense, and is more consistent with the rest of Drupal core/follows more best practices.

Tried, this is impossible. We need these subforms to be aware of the Text Editor config entity this form is generated for. We can't add a new argument to this method.
Well … what if $form_state->getFormObject()->getEntity() actually returned the text editor config entity it's supposed to return?!

Furthermore, the submitConfigurationForm() part definitely does not make sense — since this will not be submitted by itself.
That's actually fine … plugin system maintainer @tim.plunkett has already set an example in 2014, in #1846172: Replace the actions API, where \Drupal\Core\Action\Plugin\Action\EmailAction::submitConfigurationForm() does

  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
    $this->configuration['recipient'] = $form_state->getValue('recipient');
    $this->configuration['subject'] = $form_state->getValue('subject');
    $this->configuration['message'] = $form_state->getValue('message');
  }

Clearly, that's not saving anything! Same in the more recent \Drupal\Core\Layout\LayoutDefault::submitConfigurationForm().

Finally, validateConfigurationForm() actually invites writing form validation logic, but it really should have validation constraints. We could simply not call the validation method…

Note that this branch is relative to #3222852: <dl> <dt> <dd> by introducing "Manually editable HTML tags" configuration on Source Editing — i.e. it includes its full commit history, because that's where this problem became apparent. This should either land after #3222852, or it should be rebased to apply to latest 1.0.x — either way works.

Happy to push this forward more, but would first like reviews, especially from @tim.plunkett.

Because we constantly throw away and rebuild these plugins (not sure why, but we do)

I noticed that too while debugging… is it because we are not yet following certain best practices? Or is it because we did not yet optimize things at all?

Reproduced:

Digging deeper… found the root cause.

  public function getPlugin(string $plugin_id, ?EditorInterface $editor): CKEditor5PluginInterface {
    $configuration = [];
    if ($editor) {
      $configuration = $editor->getSettings()['plugins'][$plugin_id] ?? [];
    }
    return $this->createInstance($plugin_id, $configuration);
  }

(introduced in https://git.drupalcode.org/project/ckeditor5/-/merge_requests/93/diffs?c...)

Except that is still somehow not taking effect (or happening too late, too early, etc).

That is exactly it: it's happening "too late". Because we introduced \Drupal\ckeditor5\AdminUi::getSubmittedEditorAndFilterFormat() which operates on $form_state->getValues() and/or $form_state->getUserInput() to make the dynamic AJAX form stuff possible at all (at least back then), form validation logic has not yet run, and hence these transformations have not yet run.

Note I said "at least back then". Maybe by now we can actually refactor AdminUi::getSubmittedEditorAndFilterFormat() away? 🤞🤞🤞

But to keep scope manageable here, I'd say we could instead be invoking from within AdminUi::getSubmittedEditorAndFilterFormat()each plugin's ::validateConfigurationForm() and ::submitConfigurationForm(). Not pretty, not elegant, but a stepping stone?

Actually, why is ::submitConfigurationForm() not yet implemented? I thought we'd match EmailAction::submitConfigurationForm()'s pattern I quoted in #2?

Met with Tim, discussed this in detail, he's going to push it forward further :)

Updating issue metadata to reflect that.

@tim.plunkett: That looks WONDERFUL! Review posted. Basically only nits.

I'm happy with this going as-is, so: RTBC.

I'm signing off for today. Feel free to merge. If you don't: are you okay with me reviewing and merging this tomorrow, and pushing this to completion to the best of my ability? It's green thanks to you, it's looking waaaaaay better than in HEAD thanks to you, and so I'd love to get it in ASAP :) (Ideally I wake up to this having been merged! :D)

I did not like that last commit making the new CKEditor5PluginManager::getPlugin() public, but that's it. Tracking that in #3228465-7: Final pass over the public API prior to creating core merge request.

Actually … I do not like the pre-existing public function getPlugins() on CKEditor5PluginManager either. And I think you just made it easier to solve that. There's a lot of public API on the plugin manager that shouldn't be public API. That's already captured on #3228465 fortunately.

In digging into this, I just realized something: \Drupal\ckeditor5\Plugin\Editor\CKEditor5 is a plugin instance. Since this commit, it now contains

  /**
   * The set of configured CKEditor 5 plugins.
   *
   * @var \Drupal\ckeditor5\Plugin\CKEditor5PluginInterface[]
   */
  private $plugins;

But those are constructed using

  private function getPlugins(EditorInterface $editor): array {
    if (!$this->plugins) {
      $this->plugins = $this->ckeditor5PluginManager->getPlugins($editor);
    }
    return $this->plugins;
  }

… which means $this->plugins is now tied to a EditorInterface config entity: the plugin instances in the text editor plugin instance are coupled to that particular text editor config entity. But the text editor plugin instance is not coupled to a particular text editor config entity. Effectively we've made the Text Editor plugin instance stateful where it stateless before.

Creating an issue…

Created #3228945: CKEditor 5 plugin instances shenanigans: text editor plugin is statefully coupled + plugin instance methods on manager for #18.