Make error messages for embedded media themeable

Here's an interdiff I'm extracting for reference. It's built off of #2994696: Render embedded media items in CKEditor #194.

Not interdiffing as #2 was there for reference and not intended to work. This builds off of that and adds tests which expose what might be an issue: When performing the test steps manually, I can trigger the preprocess hook that confirms the message is themeable. In the FunctionalJavascript test it does not work, as I discovered it is generating the markup from theDrupal.theme.mediaEmbedError in plugin.es6.js.

I'm hoping it's just been a long week and there's an obvious way to resolve this, but it seems like making the embedded media error messages themeable could cause consistency problems as they're not always generated by the theme layout.

Thanks for the patch!

The consistency problem you're describing is actually the expected behavior. There are two types of error messages related to media embeds:
1) One from the backend, that comes from the preview route (which should be themable in a twig template). This one I like to call (borrowed from Wim Leers) the "missing media indicator".
2) One from the javascript:

  Drupal.theme.mediaEmbedError = function () {
    var error = Drupal.t('An error occurred while trying to preview the media. Please save your work and reload this page.');
    return '<div class="media-embed-error media-embed-error--preview-error">' + error + '</div>';
  };

This one is out of scope for this issue, as it already has a javascript theme function. The test updated in #4, ::testPreviewFailure() is the second one, so that's the wrong error message. It shouldn't get the template modifications.

A second thing, the changed the attached library shouldn't change based on #2, but keep the current library. I'm switching it back. The correct library in HEAD is media/media_embed.

Adding functional js testing for the missing media indicator, as we only have a kernel test right now. I think there's value in putting them both in the same test function as well as some comments explaining the differences between the two types of errors. This will add a bit more test coverage and also provide some documentation if someone greps for renderMissingMediaIndicator() in the code base.

Would solving #2859550: "status_messages"-like render element with a preset message help with this issue as it sounds like the two might be related?

A related issue over there #3078400: Remove hard coded messages markup might also be relevant here.

Looking good! I found only nits.

1. +++ b/core/modules/media/templates/media-embed-error.html.twig
   @@ -0,0 +1,16 @@
   + * - string message: The message text.
   

   🤓The string string here should be removed.

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -210,6 +211,31 @@ public function testPreviewFailure() {
   +    // There's a second kind of error message that comes from the backend.
   ...
   +    // Test that restoring a valid uuid results in the media embed preview
   

   Nits:
   - s/backend/back end/
   - s/uuid/UUID/

3. +++ b/core/modules/media/tests/src/Kernel/MediaEmbedFilterTest.php
   --- /dev/null
   +++ b/core/themes/classy/templates/content/media-embed-error.html.twig
   

   🤔 What's the point of this Twig template if it does exactly the same anyway?

4. +++ b/core/themes/classy/templates/content/media-embed-error.html.twig
   @@ -0,0 +1,13 @@
   + * - string message: The message text.
   + * - string classes: The classes to add to the HTML tag.
   
   +++ b/core/themes/stable/templates/content/media-embed-error.html.twig
   @@ -0,0 +1,13 @@
   + * - string message: The message text.
   + * - string classes: The classes to add to the HTML tag.
   

   Same here as in point 1.

Addressing #9.
Fixed nits and dropped redundant templates.

Adding media-embed-error to the list of templates to skip for stable. This allows us to not put the redundant template in stable. This hasn't been used but once before. So, are we supposed to copy redundant templates to stable? I'm adding a second patch here that has the template in stable, since I suspect we're breaking a precedent by not copying it to stable.

Same as the last one (with stable template), but with the updates suggested in #9.1 and #9.4.

Per #3072319-16: Determine the scope of default styling for embedded media (what belongs in Classy, Stable, etc.?):

Discussed with @lauriii.

The only reason we have this issue, which is about deciding where the default styling for the "missing media indicator" lives is that there even is default markup with default classes. Once we do #3071713: Make error messages for embedded media themeable, there is no default styling: the markup with the .media-embed-error error should itself be moved to Classy, then Classy and its subclasses will get it. Any themes that choose not to use classy as their base theme already consciously choose to not inherit any classes or CSS, so it's normal to expect them to specify their own classes and CSS.

I'll roll a patch for #3071713: Make error messages for embedded media themeable per @lauriii's recommendations above.

Thanks, @lauriii! 🙏🥳

Moving issue credit over.

This implements @lauriii's suggestions for the server-side (@Filter=media_embed)-triggered media embed errors.

The client-side (@CKEditorPlugin=drupal_media)-triggered media embed errors still needs to be handled.

And this does the client-side portion.

Which also revealed a bug: in HEAD, it's impossible for themes to override the Drupal.theme.* functions defined in core/modules/media/js/plugins/drupalmedia/plugin.es6.js!

(This is untested, I need to move on to other issues. But I trust @oknate can push it across the finish line 😀!)

Fixes the test failure in #17.

And this should fix most of the test failures in #18 (untested).

Fixing tests.

#26.9: Maybe, good question! 👍

Let me explain why I did it this way. I grepped for Drupal.theme. in JS files in core/themes/ and found these in stable.info.yml:

libraries-extend:
  core/drupal.ajax:
    - stable/drupal.ajax
  user/drupal.user:
    - stable/drupal.user

Hence the use of libraries-extend instead of libraries-override.

Responding to #26:
1. One is for the filter kernel test and one is for the functional javascript CKEditor test. I guess we don’t need it in both places or the test modules could be combined. But I think that’s out of scope. For now, I think it’s appropriate to have the hook in both places, as I think it's valuable to test both in the filter kernel test and in the CKEditor integration FunctionalJavascript test. I think a follow-up issue to see if those two test modules could be combined is the right thing to do.
2. ✅Undoing that change. I misread it, and missed the assertEmpty, so I thought, the wait was unnecessary. Since we’re expecting it to be empty, having a shorter wait time was right. Undoing the change.
3. ✅ Ruined the suspense with a lengthy explanation.
4. ✅ Removed redundant assertion.
5. ✅ Replaced how we’re setting classy to not use config factory.
6. ✅ Removed the assertion, and added the class to the previous assertion.
7. ✅ Updated to use cssSelect()
8. ✅ I alphabetized them. Seems out of scope? Never mind, undoing this change. It looks very out of scope. They're not alphabetized.
9. We actually want to load both, as we’re only overriding the theme function for the missing media indicator, while leaving the edit button alone. if we used libraries-override without adding the edit button, we’d lose the edit button, and if we add the edit button, I guess we’re doing it unnecessarily.

1. #28.3 😂

2. +++ b/core/modules/media/tests/src/Kernel/MediaEmbedFilterTest.php
   @@ -296,13 +296,11 @@ public function testMissingEntityIndicator($uuid, array $filter_ids, array $addi
   -    $this->assertSame($expected_class, (string) $error_element['class']);
   +
      }
   

   Übernit: extraneous newline being added here.

3. As far as I'm concerned, this is RTBC. I can't RTBC though, I've written too much of the code in this patch. Hopefully @phenaproxima will RTBC soon.

1. +++ b/core/modules/media/js/media_embed_ckeditor.theme.es6.js
   @@ -0,0 +1,33 @@
   +   * @return {string}
   +   *   A string representing a DOM fragment.
   +   */
   +  Drupal.theme.mediaEmbedPreviewError = () =>
   +    `<div>${Drupal.t(
   +      'An error occurred while trying to preview the media. Please save your work and reload this page.'
   +    )}</div>`;
   

   Let's add a @see comment to reference the template here.

2. +++ b/core/modules/media/templates/media-embed-error.html.twig
   @@ -0,0 +1,16 @@
   + * @ingroup themeable
   + */
   +#}
   

   Let's add @see comment here to reference the theme function.

3. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -237,25 +237,11 @@ protected function renderMedia(MediaInterface $media, $view_mode, $langcode) {
   -      '#type' => 'html_tag',
   -      '#tag' => 'div',
   

   💯

4. +++ b/core/modules/media/templates/media-embed-error.html.twig
   @@ -0,0 +1,16 @@
   +
   
   +++ b/core/themes/classy/templates/content/media-embed-error.html.twig
   @@ -0,0 +1,17 @@
   +
   
   +++ b/core/themes/stable/templates/content/media-embed-error.html.twig
   @@ -0,0 +1,16 @@
   +
   

   🕵️‍♂️ Ubernit: We can remove this line changes because otherwise, they end up in the actual rendered output.

Fixing nits in #29 and #31

Doing my best to address #33

1. +++ b/core/modules/media/js/media_embed_ckeditor.theme.es6.js
   @@ -0,0 +1,35 @@
   +   * @see \Drupal\media\Plugin\Filter\MediaEmbed::renderMissingMediaIndicator()
   
   +++ b/core/themes/classy/templates/content/media-embed-error.html.twig
   --- /dev/null
   +++ b/core/themes/stable/templates/content/media-embed-error.html.twig
   

   We can probably just reference the template directly. As in @see media-embed-error.html.twig.

2. +++ b/core/modules/media/templates/media-embed-error.html.twig
   @@ -0,0 +1,21 @@
   + * @see Drupal.theme.mediaEmbedPreviewError
   
   +++ b/core/themes/classy/templates/content/media-embed-error.html.twig
   @@ -0,0 +1,22 @@
   + * @see Drupal.theme.mediaEmbedPreviewError
   
   +++ b/core/themes/stable/templates/content/media-embed-error.html.twig
   @@ -0,0 +1,21 @@
   + * @see Drupal.theme.mediaEmbedPreviewError
   

   👍

3. #33.4 still needs to be addressed.

Trying again to address #33.

+++ b/core/themes/classy/classy.libraries.yml
@@ -97,3 +97,14 @@ user:
+      css/components/media-embed-error.css: {}

We should load this CSS in the CKEditor as well by using the ckeditor_stylesheets property in the classy.info.yml file.

😵🤦‍♀️ Of course!

1) Adding ckeditor_stylesheets to classy.info. I confirmed without this, it's not styled in the CKEditor and with it, it's styled. Added an assertion to CKEditorIntegrationTest::testErrorMessages().
2) I was getting an error due to CKEditor trying to load a css file that no longer exists in DrupalMedia::getCssFiles(), so removed that.

-      $this->moduleExtensionList->getPath('media') . '/css/filter.media_embed.css',

3) I checked and found the library deleted from media, 'media_embed' was still in stable, so removed that and it's css file.

• #41.1: test coverage, yay!
• #41.2 + #41.3: thanks! 👍 I missed those in #17! 😅

Fixing the test failure, due to new ckeditor_stylesheets addition.

The patch looks good. Could we open a change record to notify theme maintainers not using Stable of the new template and theme function?

+++ b/core/modules/media/js/media_embed_ckeditor.theme.es6.js
@@ -0,0 +1,35 @@
+   * @param {string} error
+   *   The error message to display
+   *

Removed this on commit because the param doesn't exist. I also used yarn prettier to fix eslint failures.