Ability to configure additional languages (e.g. "bash" or "SQL") for CKEditor 5 CodeBlock plugin

Closed 📌 CK5 code block languages are not configurable Closed: duplicate as a duplicate.

This is a solid starting point, but I fear that people will keep asking for more languages. We may need to have better configurability, similar to what we have for the Style plugin… although that does result in a more complex configuration UI. I think that may be reasonable though.

💯💯💯

The current patch UI is this:

My use case is that I want to add support for YML, bash, etc.

I would love (and expect) to be able to do something like this:

Note the @lauriii's patch in 📌 CK5 code block languages are not configurable Closed: duplicate implements this. Thoughts on continuing with that patch?

Screenshot:

Does the CKEditor 5 plugin actually support arbitrary languages? According to https://ckeditor.com/docs/ckeditor5/latest/features/code-blocks.html#con..., it does.

I'd like to avoid adding more of this thing|label\n-style syntax. That's why I think checkboxes are nice. Can you think of a scalable UI pattern that is more userfriendly than a <textarea> with a bunch of custom syntax?

Can you think of a scalable UI pattern that is more userfriendly than a with a bunch of custom syntax?

I can't think of any that's in use within Drupal core.

Would you be in favor of adding thing|label\n syntax (which is used throughout core) and then creating a followup issue to change the UI?

Discussed this with @mherchel in Drupal Slack: https://drupal.slack.com/archives/C01GWN3QYJD/p1680107668028699.

He convinced me that we shouldn't hold this issue up on improving the UI. That is an independent problem that affects many UIs in Drupal core. This single one isn't going to make the difference. Plus, this introduces the opportunity to in the future fix two config UIs at the same time: that for the Style CKEditor 5 plugin and that of the CodeBlock plugin!

The crucial requirement for that is already present: store the settings in a structured way, so that the UI is decoupled from the exact syntax in that <textarea>. Or rather, it's partially present in the Drupal\ckeditor5\Plugin\CKEditor5Plugin\CodeBlock class in the current MR, it's 100% present in \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Style. This MR can copy/paste the pattern already established in Drupal core 👍

So: +1 for @mherchel's proposal in #31, and let's create a follow-up for that improved UI 😊

+1 that it would be nice to something like what #32 is describing. However, it's not super straight forward to build a UI like this like I discovered with @timplunkett on 📌 List key|label entry field is textarea, which doesn't give guidance towards the expected input Fixed . Because of that, I think it would be perfectly fine to leave that for a follow-up as suggested by #34. 👍

Oh wow, that'd be another place where that same pattern is used, and that same follow-up should target that too! 👍 In fact … that issue SHOULD be the follow-up! 😄

Done: #2521800-27: List(text) key|label entry field is freetext, which doesn't give guidance towards the expected input. → .

GREAT 😄

mherchel → credited TanujJain-TJ → .

Attaching the patch from 📌 CK5 code block languages are not configurable Closed: duplicate

This was originally created by @lauriii, and then had some code formatting changes done by @TanujJain-TJ (adding credit now).

The patch still isn't massing code checks, so I'm attaching one more interdiff from the patch here, and hopefully it will pass.

Still needs tests.

Attaching the same patch, but running against 10.1.x

Previous patch had an xdebug_break(); in it. Yanking that out.

New patch adds a Nightwatch test to verify the functionality of the new editable code block languages.

There's still some legitimate CK5 kernel tests failing, so this will still fail.

Updating dictionary.txt

The last submitted patch, 43: 3282233-43.patch, failed testing. View results →

1. +++ b/core/modules/ckeditor5/config/schema/ckeditor5.schema.yml
   @@ -141,6 +141,31 @@ ckeditor5.plugin.media_media:
   +      sequence:
   +        type: mapping
   +        label: 'Language'
   +        mapping:
   +          label:
   +            type: label
   +            label: 'Language label'
   +          language:
   +            type: string
   +            label: 'Language key'
   

   🤩 This keeps the door open for a better UI in the future! 👏 (see #34 & #36) — and it matches the pattern used by the Style plugin.

2. +++ b/core/modules/ckeditor5/config/schema/ckeditor5.schema.yml
   @@ -141,6 +141,31 @@ ckeditor5.plugin.media_media:
   +      orderby: ~
   

   👍 I first thought this should be orderby: key, but actually this is better: it allows the administrator to control in which order the languages are presented to the end user 👍

3. +++ b/core/modules/ckeditor5/config/schema/ckeditor5.schema.yml
   @@ -141,6 +141,31 @@ ckeditor5.plugin.media_media:
   +      constraints:
   +        NotBlank:
   +          message: "Enable at least one language, otherwise disable the Code Block plugin."
   +        UniqueLabelInList:
   +          labelKey: label
   

   👍 This also matches the pattern established by the Style plugin.

4. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
   @@ -0,0 +1,150 @@
   +  public static function parseLanguagesFromValue(string $form_value): array {
   

   🐛 This should be protected, not public.

   The reference \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Style::parseStylesFormValue() was public to allow \Drupal\ckeditor5\Plugin\CKEditor4To5Upgrade\Core::mapCKEditor4SettingsToCKEditor5Configuration() to reuse it. That's not needed here.

5. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
   @@ -0,0 +1,150 @@
   +  public function getElementsSubset(): array {
   +    return array_column($this->configuration['languages'], 'language');
   +  }
   

   🐛 This is incorrect. This returns

   ['plaintext', 'c', 'cs', 'cpp', 'css', 'diff', …]
   

   But it really intended to generate:
   <code class="language-plaintext language-c language-cs language-cpp language-css language-diff …">.

   We could absolutely implement it that way.

   But … that actually comes with a pretty significant downside: it would mean that if the site administrator later changed the available languages that existing content (text stored in the DB) would not longer be considered valid. The set of languages that are configured here could easily evolve over time.

   So I think that in this case, not implementing CKEditor5PluginElementsSubsetInterface and ALWAYS returning <code class="language-*"> would actually be reasonable.

6. 🤩 The Nightwatch test looks awesome!

• StandardTest fails because editor.editor.full_html has not been updated to include the configuration for the codeBlock plugin.
• ConfigurablePluginTest and SmartDefaultSettingsTest fail for a similar reason but instead of changing code we need to change test expectations: there now is default configuration, and we're not yet expecting that in these tests.

These all just require me to copy over \Drupal\ckeditor5\Plugin\CKEditor5Plugin\CodeBlock::defaultConfiguration(), so doing that to hopefully enable this patch to still land in 10.1 😊

ValidatorsTest fails due to the bug I pointed out in #45.5. I can fix that by just deleting a few LoC!

Now tests should pass.

The last submitted patch, 46: 3282233-46.patch, failed testing. View results →

+++ b/core/modules/ckeditor5/ckeditor5.post_update.php
@@ -89,3 +89,19 @@ function ckeditor5_post_update_plugins_settings_export_order(&$sandbox = []) {
+/**
+ * Updates Text Editors using CKEditor 5 Code Block.
+ */
+function ckeditor5_post_update_code_block(&$sandbox = []) {
+  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
+  $config_entity_updater->update($sandbox, 'editor', function (Editor $editor): bool {
+    // Only try to update editors using CKEditor 5.
+    if ($editor->getEditor() !== 'ckeditor5') {
+      return FALSE;
+    }
+    $settings = $editor->getSettings();
+
+    return in_array('codeBlock', $settings['toolbar']['items'], TRUE);
+  });
+}

🐛 This does not actually update the configuration.

… and that would've been caught by an update test 😅

I know these tests can be a PITA to write, but I've done it so many times at this point that I figure I'd do that for you, @mherchel.

(It's 99% copy/paste of \Drupal\Tests\ckeditor5\Functional\Update\CKEditor5UpdatePluginSettingsSortTest, so I don't think this should prevent me from RTBC'ing it after the update path logic is added and the test is passing.)

The last submitted patch, 47: 3282233-47.patch, failed testing. View results →

For #49.

@mherchel told me he's traveling to a Drupal conference and asked me to write the update path (#49) and fix the nit (#45.4).

Unfortunately, this means I can in principle no longer RTBC this … even though I really only made trivial additions to help get this across the finish line. 😞

I made a small mistake in #46: I appended the ckeditor5_codeBlock config at the end, but thanks to config schema's orderby, it should've have been sorted by key — thankfully ConfigImportAllTest caught that 😊

The last submitted patch, 52: 3282233-52.patch, failed testing. View results →

Great! This helps unblock my blog migration to D10. @mherchel demoed this to me at MidCamp.

Improved the issue summary.

Fix markup.

Drupal.org's code filter is horribly broken 🥲

Untagging rc target and moving the relevant information out of #26 into the issue summary instead.

Dear reviewer:

+++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
@@ -0,0 +1,138 @@
+class CodeBlock extends CKEditor5PluginDefault implements CKEditor5PluginConfigurableInterface {
...
+  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
...
+  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
...
+  protected static function parseLanguagesFromValue(string $form_value): array {
...
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {

Note that all of this closely resembles the logic in \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Style.

Run diff core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/Style.php core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php to convince yourself.

+++ b/core/modules/ckeditor5/ckeditor5.post_update.php
@@ -89,3 +89,43 @@ function ckeditor5_post_update_plugins_settings_export_order(&$sandbox = []) {
+      $settings = $editor->getSettings();
+      // @see \Drupal\ckeditor5\Plugin\CKEditor5Plugin\CodeBlock::defaultConfiguration()
+      $settings['plugins']['ckeditor5_codeBlock'] = [
+        'languages' => [
+          ['label' => 'Plain text', 'language' => 'plaintext'],
+          ['label' => 'C', 'language' => 'c'],
+          ['label' => 'C#', 'language' => 'cs'],
+          ['label' => 'C++', 'language' => 'cpp'],
+          ['label' => 'CSS', 'language' => 'css'],
+          ['label' => 'Diff', 'language' => 'diff'],
+          ['label' => 'HTML', 'language' => 'html'],
+          ['label' => 'Java', 'language' => 'java'],
+          ['label' => 'JavaScript', 'language' => 'javascript'],
+          ['label' => 'PHP', 'language' => 'php'],
+          ['label' => 'Python', 'language' => 'python'],
+          ['label' => 'Ruby', 'language' => 'ruby'],
+          ['label' => 'TypeScript', 'language' => 'typescript'],
+          ['label' => 'XML', 'language' => 'xml'],
+        ],
+      ];
+      $editor->setSettings($settings);

This needs to happen in a hook_entity_presave() so that it also applies to config entities that are exported in install profiles or modules. Then the post update itself just needs to save the config entity and the changes will be applied here too.

Agreed with improving the UI in the follow-up identified. I did not review the entire patch.

This should address #61.

Addressing the PHPCS violation.

Just tested patch #63, and it works as expected! Looked through code, and seems logical (as far as I can tell).

RTBC (pending tests passing)

The last submitted patch, 63: 3282233-63.patch, failed testing. View results →

Looks like I forgot to apply this only for editors with CKEditor 5 🤦‍♂️

There was only a random functional JS test failure in a test unrelated to CKEditor 5.

Re-testing and back to RTBC.

1. +++ b/core/modules/ckeditor5/ckeditor5.module
   @@ -648,3 +650,34 @@ function _ckeditor5_theme_css($theme = NULL): array {
   + */
   +function ckeditor5_entity_presave(EntityInterface $entity) {
   +  if ($entity instanceof Editor && $entity->getEditor() === 'ckeditor5') {
   +    $settings = $entity->getSettings();
   +    if (in_array('codeBlock', $settings['toolbar']['items'], TRUE) && !isset($settings['plugins']['ckeditor5_codeBlock'])) {
   

   Can this use hook_editor_presave()?

2. +++ b/core/modules/ckeditor5/ckeditor5.post_update.php
   @@ -89,3 +89,18 @@ function ckeditor5_post_update_plugins_settings_export_order(&$sandbox = []) {
   + */
   +function ckeditor5_post_update_code_block(&$sandbox = []) {
   +  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
   +  $config_entity_updater->update($sandbox, 'editor', function (Editor $editor): bool {
   +    // Only try to update editors using CKEditor 5.
   +    if ($editor->getEditor() !== 'ckeditor5') {
   +      return FALSE;
   +    }
   

   This looks good.

+++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
@@ -0,0 +1,138 @@
+      '#title' => $this->t('Code Block Languages'),
+      '#type' => 'textarea',
+      '#description' => $this->t('A list of code block languages that will be provided in the "Code Block" dropdown. Enter one value per line, in the format key|label. Example: php|PHP'),
+    ];

Should this be 'A list of languages'. 'code block languages' looks like a type of language, but we just mean languages available for the code block plugin. Or should it even say 'Programming languages' to avoid confusion with the other sort of language?

+++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
@@ -0,0 +1,138 @@
+    $form['languages'] = [
+      '#title' => $this->t('Code Block Languages'),
+      '#type' => 'textarea',

Same question with 'Languages' vs. 'Code block languages' - depends on context in the UI I guess.

I'm assuming we don't have code block enabled in any existing configuration so no shipped config to update?

• #68.1:

  D'oh, now I understand. You were worried that this is now running for every entity save instead of just Editor entities! 😅

  ✅ Done! Matched the pattern established by comment_entity_view_display_presave(), layout_builder_entity_view_display_presave() etc.

• RE: "code block languages" → https://ckeditor.com/docs/ckeditor5/latest/features/code-blocks.html#con... uses both terms. Even though e.g. HTML and SQL aren't programming languages, I agree that this is a clearer term.

  ✅ Done. Updated UI:
  

• I'm assuming we don't have code block enabled in any existing configuration so no shipped config to update?

  Correct. 👍 In more detail: Full HTML has the codeBlock plugin enabled, but there was indeed no configuration for it, hence no config was ever shipped. (That's why the update path test makes grateful use of that existing config entity to prove that the necessary default configuration is now added.)

I also tried to address the feedback. Our interdiffs were identical with one exception:

+++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
@@ -25,9 +25,9 @@ class CodeBlock extends CKEditor5PluginDefault implements CKEditor5PluginConfigu
+      '#title' => $this->t('Programming Languages'),

We may want to have a lowercase l on languges but other than that looks good. This could be fixed on commit.

In more detail: Full HTML has the codeBlock plugin enabled, but there was indeed no configuration for it, hence no config was ever shipped.

What about core/profiles/standard/config/install/editor.editor.full_html.yml - if that's re-exported, won't it have the code block config added?

+++ b/core/modules/ckeditor5/tests/src/Nightwatch/Tests/ckEditor5CodeSyntaxTest.js
--- a/core/profiles/standard/config/install/editor.editor.full_html.yml
+++ b/core/profiles/standard/config/install/editor.editor.full_html.yml

+++ b/core/profiles/standard/config/install/editor.editor.full_html.yml
@@ -32,6 +32,50 @@ settings:
+    ckeditor5_codeBlock:

Isn't that use case covered by this?

. Even though e.g. HTML and SQL aren't programming languages, I agree that this is a clearer term.

I was going to quietly ignore that (and CSS). Agreed it's clearer if not 100% precise.

We may want to have a lowercase l on languges but other than that looks good. This could be fixed on commit.

Agreed and done.

Isn't that use case covered by this?

Sorry, yes, that's exactly what I meant.

I looked through the rest of the patch and can't find any more to complain about, I am not really qualified to review the nightwatch test but other people have done (and it's well commented and easy enough to follow along).

Added a change record: https://www.drupal.org/node/3356871 →

Committed/pushed to 10.1.x, thanks!

Next step: 📌 Provide an upgrade path from "codesnippet" contrib CKEditor 4 plugin to "CodeBlock" core CKEditor 5 plugin Fixed 😊

I published the change record.

Automatically closed - issue fixed for 2 weeks with no activity.