CKEditor5PluginManager: use PHP attributes instead of doctrine annotations

Yes, let's 👍 I just didn't get to that yet.

How are these changes not DX regressions? 😬

1. -      'The "ckeditor5_invalid_plugin_foo_bar" CKEditor 5 plugin definition must contain a "ckeditor5.plugins" key.',
   +      \ArgumentCountError::class,
   +      NULL,
   

2. -      'The "ckeditor5_invalid_plugin_foo_bar" CKEditor 5 plugin definition has a "drupal.label" value that is not a string nor a TranslatableMarkup instance.',
   +      \TypeError::class,
   

Only if the CKEditor 5 plugin author uses the PHP attributes plugin definition style, they get a good DX (and in that case I'd say: a better DX, if they're using an IDE, like you both are saying! 👏 And I personally love that! 🤩).

But … I disagree it's equally precise, because the 99% case is a CKEditor 5 plugin definition in YAML, where after this change you will need a Xdebug + knowing how to use it to understand the error.

Most CKEditor 5 plugins are not configurable and hence have no need for a PHP class, and would therefore be unlikely to use the PHP attributes plugin definition style.

The new DX for a CKEditor 5 plugin author using a YAML plugin definition

Defining the module's *.ckeditor5.yml file like this:

ckeditor5_invalid_plugin_foo_bar:
  ckeditor5: {}

Now results in:

1) Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testInvalidPluginDefinitions with data set "added ckeditor5" ('ckeditor5_invalid_plugin_foo_...r5: {}', 'ArgumentCountError', null)
ArgumentCountError: Too few arguments to function Drupal\ckeditor5\Attribute\CKEditor5AspectsOfCKEditor5Plugin::__construct(), 0 passed in /Users/wim.leers/core/core/modules/ckeditor5/src/Attribute/CKEditor5Plugin.php on line 58 and at least 1 expected

/Users/wim.leers/core/core/modules/ckeditor5/src/Attribute/CKEditor5AspectsOfCKEditor5Plugin.php:22

versus previously:

1) Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testInvalidPluginDefinitions with data set "added ckeditor5" ('ckeditor5_invalid_plugin_foo_...r5: {}', 'The "ckeditor5_invalid_plugin..." key.')
Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: The "ckeditor5_invalid_plugin_foo_bar" CKEditor 5 plugin definition must contain a "ckeditor5.plugins" key.

/Users/wim.leers/core/core/modules/ckeditor5/src/Plugin/CKEditor5PluginDefinition.php:90
/Users/wim.leers/core/core/modules/ckeditor5/src/Plugin/CKEditor5PluginManager.php:97
/Users/wim.leers/core/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:339
/Users/wim.leers/core/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:213
/Users/wim.leers/core/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php:198
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

and similar for now:

1) Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testInvalidPluginDefinitions with data set "added drupal.label" ('ckeditor5_invalid_plugin_foo_...el: {}', 'TypeError')
TypeError: Drupal\ckeditor5\Attribute\DrupalAspectsOfCKEditor5Plugin::__construct(): Argument #1 ($label) must be of type Drupal\Core\StringTranslation\TranslatableMarkup|string|null, array given, called in /Users/wim.leers/core/core/modules/ckeditor5/src/Attribute/CKEditor5Plugin.php on line 59

…

vs previously:

1) Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testInvalidPluginDefinitions with data set "added ckeditor5" ('ckeditor5_invalid_plugin_foo_...r5: {}', 'The "ckeditor5_invalid_plugin..." key.')
The "ckeditor5_invalid_plugin_foo_bar" CKEditor 5 plugin definition has a "drupal.label" value that is not a string nor a TranslatableMarkup instance.

…

In other words:

1. before this commit a person trying to get a CKEditor 5 plugin working in Drupal got very precise feedback: the exception was saying which exact CKEditor 5 plugin definition was invalid, and where exactly the invalidity was.
2. after this commit, that same person will only get a PHP error that does not tie it back to what the person is doing: no mention of the problematic CKEditor 5 plugin definition. If that person is comfortable with an IDE and CKEditor 5, they will be able to narrow it down. Otherwise, they won't. And it'll result in frustration.

A huge amount of effort went into ensuring non-ambiguous error messages for invalid CKEditor 5 plugin definitions, for the best possible DX.

Conclusion

I see two choices:

1. reducing the type-narrowness of \Drupal\ckeditor5\Attribute\CKEditor5AspectsOfCKEditor5Plugin::__construct() and \Drupal\ckeditor5\Attribute\DrupalAspectsOfCKEditor5Plugin::__construct(), but this would have negative DX consequences for the "PHP Attribute in IDE" scenario ⇒ bad for the minority that defines PHP classes for their CKEditor 5 plugins
2. moving some of the validation into \Drupal\ckeditor5\Attribute\CKEditor5Plugin::__construct(), in the is_array(…) === TRUE case.

I think the latter is preferable: keep the DX improvements this issue brought for a minority without reducing the DX for the majority.

MR inbound.

I believe I've fully restored the original DX now. But the validation order is slightly different, so the test cases had to change slightly. This is because the use of the strongly typed PHP attributes forces certain validation to happen earlier now.

Thank you, both of you! 😊

And again, nice work on the improved DX for the "wrote a PHP class for my CKEditor 5 plugin" case! 👏

P.S.: we would be able to remove the expected class now, because it's now again always InvalidPluginDefinitionException. Follow-up issue? Do we leave it as-is? I don't feel strongly about that — it was the DX I felt strongly about 😇