Document correct filter_html_image_secure + filter_align + filter_caption + entity_embed filters order, write tests, and ensure correct by default

I believe this is an Entity Embed issue, as the actually embedding happens outside of File Browser's domain. Should we move this issue to their queue?

Actually, you only have to change the order of the filters, not disable them.

@dagmar What order should the filters be in? I can go ahead and update the project description with this information if it will help users.

Sure. Actually this belogns to entity embed project.

The entity embed filter should be placed after the Restrict images to this site

I've noticed this problem in the past too. However, I think that I've seen it working on some sites. If that is true it could indicate that this problem depends on the environment configuration.

Adding a note to the docs is a simple first step, but I think that we should check why that happens and make sure image URLs are built in a way that avoids this problem.

Related: #2969704: Document correct filter_align + filter_caption + entity_embed filters order, write tests, and ensure correct by default. Merging that into this issue. Updated IS.

This blocks #2614350: entity_embed's text filter should be enabled by default … but in which text formats?.

1. Should we modify the plugin definition and hack their default values?

   I'd argue that wouldn't be a hack, but a minor improvement. See filter.format.basic_html.yml:

     filter_align:
       id: filter_align
       provider: filter
       status: true
       weight: 7
       settings: {  }
     filter_caption:
       id: filter_caption
       provider: filter
       status: true
       weight: 8
       settings: {  }
   

   and filter.format.full_html.yml:

     filter_align:
       id: filter_align
       provider: filter
       status: true
       weight: 8
       settings: {  }
     filter_caption:
       id: filter_caption
       provider: filter
       status: true
       weight: 9
       settings: {  }
   

   IOW: the two default formats in core are already enabling those two filters and are making them run early.

2. As it turns out \Drupal\filter\Plugin\Filter\FilterHtmlImageSecure has weight = 9 in its annotation, so I guess setting 10 as our default weight should be enough for issue #1.

   Shouldn't \Drupal\entity_embed\Plugin\Filter\EntityEmbedFilter run before \Drupal\filter\Plugin\Filter\FilterHtmlImageSecure?

3. Also, I couldn't quite get the <figcaption> thing working as described in the IS, I'm probably missing something, will continue on this tomorrow.

   Neither can I. I just tried to reproduce this. I:

   1. did a fresh D8 install
   2. went to /admin/config/content/embed, created a Media embed button
   3. went to /admin/config/content/formats/manage/basic_html and turned on the Display embedded entities filter + dragged the "Media" button I just created into the active toolbar
   4. went to /node/add/article and inserted an image from my Media Library, aligned it to the center and captioned it
   5. … and upon saving, the node looked perfectly fine!

   So given that … I'm not sure about the correctness of

4. Given that, I also couldn't reproduce the first problem actually … the one that @marcoscano provided detailed STR for in #8. Turns out it's because Marcos was using the "Thumbnail" formatter, which allows one to pick an image style, and image styles are known to not work when using FilterHtmlImageSecure, see #2528214-51: "Restrict images to this site" blocks image style derivatives.

Conclusion: AFAICT there's nothing we need to do here, except document explicitly that the current default weight of zero is fine, and this filter should run:

• before the alignment filter
• before the captioning filter
• before the "restrict images to this site only" filter

but that #2528214: "Restrict images to this site" blocks image style derivatives really needs to be fixed.

Would it be worth adding validation to the form to enforce this order is respected? (since we know if they are outside of this order things won't work).

Yes! That'd allow us to have much more confidence that it's working correctly.

+++ b/entity_embed.module
@@ -85,3 +86,45 @@ function entity_embed_entity_embed_display_plugins_for_context_alter(array &$def
+function entity_embed_filter_format_edit_form_validate($form, FormStateInterface $form_state) {

This only works on the form. Ideally … we'd be able to surface this in the status report. Then existing sites would also benefit from this.

But that would require the validation to live at the config level, and that'd pretty much mean blocking this on #2869792: [meta] Add constraints to all config entity types, which would be silly.

So I'm fine with doing this only at the form level. That's pragmatic.

As discussed: #2528214-53: "Restrict images to this site" blocks image style derivatives:

@marcoscano pointed out while discussing this in #2752253: Document correct filter_html_image_secure + filter_align + filter_caption + entity_embed filters order, write tests, and ensure correct by default that it actually doesn't make sense for this filter to be filtering <img src=…> that wasn't entered by the content author: if that HTML is imported from other content on the site (transcluded, embedded …), then it shouldn't be ignoring (i.e. not securing) the transcluded HTML.

This is a way we could achieve that.

1. +++ b/entity_embed.module
   @@ -85,3 +86,38 @@ function entity_embed_entity_embed_display_plugins_for_context_alter(array &$def
   +    && !empty($filters['entity_embed']['weight'])
   +    && ($filters['filter_align']['weight'] >= $filters['entity_embed']['weight'])) {
   

   Doesn't actually work yet because this contains strings; the string "0" is considered "empty" by empty().

2. +++ b/entity_embed.module
   @@ -85,3 +86,38 @@ function entity_embed_entity_embed_display_plugins_for_context_alter(array &$def
   +    $form_state->setErrorByName('filters', t('The "Entity Embed" filter needs to be placed before the "Align images" filter.'));
   

   "Entity Embed" is not the actual label of this filter.

3. +++ b/entity_embed.module
   @@ -85,3 +86,38 @@ function entity_embed_entity_embed_display_plugins_for_context_alter(array &$def
   +    && ($filters['filter_caption']['weight'] >= $filters['entity_embed']['weight'])) {
   

   More importantly: the >= condition should be <= .

Logic is simplifiable.

Removing defunct D8Media tag.

Very useful patch, and makes sense to me. Couple of things...

1. +++ b/entity_embed.module
   @@ -85,3 +86,45 @@ function entity_embed_entity_embed_display_plugins_for_context_alter(array &$def
   +  $get_filter_label = function ($filter_plugin_id) use ($form) {
   +    return (string) $form['filters']['order'][$filter_plugin_id]['filter']['#markup'];
   +  };
   

   This seems like a strange and somewhat unreliable way of getting the filter label. Can we use the filter plugin manager to retrieve it directly from the plugin definition instead?

2. +++ b/entity_embed.module
   @@ -85,3 +86,45 @@ function entity_embed_entity_embed_display_plugins_for_context_alter(array &$def
   +        '%align-filter-label' => $get_filter_label('filter_caption')
   

   The key here looks like copypasta...

1. This seems like a strange and somewhat unreliable way of getting the filter label.

   If we need to consider this unreliable, then we also need to consider this form-based validation logic unreliable. Because that too depends on the form structure remaining the same.

   Like I said in #15, ideally this would be implemented using validation constraints. But that'd block this on #2869792: [meta] Add constraints to all config entity types, which doesn't make sense.

2. You're right! Fixed.

Committed and pushed, thanks!

Yay, #2614350: entity_embed's text filter should be enabled by default … but in which text formats? is now unblocked!

Looking at this again, I think we need to re-evaluate this.

Even though I managed to convince @marcoscano, I think he was right after all :)

One of the consequences of running the entity_embed filter before the alignment filter is that it results in a caption with a class of caption-article for entities, rather than caption-entity-embed. IOW: we lose the semantics of what we're captioning. IOW: letting captioning and alignment filters run before the Entity Embed filter, unlike what I said before.

If we do that, then we might as well run the Entity Embed filter last, which then also achieves @marcoscano's point, which I originally felt wasn't as important, but now I've become convinced it's actually better: it means that no filters will ever mess with content (HTML) transcluded (embedded) from other sources (entities or otherwise), which is great. Filters exist for securing and transforming user input, and transcluded/embedded content is not user input.

On top of that, this makes it much easier to explain to site builders how to fix their site.

Patch attached.

Manually testing I found two bugs. This error displays even if the Caption images filter isn't enabled:

"The Display embedded entities filter needs to be placed after the Caption images filter".

It needs to check $filters['filter_caption']['status'] as well. If status is 0, no comparison is needed, and no error should be thrown.

Also there's a copy error here:

if (isset($filters['filter_html_image_secure']['weight']) && $filters['filter_caption']['weight'] >= $filters['entity_embed']['weight']) {
+      $form_state->setErrorByName('filters', t('The %entity-embed-filter-label filter needs to be placed after the %image-secure-filter-label filter.', [

it should be comparing filter_html_image_secure's weight here not filter_caption.

👌🙏

fix the two issues above. Since the code is checking the same thing on all three, I put it in a loop.

It would be great to get this fixed. I'm trying to write functional tests and I'm having to include extra code to account for this bug where you must enable align filter and caption filter in order to enable entity embed filter.

    // Currently there is a bug where you must enable filter_align and caption
    // and have them in a certain order.
    // See https://www.drupal.org/project/entity_embed/issues/2752253
    $page->checkField('filters[entity_embed][status]');
    $page->checkField('filters[filter_align][status]');
    $page->checkField('filters[filter_caption][status]');
    $page->checkField('filters[filter_html][status]');
    $this->assertSession()->buttonExists('Show row weights')->press();
    $page->selectFieldOption('filters[entity_embed][weight]', '0');
    $page->selectFieldOption('filters[filter_caption][weight]', '1');
    $page->selectFieldOption('filters[filter_align][weight]', '2');

You shouldn't have to enable filter_caption and filter_align in order to enable the entity_embed filter!

With the JSON:API core patch finally having reached RTBC yesterday, I will look at this (and other Media issues) again starting tomorrow! Now going back to enjoy my day off :)

Per feedback in #33, simplified empty check and updated to use format plural. I renamed some variables to make them clearer.

Thanks @oknate and @marcoscano for pushing what I did in #29 to completion :)

1. #33: For my own understanding, I created a #29 to #33 interdiff (attached). It's just DRY refactoring. Great!
2. #35: with the commit of #3022754: @EntityEmbedDisplay=image plugin: no image-specific warning displays when invalid image selected earlier today, we should now be able to remove that. Rerolling for that.
3. #36 + #38: Well done!
4. The only remaining concern I have is that this validation only runs through the UI (it's using a #validate callback). Ideally, we'd be using validation constraints. #2969065: Use typed config validation constraints for validation of cdn.settings simple config has shown this is possible. But it requires validating the entire config entity, and it's not up to a particular contrib module (like this one) to enable that for FilterFormat config entities: if multiple contrib modules do this, we'd validate multiple times. Plus, it's possible some contrib modules get their validation constraints wrong. So … not holding this back for that. Especially if nothing in core is doing this today anyway.
5. Removing Needs documentation since the documentation is built-in.
6. That leaves Needs tests. Working on those.

Oops, I deleted a bit too much from what #3022754: @EntityEmbedDisplay=image plugin: no image-specific warning displays when invalid image selected added :)

And here are tests. The failing test-only patch is also the interdiff.

🥳

Here's a nit:

It's colloquial to leave out the noun and start with the verb "should". Perhaps we should be a bit more formal and change:

"Should usually run as the last filter,"

to

"This should usually run as the last filter,"

Embeds entities using data attributes: data-entity-type, data-entity-uuid, and data-view-mode. Should usually run as the last filter, since it does not contain user input

Sorry I didn't get this in before it was merged. It's not a blocker.

Thanks, Wim for getting this issue committed! Awesome.

I'm sure a lot of language in this module could use tightening. If you'd like to improve that, I suggest you create an issue specifically for UI string improvements :) Then all the credit will be yours too ;) :D

Sure, we crossed streams as you were committing. Thanks for getting this in!

Created change record: https://www.drupal.org/node/3051742