CKEditor 5 plugin definitions should be derivable

Oops 🤣

Crediting @balintpekker! :)

PHPStorm's automatic reformatting caused that … 😬

And more…

I forgot how painfully long this takes. Let's run only CKE5's test suite.

Expand test coverage.

Also: run only CKE5's kernel tests, not all.

And … fix!

CR also created: https://www.drupal.org/node/3313821

Stricter test coverage.

Ready for review now!

Last but not least: test coverage to prove that this works fine for PHP class annotation-based plugin definitions too!

Pointing out some key changes to help review this:

1. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5PluginManager.php
   @@ -61,6 +62,7 @@ protected function getDiscovery() {
   +      $discovery = new ContainerDerivativeDiscoveryDecorator($discovery);
   

   This is the key change, but changes were necessary elsewhere too: in the actual production code to A) allow defining a deriver, in the test infrastructure B) allow this to be tested thoroughly.

2. +++ b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
   @@ -78,38 +82,44 @@ protected function setUp(): void {
   +  private function mockModuleInVfs(string $module_name, string $yaml, array $additional_files = []): ContainerInterface {
   

   This extracts a helper method from ::testInvalidPluginDefinitions(), so that it can be reused in the new test method.

3. +++ b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
   @@ -123,12 +133,15 @@ public function testInvalidPluginDefinitions(string $yaml, ?string $expected_mes
   +      'container.namespaces' => [
   +        "Drupal\\$module_name" => vfsStream::url("root/$site_directory/modules/$module_name/src"),
   +      ] + $this->container->getParameter('container.namespaces'),
        ] + $this->container->getParameterBag()->all()));
   

   This is a necessary addition to allow \Drupal\Component\Annotation\Plugin\Discovery\AnnotatedClassDiscovery::getDefinitions() to find PHP class annotation-based plugin definitions.

Run all tests again.

1) Drupal\Tests\system\Functional\UpdateSystem\UpdatePathTestBaseFilledTest::testUpdatedSite
GuzzleHttp\Exception\ConnectException: cURL error 52: Empty reply from server (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for http://php-apache-jenkins-drupal-patches-151554/subdirectory/update.php/start?id=14&op=do_nojs

→ definitely an unrelated failure 🙃

Eh …

1) Drupal\Tests\layout_builder\Functional\LayoutBuilderFormModeTest::testDiscardValidation
Drupal\Core\Extension\Exception\UnknownExtensionException: Unknown themes: classy.

Apparently #3165010: Using the layout builder discard changes button should ignore any input and skip validation temporarily broke HEAD, it's been fixed. Re-testing again.

Thanks for the review!

1. It is indeed already technically NULL. But … this particular key-value pair in the annotation explicitly allows NULL as a valid value, as well as any string. Hence also @var string|null, unlike for example for protected $label, which is NULL by default but is required.

   This is used by \Drupal\Component\Annotation\Doctrine\DocParser::collectAnnotationMetadata(), which interprets @var.

   Precedents: \Drupal\media\Annotation\MediaSource::$thumbnail_alt_metadata_attribute and \Drupal\media\Annotation\MediaSource::$thumbnail_title_metadata_attribute.

2. 😬 Hard to recall after introducing this 3 weeks ago in #12 … did some debugging, figured it out again 👍
3. No, the first argument for each of the test cases is YAML; the PHP code lives under $additional_files. Same thing for the pre-existing providerTestInvalidPluginDefinitions() + testInvalidPluginDefinitions().

Paired with @balintpekker on #3272732: Drupal 10 & CKEditor 5 readiness, to figure out why he couldn't get his deriver to work. Turns out he was returning an array as the CKEditor 5 plugin definition in his deriver, instead of a CKEditor5PluginDefinition instance.

The deriver infrastructure introduced here is working perfectly fine for that issue now (see https://git.drupalcode.org/project/entity_embed/-/merge_requests/12/diff...) … except that this clearly points to a low-hanging fruit DX improvement! 🍎

So let's do that :)

All we need to fix that is apply the same approach as \Drupal\Core\Layout\LayoutPluginManager::processDefinition 🤓

The 1) Drupal\Tests\datetime\Functional\DateTimeWidgetTest::testDateonlyDefaultValue failure is 1000% unrelated — I wonder if this is some DST thing? 🤯

@berdir just pointed me to #2993165: Date Only field shows incorrect default value when UTC date is different than user's date, which seems related?

#26 was for the 10.1.x result.

On the 10.0.x test result, we get:

1) Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest::testWidgetViews

… yet another random JS test fail… created #3317378: [random test failure] Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest::testWidgetViews random fail.

#30: that's the usual, yes, but not a requirement. LayoutPluginManager does the same.

Basically, by not using blobs/arrays of doom but typed objects, we can offer a vastly better DX.

@effulgentsia asked in chat:

For https://www.drupal.org/project/drupal/issues/3313473#comment-14761626, it looks in the test coverage and change record, it's following the pattern of derivativeID = baseID . '_' . suffix, which then leads to a total pluginID of
{baseID}:{baseID}_{suffix}. Is that a pattern we use elsewhere? I'm not clear why this can't be simplified to derivativeID = suffix and total pluginID = {baseID}:{suffix}.

CKEditor 5 plugins are not allowed to use periods or colons in their plugin ID, because otherwise it becomes impossible to define config schema for them. This was done and enforced in #3215689: Rename CKEditor 5 plugin IDs to guarantee all plugins can be made configurable, where \Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition::__construct() got this:

  public function __construct(array $definition) {
    $this->id = $id = $definition['id'];

    $expected_prefix = sprintf("%s_", $definition['provider']);
    if (strpos($id, $expected_prefix) !== 0) {
      throw new InvalidPluginDefinitionException($id, sprintf('The "%s" CKEditor 5 plugin definition must have a plugin ID that starts with "%s".', $id, $expected_prefix));
    }
…

This proves that the actually used plugin ID is not {baseID}:{baseID}_{suffix} but {baseID}_{suffix} — otherwise tests would've been failing. (Although to be fair, explicit checks that disallow . and : would be even better.)

The reason the {baseID}:{baseID}_{suffix} plugin ID exists at all is simply because it's the unused array key generated by \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator::encodePluginId() in \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator::getDerivatives() — they're really just arbitrary array keys to prevent conflict. The source of truth is in the generated derivative itself.

You can see this in:

+++ b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
@@ -1519,4 +1546,363 @@ public function testExternalLinkAutomaticLinkDecoratorDisallowed(): void {
+        'ckeditor5_derived_plugin_foo:ckeditor5_derived_plugin_foo_A' => [
...
+          'id' => 'ckeditor5_derived_plugin_foo_A',
...
+        'ckeditor5_derived_plugin_foo:ckeditor5_derived_plugin_foo_B' => [
...
+          'id' => 'ckeditor5_derived_plugin_foo_B',

These array keys are irrelevant/arbitrary, the 'id' key-value pairs matter, because they will be returned by \Drupal\Component\Plugin\Definition\PluginDefinitionInterface::id().

Several other derivers in core set an explicit derived plugin ID. IOW: it looks like this "plugin IDs can get out of sync" thing is an oversight in the plugin system. And it looks like this has been known for a while:

• #2458769: Derivative plugin definitions contain base plugin IDs instead of derivative IDs
• #2035345: Reconsider whether to use ':' as separator for derivative plugins

The only solution until those 2 issues are fixed would be to create a tweaked ContainerDerivativeDiscoveryDecorator, which overrides encodePluginId(). I'd rather see the plugin system get improved.

P.S.: once again unrelated random failures in #24 too, opened an issue for them: #3317515: Random fail in Drupal\Tests\quickedit\FunctionalJavascript\CKEditor5IntegrationTest::testArticleNode() on 9.4 and 9.5.

I'm almost convinced by that argument, but what's the problem with colons in config schema? Can't you quote the key like in https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/config/sche...?

— @effulgentsia in chat.

WELL DAMN! Will have to address that then…

This is way better, thanks so much @effulgentsia! 🤩

Re-testing #35 on 9.5 — the only failure was fixed by #3317515-12: Random fail in Drupal\Tests\quickedit\FunctionalJavascript\CKEditor5IntegrationTest::testArticleNode() on 9.4 and 9.5, which got committed in the past 😄

There is a legitimate 9.4 failure — but we can deal with that later, let's first get this into the 3 most important branches 🤞

#38:
But encodePluginId() only sets the array key, it doesn't actually update the plugin definition. That's exactly the bug that #2458769: Derivative plugin definitions contain base plugin IDs instead of derivative IDs still needs to still solve. I'm happy to add a @todo for that? Done.

Idk that it makes sense for the plugin definition constructor to have any of this logic (for that matter, I'm not sure I like the pre-existing code in here either)

This statement of yours made things "click" for me! 😄

I've moved all of this logic into \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::processDefinition() and … sure enough, that works fine! It even hugely simplifies the code you were concerned about (and I was too!) in #32 👍

#39:

getDerivativeDefinitions() should return an array of arrays, not an array of objects.

-1 — the $base_plugin_definition it receives is already a \Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition instance. This is unavoidable because \Drupal\Component\Annotation\Plugin\Discovery\AnnotationBridgeDecorator::getDefinitions() explicitly calls <Plugin>::get(), and \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator::getDerivatives() runs after that.

That means that

          // Use this definition as defaults if a plugin already defined
          // itself as this derivative.
          if ($derivative_id && isset($base_plugin_definitions[$plugin_id])) {
            $derivative_definition = $this->mergeDerivativeDefinition($base_plugin_definitions[$plugin_id], $derivative_definition);
          }

is never executed for CKEditor 5 plugin definitions. Nor do I actually even understand when that would ever get executed.

(Note that \Drupal\Core\Plugin\DefaultPluginManager::processDefinition() has a similar note about array-based definitions vs others.)

(Same thing for Layout plugins btw, see/put a breakpoint in \Drupal\Core\Layout\Annotation\Layout::get() and run \Drupal\Tests\Core\Layout\LayoutPluginManagerTest::testProcessDefinition() for example. For them too, that cited code block is never executed.)

(In fact, the only way I was able to get that code to execute is by running \Drupal\KernelTests\Core\Plugin\DerivativeTest::testDerivativeDecorator, and in doing so I discovered it merges the definitions incorrectly: the derivative's label is overwritten by the base definition's label …)

IOW: #38 has been addressed, #39 has not, because I don't see how I could 😅

#41.1: INDEED! 😄

Excellent nits, addressed them 😊

1. Found one more doc line that needed updating like #41.2
2. Thanks to the changes in #40, we can now construct CKEditor5PluginDefinition instances in the provider, which both simplifies the assertions and makes them easier to understand
3. Figured out why the tests failed on 9.4 only: Drupal\Component\DependencyInjection\ContainerInterface was introduced by #2531564: Fix leaky and brittle container serialization solution. We don't need that in this particular test helper, we can rely on its parent, Symfony\Component\DependencyInjection\ContainerInterface.

Looks like #43 will be green on all 4 branches 😊👍

Change record updated to match the current state of the patch 👍

Actually, there's one more thing I think this should do: throw an exception just like \Drupal\Core\Config\TypedConfigManager::alterDefinitions() does when it detects an "alter hook" implementation adding new CKEditor 5 plugins — that should be forbidden.

Thoughts, @tim.plunkett?

#46: agreed 👍

Follow-up created: #3318353: hook_ckeditor5_plugin_info_alter() implementations that add a plugin should not be allowed.

Wonderful, thank you so much! 😊 This means #3272732: Drupal 10 & CKEditor 5 readiness is unblocked! 👍