CKEditorPluginConfigurableInterface::settingsForm() docs don't match API usage

I have reviewed the above patch and I feel it might have some potential issue. I am attaching an improved patch that fixes the problem.

#1890502-53: WYSIWYG: Add CKEditor module to core introduced that:

   * @return array|FALSE
   *   A render array for the settings form, or FALSE if there is none.
   */
  public function settingsForm(array $form, FormStateInterface $form_state, Editor $editor);

… but I'm not sure it ever actually made sense. It slipped through the cracks, IMO.

IMO this is a bug in the API documentation, not a bug in the API. Unless you can describe a convincing use case.

Hm… interesting! Can't you make two derivers, one for the configuration-less plugins and one for the plugins with configuration?

That'd feel much cleaner. Because if some CKEditor plugins aren't configurable, they shouldn't implement CKEditorPluginConfigurableInterface.

Note that the documentation of that interface says this:

/**
 * Defines an interface for configurable CKEditor plugins.
 *
 * This allows a CKEditor plugin to define a settings form. These settings can
 * then be automatically passed on to the corresponding CKEditor instance via
 * CKEditorPluginInterface::getConfig().

i.e. this interface specifically signals that a CKEditor plugin can define a settings form.

Can you tell a bit about what you're doing? Because needing derivers for all those combinations is indeed a pain. But on the other hand it's also difficult to imagine why you'd even need to do that. Having concrete examples to look at would make that a lot easier to understand. Especially this part is difficult to understand/imagine:

The real problem (in my use case) is when a plugin's settings is depended on what other plugins is available and what settings the other plugins have.

You also mention Buttons, so do you also have examples of where you are implementing CKEditorPluginButtonsInterface but the CKEditor plugin does not have any buttons?

Finally: note that the whole point of having all these interfaces is to allow for composition. What you're describing explicitly goes against composition.

Don't get me wrong, I totally want to accommodate you, but I also have to think about long-term maintainability and code/API understandability. I first want to properly understand your use case.

One concept I'm working on is mapping CKEditor plugins (from http://ckeditor.com/addons/plugins/all), so they easily can be added to the formats configuration page, and managed there.

Wow! Did you talk to the CKEditor team? They were interested in doing exactly that some time ago, but I guess they got caught up in more urgent things in the mean time.

I'm working with a single class that implements all 3 interfaces […] Not adding a button (by returning an empty array) works fine, but not adding a settingsForm by returning either an empty array or FALSE didn't work (FALSE resulted in an error and an empty array resulted in an empty vertical tab).

Like I said in #10, this means those interfaces being implemented by a certain plugin becomes meaningless. That is the big problem here.

CKEditorPluginManager does this:

      // Enable this plugin if it provides a button that has been enabled.
      if ($plugin instanceof CKEditorPluginButtonsInterface) {
        $plugin_buttons = array_keys($plugin->getButtons());
        $enabled = (count(array_intersect($toolbar_buttons, $plugin_buttons)) > 0);
      }
      // Otherwise enable this plugin if it declares itself as enabled.
      if (!$enabled && $plugin instanceof CKEditorPluginContextualInterface) {
        $enabled = $plugin->isEnabled($editor);
      }

And that could indeed conceivably be worked around by implementing those interfaces in a particular way. But note that that then just happens to work for CKEditorPluginManager — there totally could be other code using those interfaces, and perhaps you'd break those cases. I can see that risk being low though.

But, what about this code from CKEditorPluginManager:

        // Provide enough metadata for the drupal.ckeditor.admin library to
        // allow it to automatically show/hide the vertical tab containing the
        // settings for this plugin. Only do this if it's a CKEditor plugin that
        // just provides buttons, don't do this if it's a contextually enabled
        // CKEditor plugin. After all, in the latter case, we can't know when
        // its settings should be shown!
        if ($plugin instanceof CKEditorPluginButtonsInterface && !$plugin instanceof CKEditorPluginContextualInterface) {
          $form['plugins'][$plugin_id]['#attributes']['data-ckeditor-buttons'] = implode(' ', array_keys($plugin->getButtons()));
        }

How can you possibly work around that? AFAICT this edge case must already be broken in your current code?

But I see the difference of opinion and I'm not feeling as strongly about the empty vertical tab issue as when I created the thread.

It's not really about difference of opinion. It's that implementing interfaces that are actually irrelevant/unused by a particular CKEditor plugin leads to bugs and unmaintainability elsewhere.

I could absolutely accept this change to accommodate this use case (assuming core committers would agree, which I doubt), but then I'd be breaking the general principle/pattern/logic here. For a single use case.

I see your point/pain. I think this is D7-style-thinking, and that's fine in and of itself, but I think it's more appropriate to stick with the interface composition pattern

Also, you're doing something very advanced.

However, I have a good answer for you :) You can create one class for each combination and automatically select the appropriate one. See \Drupal\Core\Entity\Plugin\Derivative\DefaultSelectionDeriver::getDerivativeDefinitions() for a very clear example. I think/guess you may still somewhat dislike that, but let's be honest: this is a super advanced use case, where you're essentially auto-generating (Drupal) plugins for many, many CKEditor plugins! So I think that's only fair.

Other examples:

• \Drupal\config_translation\Plugin\Derivative\ConfigTranslationContextualLinks::getDerivativeDefinitions()
• \Drupal\config_translation\Plugin\Derivative\ConfigTranslationLocalTasks::getDerivativeDefinitions()
• \Drupal\views\Plugin\Derivative\DefaultWizardDeriver::getDerivativeDefinitions()
• \Drupal\views\Plugin\Derivative\ViewsEntityArgumentValidator::getDerivativeDefinitions()

as this module is my first encounter with D8 and sort of my own Hello World, to familiarize myself.

An ambitious first encounter! :D :)

But to get back to the issue, the only actual code change that needs to be done is removing the FALSE part of the documentation?

Yes, indeed.

Please keep me posted, feel free to ping me on IRC. I want to help you succeed, and I want to point to your module from the documentation at https://www.drupal.org/developing/api/8/ckeditor (being the most advanced use of the CKEditor module's plugins possible) and https://www.drupal.org/developing/api/8/plugins (being a good concrete example of a very advanced use case of the Plugin API.

Thanks!

Committed 01f0ab0 and pushed to 8.1.x and 8.2.x. Thanks!

I did not commit to 8.0.x because whilst this is only a documentation change - it is an interface return value change.

#17: But it doesn't work in 8.0.x either. It's a bug in the documentation. So I think it'd be better to also commit this to 8.0.x.

@Wim Leers okay - makes sense. Committed 2ea8c26 and pushed to 8.0.x. Thanks!

Thanks :)