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

You're right — #3263384: Add ckeditor5-code-block package and CodeBlock plugin added the Code Block plugin, but did not yet allow any configuration:

ckeditor5_codeBlock:
  ckeditor5:
    plugins:
      - codeBlock.CodeBlock
      - htmlSupport.GeneralHtmlSupport
    config:
      # The CodeBlock plugin supports only `<pre><code>…< /code></pre>`.
      # Configure GHS to support `<pre>…</pre>` markup as well.
      htmlSupport:
        allow:
          -
            name: pre
  drupal:
    label: Code Block
    library: ckeditor5/drupal.ckeditor5.codeBlock
    admin_library: ckeditor5/admin.codeBlock
    toolbar_items:
      codeBlock:
        label: Code Block
    elements:
      - <pre>
      - <code class="language-*">

We can (and should) totally add that. It just was not necessary for feature parity with CKEditor 4. Hence it's not a stable blocker. We're first tackling stable blockers.

We'll get to this after CKEditor 5 is stable!

Actually … would you be interested in contributing this feature? 🤓

You'd need to do this:

    drupal:
      label: Code Block
      library: ckeditor5/drupal.ckeditor5.codeBlock
      admin_library: ckeditor5/admin.codeBlock
+     class: Drupal\ckeditor5\Plugin\CKEditor5Plugin\CodeBlock
      toolbar_items:
        codeBlock:
          label: Code Block
      elements:
        - <pre>
        - <code class="language-*">

… and then copy/paste \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Heading, rename it to CodeBlock, and modify it to be about languages to support 😊

Awesome! 😄

Wow! 🤩 You went all the way! 😄👏 Validation constraints included! Thank you so much!

One thing I still can not seem to figure out is how to update the dropdown list in Ckeditor code block button. It keeps displaying the list from config provided by the vendor javascript file: https://github.com/drupal/core/blob/9.4.x/assets/vendor/ckeditor5/code-b...

Ah, yes!

For that, we need to generate the correct CKEditor 5 JS plugin configuration based on the Drupal-managed/stored configuration. But … I see you already have a ::getDynamicPluginConfig() that looks good. 👏 So what's going on? 🤔

Do you see the JS configuration you would expect if you go to a page where your CKEditor 5 instance is active and you type this in the console?

drupalSettings.editor.formats.basic_html.editorSettings.codeBlock

?

I'm not yet familiar with the core test setup. So I did not provide any. What kind of tests should a feature like this receive?

Great question! For this kind of thing, where all we're doing is non-risky configuration for a CKEditor 5 plugin, what we tend to do is write a unit test for them. (Yes, that is possible thanks to the use of \Drupal\Component\Plugin\ConfigurableInterface! 👍) The unit test basically tests the default configuration plus relevant edge cases you can think of, and verifies that getDynamicPluginConfig() actually generates the expected CKEditor 5 JS plugin configuration.

Examples: \Drupal\Tests\ckeditor5\Unit\HeadingPluginTest, \Drupal\Tests\ckeditor5\Unit\ListPluginTest, \Drupal\Tests\ckeditor5\Unit\AlignmentPluginTest.

Are merge requests usually made against the stable branch? I've targeted 9.3.x.

Usually they actually go against the "next" branch, which right now is 9.5.x. But … the CKEditor 5 module is "special": it is experimental. Which means that every change gets committed to every branch (so currently that is 9.3.x, 9.4.x, 9.5.x and 10.0.x — this is a pretty special time in the Drupal core release cycle — usually it's only two branches!), to make sure that sites/people testing experimental modules are actually testing the latest and greatest.

So: what you did is perfect — once this gets to RTBC we'll just need to create a patch (or multiple) to ensure it passes tests against all those branches. (I'm happy to do that for you if you'd rather not.)

Question

What was this experience like for you? Any and all feedback on the DX would be deeply appreciated 😊 Now is the time when we can still make tweaks. Please be brutally honest 😄 🙏

I think I found the answer to the one problem we were having here: https://git.drupalcode.org/project/drupal/-/merge_requests/2319/diffs#no...

BTW, if you want the tests to run a lot faster on this merge request: this patch will run only the CKE5 tests. We'll have to remove it eventually, but while working to get this to pass tests, this can save you a lot of time :) Test results in <10 minutes 👍

RE: contribution experience

I would love to know what setup core developers are using to work on the Drupal development codebase. I prefer Docker based setups. But the ones listed here require manual setup steps: https://www.drupal.org/community/contributor-guide/reference-information...

"Bare metal": natively installed PHP + Apache + MySQL using this guide: https://getgrav.org/blog/macos-monterey-apache-multiple-php-versions. Everything else interferes too much. Core's code base is large. Docker performance on macOS is poor (but I hear this is maybe changing?). Performance matters when running core tests. (Though unit tests are probably fine in Docker too.)

What I'm currently missing a bit is a quick environment/codebase setup that allows me to easily run and debug the tests locally. Because I was looking for a solution for my contributed module field_highlightjs I tried including a Drupal core codebase setup in there. But this proves to be somewhat annoying.

cp core/phpunit.xml.dist core/phpunit.xml and tweak core/phpunit.xml to specify your local DB + URL. As of that point, you can run all tests locally. Also through PHPStorm.

Test failures

The tests that are failing make a ton of sense:

• CKEditor4to5UpgradeCompletenessTest fails because now that this plugin has been made configurable, our test coverage that verifies that configurable plugins generate the sensible configuration when upgrading from CKE4 is failing. This is good — we need to add that 🤓
• Drupal\Tests\ckeditor5\Kernel\ConfigurablePluginTest::testDefaults(): same thing — this tests the expected default configuration for each configurable CKEditor 5 plugin. And this one was just made configurable, whereas previously it was not. A simple addition to the expectations will suffice! 🤓
• SmartDefaultSettingsTest has 2 failing test cases: those that place the CodeBlock toolbar item. Why? Because the default configuration that you just added is now present after upgrading from CKEditor 4. This is also good — we need to update the expectations 🤓
• ValidatorsTest: fails due to this change:
  

  +++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
  @@ -238,16 +238,70 @@ ckeditor5_codeBlock:
  -      - <code class="language-*">
  +      - <code class="*">
  

  Why that change? I think we can revert that 😄

Review

See the last bullet above, and:

1. +++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
   @@ -238,16 +238,70 @@ ckeditor5_codeBlock:
   +      codeBlock:
   +        languages:
   

   Nit: if you move this and everything under it a few lines higher, to before the htmlSupport: line, the diff will become simpler to review 🤓

2. +++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
   @@ -238,16 +238,70 @@ ckeditor5_codeBlock:
   +          - { language: 'html', label: 'HTML', class: 'language-html language-xml' }
   

   Why two classes here? 🤔

3. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
   @@ -0,0 +1,214 @@
   +  const DEFAULT_CONFIGURATION = [
   +    'enabled_languages' => [
   +      'bash',
   +      'css',
   +      'diff',
   +      'html',
   +      'javascript',
   +      'json',
   +      'less',
   +      'markdown',
   +      'php',
   +      'scss',
   +      'sql',
   +      'twig',
   +      'xml',
   +      'yaml',
   +    ],
   +  ];
   

   This does not match what's documented upstream as the default:
   https://ckeditor.com/docs/ckeditor5/latest/api/module_code-block_codeblo..., where it says:

   languages: [
   	{ language: 'plaintext', label: 'Plain text' }, // The default language.
   	{ language: 'c', label: 'C' },
   	{ language: 'cs', label: 'C#' },
   	{ language: 'cpp', label: 'C++' },
   	{ language: 'css', label: 'CSS' },
   	{ language: 'diff', label: 'Diff' },
   	{ language: 'html', label: 'HTML' },
   	{ language: 'java', label: 'Java' },
   	{ language: 'javascript', label: 'JavaScript' },
   	{ language: 'php', label: 'PHP' },
   	{ language: 'python', label: 'Python' },
   	{ language: 'ruby', label: 'Ruby' },
   	{ language: 'typescript', label: 'TypeScript' },
   	{ language: 'xml', label: 'XML' }
   ]
   
   

   Why is that?

4. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
   @@ -0,0 +1,214 @@
   +  public function getElementsSubset(): array {
   +    $enabled_languages = $this->getEnabledLanguages();
   +    $plugin_definition = $this->getPluginDefinition();
   +    $all_elements = $plugin_definition->getElements();
   +    $subset = HTMLRestrictions::fromString(implode($all_elements));
   +    $classes = [];
   +    foreach ($plugin_definition->getCKEditor5Config()['codeBlock']['languages'] as $configured_language) {
   +      if (in_array($configured_language['language'], $enabled_languages, TRUE)) {
   +        $classes[] = $configured_language['class'] ?? 'language-' . $configured_language['language'];
   +      }
   +    }
   +    $element_string = '<pre> <code class=' . '"' . implode(' ', $classes) . '"' . '>';
   +    $subset = $subset->intersect(HTMLRestrictions::fromString($element_string));
   +
   +    return $subset->toCKEditor5ElementsArray();
   +  }
   

   I think this could be a lot simpler. Something like this:

       $subset = $this->getPluginDefinition()->getElements();
       $subset = array_diff($subset, ['<code class="language-*">']);
       foreach ($plugin_definition->getCKEditor5Config()['codeBlock']['languages'] as $configured_language) {
         if (in_array($configured_language['language'], $enabled_languages, TRUE)) {
           $class = $configured_language['class'] ?? 'language-' . $configured_language['language'];
           $subset[] = sprintf('<code class="%s">', $class);
         }
       }
       return $subset;
   

5. +++ b/core/modules/ckeditor5/tests/src/Unit/CodeBlockPluginTest.php
   @@ -0,0 +1,182 @@
   +    // This matches the language list from ckeditor5.ckeditor5.yml so our test
   +    // will fail whenever that list is altered. That is because removing a
   +    // language could result in the allowed tags being altered.
   +    $all_languages = [
   +      ['language' => 'plaintext', 'label' => 'Plain text'],
   +      ['language' => 'accesslog', 'label' => 'Apache Access Log'],
   +      ['language' => 'bash', 'label' => 'Bash'],
   +      ['language' => 'c', 'label' => 'C'],
   +      ['language' => 'cpp', 'label' => 'C++'],
   +      ['language' => 'csharp', 'label' => 'C#'],
   +      ['language' => 'css', 'label' => 'CSS'],
   +      ['language' => 'diff', 'label' => 'Diff'],
   +      ['language' => 'dockerfile', 'label' => 'Dockerfile'],
   +      ['language' => 'go', 'label' => 'Go'],
   +      ['language' => 'groovy', 'label' => 'Groovy'],
   +      ['language' => 'html', 'label' => 'HTML', 'class' => 'language-html language-xml'],
   +      ['language' => 'http', 'label' => 'HTTP'],
   +      ['language' => 'ini', 'label' => 'TOML, also INI'],
   +      ['language' => 'java', 'label' => 'Java'],
   +      ['language' => 'javascript', 'label' => 'JavaScript'],
   +      ['language' => 'json', 'label' => 'JSON'],
   +      ['language' => 'kotlin', 'label' => 'Kotlin'],
   +      ['language' => 'less', 'label' => 'Less'],
   +      ['language' => 'lua', 'label' => 'Lua'],
   +      ['language' => 'makefile', 'label' => 'Makefile'],
   +      ['language' => 'markdown', 'label' => 'Markdown'],
   +      ['language' => 'nginx', 'label' => 'Nginx config'],
   +      ['language' => 'objectivec', 'label' => 'Objective-C'],
   +      ['language' => 'perl', 'label' => 'Perl'],
   +      ['language' => 'pgsql', 'label' => 'PostgreSQL and PL/pgSQL'],
   +      ['language' => 'php', 'label' => 'PHP'],
   +      ['language' => 'php-template', 'label' => 'PHP Template'],
   +      ['language' => 'python', 'label' => 'Python'],
   +      ['language' => 'python-repl', 'label' => 'Python REPL'],
   +      ['language' => 'r', 'label' => 'R'],
   +      ['language' => 'ruby', 'label' => 'Ruby'],
   +      ['language' => 'rust', 'label' => 'Rust'],
   +      ['language' => 'scss', 'label' => 'SCSS'],
   +      ['language' => 'shell', 'label' => 'Shell Session'],
   +      ['language' => 'sql', 'label' => 'SQL'],
   +      ['language' => 'swift', 'label' => 'Swift'],
   +      ['language' => 'twig', 'label' => 'Twig'],
   +      ['language' => 'typescript', 'label' => 'TypeScript'],
   +      // cSpell:disable
   +      ['language' => 'vbnet', 'label' => 'Visual Basic .NET'],
   +      ['language' => 'vbscript', 'label' => 'VBScript'],
   +      ['language' => 'vbscript-html', 'label' => 'VBScript in HTML'],
   +      // cSpell:enable
   +      ['language' => 'xml', 'label' => 'XML'],
   +      ['language' => 'yaml', 'label' => 'YAML'],
   +    ];
   

   That is a lot of repetition/duplication 😅 I agree with the sentiment, but in this case it might be a bit much?

   Would you be opposed to applying the pattern from \Drupal\Tests\ckeditor5\Unit\LanguagePluginTest::buildExpectedDynamicConfig(), to avoid having this list hardcoded in two places?

6. +++ b/core/modules/ckeditor5/tests/src/Unit/CodeBlockPluginTest.php
   @@ -0,0 +1,182 @@
   +    $default_language_ids = [
   +      'bash',
   +      'css',
   +      'diff',
   +      'javascript',
   +      'json',
   +      'less',
   +      'markdown',
   +      'php',
   +      'scss',
   +      'sql',
   +      'twig',
   +      'xml',
   +      'yaml',
   +    ];
   

   Can't this be simplified to $default_language_ids = CodeBlock::DEFAULT_CONFIGURATION? 🤓

+++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
@@ -238,16 +238,38 @@ ckeditor5_codeBlock:
-      - <code class="language-*">
+      - <code class="*">

This change is causing the last test failure.

And … this should not actually work! You should get the validation error

The "ode" HTML tag has an attribute restriction "*". This implies all attributes are allowed. Remove the attribute restriction instead, or use a prefix (`*-foo`), infix (`*-foo-*`) or suffix (`foo-*`) wildcard restriction instead.

Ah, no, that is for the attribute name, not the attribute value.

Congrats, you've found an edge case for which we did not yet have an explicit validation check! 🤓😄 Created #3284254: HTMLRestrictions should not allow <tag attr="*"> because that is equivalent to <tag attr> for addressing that.

#22:

that has a class that does not start with "language-" I could not get the subset to resolve properly.

1. When does that happen? That seems like it should not be allowed? Even if classnames are technically arbitrarily nameable upstream, that doesn't mean that we need to allow that! I think requiring a language- prefix is A) totally reasonable, B) more predictable. I think we should enforce that restriction. We could even add an assert() to ::getDynamicPluginConfig() to prevent people from altering the metadata in this way.
2. If the subset does not resolve properly, that'd be a bug.

Patch review

Excellent work here! 👍👏

1. +++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
   @@ -238,16 +238,38 @@ ckeditor5_codeBlock:
   -      - <code class="language-*">
   +      - <code class>
   

   👎 This is effectively saying that this plugin can generate any value for the class attribute. That is not true.

2. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5Upgrade/Contrib.php
   @@ -53,7 +55,32 @@ public function mapCKEditor4SettingsToCKEditor5Configuration(string $cke4_plugin
       */
      public function computeCKEditor5PluginSubsetConfiguration(string $cke5_plugin_id, FilterFormatInterface $text_format): ?array {
   -    throw new \OutOfBoundsException();
   +    switch ($cke5_plugin_id) {
   +      case 'ckeditor5_codeBlock':
   +        $restrictions = $text_format->getHtmlRestrictions();
   +        if ($restrictions === FALSE) {
   

   👏 NICE! 😄

   But this means we should have explicit upgrade path test coverage. You can achieve that by adding a new text format + editor (just like $basic_html_format_with_pre does) and adding a new test case to \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::provider() 😊

3. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5Upgrade/Contrib.php
   @@ -53,7 +55,32 @@ public function mapCKEditor4SettingsToCKEditor5Configuration(string $cke4_plugin
   +        if (isset($restrictions['allowed']['code']['class'])
   +        && ($classes = $restrictions['allowed']['code']['class'])
   +        && is_array($classes)) {
   

   🤓 Interesting indentation here, and also very interesting that phpcs does not complain about this. Core does not allow this kind of formatting.

4. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5Upgrade/Contrib.php
   @@ -53,7 +55,32 @@ public function mapCKEditor4SettingsToCKEditor5Configuration(string $cke4_plugin
   +              return in_array('language-' . $enabled_language, $classes);
   

   🤓 Nit: Drupal core always uses strict in_array() checks, so could you specify the third parameter? 🙏

5. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
   @@ -0,0 +1,222 @@
   +      if (in_array($language, self::ALWAYS_ENABLED_LANGUAGES, TRUE)) {
   

   👍 Like here!

6. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/CodeBlock.php
   @@ -0,0 +1,222 @@
   +  public static function getStaticPluginConfig(): array {
   

   🤔 This should be private: we don't want this to become a public API that we must continue to support forever! This is only called from the unit test — then this should be a private helper on the unit test instead.

#25 is now in fact possible — #3284254: HTMLRestrictions should not allow <tag attr="*"> because that is equivalent to <tag attr> has been fixed!

Furthermore, based on the feedback at #3269387-8: Drupal 10 & CKEditor 5 support and #3269387-9: Drupal 10 & CKEditor 5 support, I'm now convinced that implementing this simple feature is sufficient for https://www.drupal.org/project/codesnippet to become obsolete when upgrading from CKEditor 4 to CKEditor 5, much like we previously did with #3274278: Migrate "codetag" contrib CKEditor 4 plugin to built-in equivalent in core's CKEditor 5.

The last commit (0f8c5c47) does not apply cleanly to D10: a small conflict needs to be resolved. Let's see how this would do on D10.

This one's for you, @Steffen Schlaer!

Note that because of limitations in the current patch, you'll for now have to manually modify core/modules/ckeditor5/ckeditor5.ckeditor5.yml, and add something like this:

          - { language: 'sql', label: 'SQL' }

Closed #3349740: CK5 code block languages are not configurable 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 #3349740: CK5 code block languages are not configurable 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 #2521800: List key|label entry field is textarea, which doesn't give guidance towards the expected input. 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 key|label entry field is textarea, which doesn't give guidance towards the expected input.

GREAT 😄

Attaching the patch from #3349740: CK5 code block languages are not configurable

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 passing 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

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.

+++ 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.)

For #49 and #45.4.

@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 😊

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)

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: It could, but it shouldn't? 🤔 This is a concern of the CKEditor 5 text editor plugin, not of the generic Text Editor module? For [#3293540], we also added the update logic (ckeditor_post_update_omit_settings_for_disabled_plugins()) to the ckeditor module, not the editor module?

  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: #3356929: Provide an upgrade path from "codesnippet" contrib CKEditor 4 plugin to "CodeBlock" core CKEditor 5 plugin 😊

I published the change record.