Move Media CKEditor 4 integration into CKEditor

Quoting Drupal\media\Plugin\CKEditorPlugin\DrupalMedia:

 * @internal
 *   This is an internal part of the media system in Drupal core and may be
 *   subject to change in minor releases. This class should not be
 *   instantiated or extended by external code.

👍

Also, AFAICT this actually is not blocked on #3271057: Move Media Library CKEditor 4 integrations from Media into CKEditor. Let's see if I'm right 🤓

Next step: the media/media_embed_ckeditor_theme asset library.

@lauriii provided guidance on how to approach that:

1. Deprecate media/media_embed_ckeditor_theme and classy/media_embed_ckeditor_theme libraries
2. Move classy/media_embed_ckeditor_theme to ckeditor and load instead of media/media_embed_ckeditor_theme
3. Needs a release note and change record
4. Move classy/css/components/media-embed-error.css to ckeditor and load by default with media integration
5. Remove mentions of the library and CSS from all other core themes

The commits that d.o/GitLab are displaying between #5 and #6 actually all happened after #6. This is very confusing 😬

Pinged the Drupal Association about this: https://drupal.slack.com/archives/CGKLP028K/p1655226648791499 — apparently #1374090: Editing a comment still changes creation date is related.

  1x: Theme "media" is extending a deprecated library. The media/media_embed_ckeditor_theme asset library is deprecated in Drupal 9.5.0 and will be removed in Drupal 10.0.0.

This deprecation error is causing a number of test failures.

Clearly the deprecation message is wrong. I debugged it, and it's actually the classy theme that is causing this. 🙃

Looks like that is caused by the 5th step not having been done yet. So … trying that now!

Now I definitely need guidance from @lauriii 🤓

I added few comment to the MR. On top of that, we need to add some overrides to rupal\KernelTests\Core\Theme\ConfirmClassyCopiesTest get rid of the failures caused by the deprecated library.

Hopefully that was the last round of this game of whack-a-mole 😬

(Turns out there was a bug in StableLibraryOverrideTest 😅)

@lauriii made the right call.

We should not be changing the media_embed_error templates. Those should remain unchanged because they're used not by the CKEditor 4 module but by \Drupal\media\Plugin\Filter\MediaEmbed::renderMissingMediaIndicator().

I dug in.

CKE4

The crucial bit is this in core/modules/ckeditor/js/plugins/drupalmedia/plugin.es6.js:

        _loadPreview(callback) {
          jQuery.get({
            url: Drupal.url(`media/${editor.config.drupal.format}/preview`),
…
            success: (previewHtml, textStatus, jqXhr) => {
…
            },
            error: () => {
              this.element.setHtml(Drupal.theme('mediaEmbedPreviewError'));
            },

→ Drupal.theme.mediaEmbedPreviewError() is only relevant for client-side preview errors. In other words: network connectivity issues.

CKE5

Same thing in CKEditor 5's equivalent at core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalmediaediting.js:

  async _fetchPreview(modelElement) {
…
    if (response.ok) {
…
      return { label, preview };
    }

    return { label: this.labelError, preview: this.themeError };
  }

with

media_media:
  ckeditor5:
…
    config:
      drupalMedia:
…
        themeError:
          func:
            name: Drupal.theme
            args: [mediaEmbedPreviewError]
            invoke: true

Conclusion

1. mediaEmbedPreviewError would've been a lot clearer if it were named mediaEmbedPreviewClientError … the "preview" was thought to imply it probably, but clearly it was not adequate
2. this means we arguably should keep it … especially because the CKEditor 5 Drupal media plugin was already designed to use the exact same template as the CKEditor 4 Drupal media plugin

Pushed 5 commits that undo many of the changes and picks for a much more targeted approach.

@bnjmnm Thanks, I just pushed the same thing, only to see after the fact you did the same 😅 The difference in line endings was caused by git 😬

Implemented solution for that last hurdle 🤞

The code itself looks good. 👍 Thank you for your patience on this @Wim Leers. 🙏

I think the code itself would be RTBC, but a critical piece of this issue is still to write a CR + release note for the fact that Drupal.theme.mediaEmbedEditButton is now in CKEditor 4.

Yay!

On it 😄

Done. CR: https://www.drupal.org/node/3292250. Release note added to issue summary.

Improved as requested in chat!

Thank you!

A bit more involved than a straight move because media was mixing generic editor integration with ckeditor4-specific stuff, but it all looks OK to me. I opened #3293741: Rename media_embed_ckeditor_theme to something more generic for a follow-up.

However, this needs a 10.1.x MR/patch, doesn't apply there.

Moving back to RTBC since #26 is green and it was just a simple reroll.

Committed/pushed the respective patches to 10.1.x/10.0.x and 9.5.x, thanks!

If we're only going to deprecate ckeditor module in 9.5.x, I'm not sure it's worth backporting. But, also not opposed if there's a good reason.