FilterCaption hard-codes allowed tags

I'm sorry, but this was a conscious choice back then to KISS, to have sane defaults. Having the ability to configure this is a nice-to-have, a pure addition, that we can do in 8.1.

To be clear: +1 to this, and I'm very grateful that you're working on this! :) But right now, we need to focus on getting RC1 out.

Looks like another patch is included with that one :)

Shouldn't the setting be called something like "allowed_caption_html"?

And shouldn't it also allow for attribute whitelisting, just like the HTML filter?

Rerolling Patch #32 because it won't apply to current 8.1.x. I can't do an interdiff for the same reason.

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImageCaption.php
@@ -62,10 +62,32 @@ public function getConfig(Editor $editor) {
+   *   String of the form '<tag1> <tag2> <a>'.
...
+   *   String of the form 'tag1 tag2; a[!href]'.
...
+      $anchor_tag = '; a[!href]';

The href attribute is hardcoded.

We should not do that. Instead, we should adopt the pattern that FilterHtml uses since #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default, with explicit whitelisting of attributes or even attribute values.

Because of that, this patch basically needs to be redone from scratch, because it is based on FilterHtml code from before that issue. It'll be easier to copy/paste the updated FilterHtml code than continuing with this patch.

Moving the test code to FilterKernelTest and rerolling the patch for 8.2.x.

Starting to look into redoing the patch with the updated FilterHtml code from Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default, which has been merged in to 8.3.x

Same patch. Changing Status to Needs review to get it tested.

This patch does not apply anymore on 8.5. Allowed tags are still hardcoded in the FilterCaption filter.

Updated the previous patch for 8.7.

Noticeable code wise differences:

1. In DrupalImageCaption.php, this line looked accidentally removed from the previous patch, so I have restored it: 'drupalImageCaption_alignFilterEnabled' => $format->filters('filter_align')->status,
2. Used ES6 as stated here: https://www.drupal.org/node/2815083
3. Updated config/install on umami install profile to use the same config as standard profile

I was able to patch on 8.5.4 and it works as expected on my site which uses entity embed. I have not tested with default captions on native inline images though.

Some tests may need a little refresh.

Added missing blank line.

#45 works great for me. RTBC+1

Thank you very much for working on this! 🙏 (Merci beaucoup!)

And shouldn't it also allow for attribute whitelisting, just like the HTML filter?

Definitely. I've updated the IS.

This is not yet done.

The feedback in #35 also has not yet been addressed.

Do you think you could also address that? 🙂

You may expect from me an update next week if noone has already addressed the feedback until then.

WOOT! 🎉 🚀

Hi, sorry for the delay.

This is still a work in progress. I have tried to reuse the same filter system as FilterHtml. It works as expected, exactly the same way as FilterHtml: tags are first stripped out, then invalid attributes are removed. Here are my thoughts:

1. The result between the old way and the new one is not 100% the same depending on some factors. My guess is that spaces before br tags are removed.
2. What would be the best way to reduce code duplicate? The system—a set of five public and protected functions—is 99% reusable between those two filters so it could be interesting to use the same system for both FilterHtml and FilterCaption.
3. In the original FilterHtml, there is a check on a filter config, filter_html_nofollow. This is why I had to add a new config to FilterCaption. Should we aswell let the user chose if he wants the rel="nofollow" attribute on links in the caption config? I am not sure about the consequences of this setting. Would it not be redundant with the "main" FilterHtml process if this one is also enabled?
4. And finally, the comments in FilterCaption states that only inline tags should be allowed. But this does not seem true, at least in the current days with the current HTML5 specs. Should we restrict the allowed tags, or should we let the user freely chose what he can insert in the captions? IMO, it could be bothersome to update this part of the code to follow the developments of the HTML standard so I would prefer to let it free.

Inputs needed. :)

Ooops sorry, I was not up to date with the 8.7.x branch.

This patch is still working well for me. I don't really know enough to give the feedback that was requested though.

Patch in #54 doesnt seem to be applyling to core version 8.9.6.

Re-rolling the patch in #54 against latest core 9.1.x dev version, so that it'll be easier to test by others and also attaching a diff_reroll file... Thanks!

Here's a re-roll of #54 that applies to 8.9.x in case anyone else is not on 9 yet like me.

And here's a reroll of #54 that applies to 8.9.6 (current HEAD has breaking changes)

Looks like the use declaration of Drupal\Component\Utility\Xss was removed in 8.9.7. Here is another reroll for that.

I think testbot is refusing to test because of coding standards issues.

Need a line break here between property definition and method doc:

   /**
+   * The processed HTML restrictions.
+   *
+   * @var array
+   */
+  protected $restrictions;
+  /**
+   * {@inheritdoc}
+   */
+  public function settingsForm(array $form, FormStateInterface $form_state) {

2 other issues reported with javascript - not sure if those are blocking tests or not...

This should give us a better idea of which tests are actually failing

We'll need to do this for the drupalmedia plugin as well.

I would love to be able use SUP and SUB tags inside my figcaptions. Anything I can do to bring this patch back to life so we can roll it into 9.3.x?
Thanks!

Here's the addition with the drupalmedia plugin

@rerolled for 9.4.x and fixed some CS errors from the patch.

Sorry, I forgot to rebuild the .js files.

Looks like test failures are unrelated.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

is this still an issue for ckeditor5?

Yes! I can confirm it is still an issue. See \Drupal\filter\Plugin\Filter\FilterCaption::process().
I cannot find any restriction on the CKEditor side anymore, though.

Backend:

Frontend:

Updated IS