CKEditor5PluginManager: use PHP attributes instead of doctrine annotations

Should we do these here too?

\Drupal\ckeditor5\Annotation\CKEditor4To5Upgrade,\Drupal\ckeditor5\Annotation\CKEditor5AspectsOfCKEditor5Plugin, and \Drupal\ckeditor5\Annotation\DrupalAspectsOfCKEditor5Plugin

If not, we'll need new issues under the meta for them too.

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

The other two plugins are now converted. And I added something to 'function get' which was not the recommended solution but I wanted something so tests would run.

Tests appear green. Just moving to NW for tests

I don't recall seeing a requirement for additional tests on any other conversion. So, I am changing the status to get reviews of the work currently done here.

@Wim leers you tagged for tests in #3 good to remove that tag?

Removing the tests tag as we haven't added them to other attribute tickets.

But comment about the typo seems correct as other files have the capital K

Keshav Patel → made their first commit to this issue’s fork.

Fixed the typo, please review.

Believe this one is good now.

We need to update @CKEditor5AspectsOfCKEditor5Plugin annotations to attributes - and the docs. Same with @DrupalAspectsOfCKEditor5Plugin... also @CKEditor5Plugin

pradhumanjain2311 → made their first commit to this issue’s fork.

@CKEditor5AspectsOfCKEditor5Plugin and @DrupalAspectsOfCKEditor5Pluginin were already converted but I did find the docs that needed to be updated.

Document updates seem fine.

Tests kinda show this but applied locally and made sure ckeditor is still working just fine.

We need to replace the plugins with attributes in

grep -R "@CKEditor5Plugin" ./core
./core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php: * @CKEditor5Plugin(
./core/modules/ckeditor5/ckeditor5.api.php: * * @CKEditor5Plugin(

grep -R "@DrupalAspectsOfCKEditor5Plugin" ./core
./core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php: * drupal = @DrupalAspectsOfCKEditor5Plugin(
./core/modules/ckeditor5/ckeditor5.api.php: * * drupal = @DrupalAspectsOfCKEditor5Plugin(

grep -R "@CKEditor5AspectsOfCKEditor5Plugin" ./core 2111ms  Mon 15 Apr 22:19:45 2024
./core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php: * ckeditor5 = @CKEditor5AspectsOfCKEditor5Plugin(
./core/modules/ckeditor5/ckeditor5.api.php: * * ckeditor5 = @CKEditor5AspectsOfCKEditor5Plugin(

Also for some unkown reason CKEditor5AspectsOfCKEditor5Plugin was renamed CKEditor5AspectsIfCKEditor5Plugin which is incorrect

kim.pepper → made their first commit to this issue’s fork.

Addressed feedback. Just one open question.

I tried to convert the test but ended up in a whole lot of pain. I'm not entirely sure if the sub-attributes make things better or worse.

Swapped out AnnotationDiscovery for AttributeDiscoveryWithAnnotations and AnnotationBridgeDecorator for AttributeBridgeDecorator. With these changes, tests in Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest are failing. Some of that is due to the new CKEditor5AspectsOfCKEditor5Plugin and DrupalAspectsOfCKEditor5Plugin taking named parameters in their constructors vs a values array in their respective annotation classes. The validation tests in CKEditor5PluginManagerTest are failing because there are intentionally arguments missing from the constructors, but it is generating a different kind of exception.

Also, CKEditor5PluginManagerTest needs a sample plugin class using attributes.

Tests are green again, and added test for attribute-based plugin.

Circling back to this as a D11 blocker.

From what I can tell the feedback has been addressed.

Applied locally, wasn't entirely sure but test functionality of ckeditor5 and nothing broke.

Unless I'm mistaken believe this is good.

Committed and pushed db51845290 to 11.x and b00d317e70 to 10.3.x. Thanks!

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,
   

Enforcement that `ckeditor5.plugins` is required is taking place in `CkEditor5AspectsIfCkEditor5Plugin` attribute constructor now, instead of in `CKEditor5PluginDefinition::validateCKEditor5Aspects()`, and an `\ArgumentCountError` is thrown.

Similarly, the type check for `drupal.label` is from `DrupalAspectsOfCKEditor5Plugin` attribute constructor typehinting instead of `CKEditor5PluginDefinition::validateDrupalAspects()`, and `\TypeError` is thrown from the constructor param type mismatch.

The exception types and messages are different, but the result is the same. Removed the exception match string because the new exception messages include a folder path string that varies depending on where Drupal files live in the file system.

@Wim Leers what @godotislate is said. PHP's type system will give you a very precise error and now if you use an IDE it'll tell you too.

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.

LGTM. Changes are well documented with clear comments. Nice work!

Committed and pushed d18ca8ba04 to 11.x and 8e3a3f6e57 to 10.3.x. Thanks!

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 😇

+1 to follow-up for #40