Allow the possibility to specify which tags to be stripped when Strip HTML checkbox is ticked. (allow-list)

Improvements... mostly stolen from the Advanced Text Formatter module...

When using the patch from #3 in this issue, if you leave the "Allowed HTML tags" field empty it results in the following error. Can the patch be adjusted to account for an empty value?

TypeError: Argument 2 passed to Drupal\Component\Utility\Xss::filter() must be of the type array or null, string given, called in /var/www/site/docroot/modules/contrib/smart_trim/src/Plugin/Field/FieldFormatter/SmartTrimFormatter.php on line 255 in Drupal\Component\Utility\Xss::filter() (line 57 of core/lib/Drupal/Component/Utility/Xss.php).

Drupal\Component\Utility\Xss::filter(Object, '') (Line: 255)
Drupal\smart_trim\Plugin\Field\FieldFormatter\SmartTrimFormatter->viewElements(Object, 'en') (Line: 80)
Drupal\Core\Field\FormatterBase->view(Object, 'en') (Line: 262)
Drupal\Core\Entity\Entity\EntityViewDisplay->buildMultiple(Array) (Line: 321)
Drupal\Core\Entity\EntityViewBuilder->buildComponents(Array, Array, Array, 'teaser') (Line: 22)
Drupal\node\NodeViewBuilder->buildComponents(Array, Array, Array, 'teaser') (Line: 263)
Drupal\Core\Entity\EntityViewBuilder->buildMultiple(Array) (Line: 220)
Drupal\Core\Entity\EntityViewBuilder->build(Array)
call_user_func(Array, Array) (Line: 378)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 490)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 64)
__TwigTemplate_b9d0525dc2fa6d439aa7b0e04d9fba5e068aae4a3e15afa8eb9cbc8c797dc620->doDisplay(Array, Array) (Line: 432)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 403)
Twig_Template->display(Array) (Line: 411)
Twig_Template->render(Array) (Line: 64)
twig_render_template('core/modules/views/templates/views-view-unformatted.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('views_view_unformatted', Array) (Line: 437)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 450)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 490)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 114)
__TwigTemplate_532bd15a1ee96b91d410ad8b8f05717c156aa86733c3c964a524ef7b00f7f6db->doDisplay(Array, Array) (Line: 432)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 403)
Twig_Template->display(Array) (Line: 411)
Twig_Template->render(Array) (Line: 64)
twig_render_template('themes/contrib/bootstrap/templates/views/views-view.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('views_view', Array) (Line: 437)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 450)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 226)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 227)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 156)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 67)
Drupal\simple_oauth\HttpMiddleware\BasicAuthSwap->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 51)
Drupal\jsonapi\StackMiddleware\FormatSetter->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 665)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

See if this works...

I took a different approach. Not sure which is best. I'm testing your patch now. Here is mine, based on yours from #3.

#5 seems to work. Thanks!

Added missing schema...

#8 applies cleanly and no errors reported. Keeping RTBC.

Patch fails to apply after latest commits. Sorry for the delay and out of order commits.

I have updated the patch so it applies and fixed some CS issues. Looks good except I would like to see a better description for the form. Nothing in the referencing link is clear about how to format the tags.. are they separated by spaces or commas? Do you add the <> or just the tag itself? Thanks for your help!

Patch #8 works for me on the default body field, but not on a Text (formatted, long) field.

Attached is the same changes from #12 rerolled for the latest released version of smart trim 8.x-1.2. This does not address @sacarney's comment about only working on body fields. I see why, but not sure how to fix.

This is the code which is causing this setting to only appear, once checking 'Strip HTML', on body field.

      '#states' => [
        'visible' => [
          ':input[name="fields[body][settings_edit_form][settings][trim_options][text]"]' => ['checked' => TRUE],
        ],
      ],

This same body-hard-coded technique is used in three other places in SmartTrimFormatter.php.

https://git.drupalcode.org/project/smart_trim/blob/8.x-1.x/src/Plugin/Fi...

https://git.drupalcode.org/project/smart_trim/blob/8.x-1.x/src/Plugin/Fi...

https://git.drupalcode.org/project/smart_trim/blob/8.x-1.x/src/Plugin/Fi...

re-rolled patch from #14 for a 8.x-1.3 version of the module.

Just created a merge request with:

1. #15
2. A fix for the #states problem reported in #14

Interdiff: https://git.drupalcode.org/project/smart_trim/-/merge_requests/6/diffs?c...

I'm attaching a 40sec demo of the fix for #14.

I didn't test the actual trimming behavior obtained by using #15. For #12, I noticed that entering 'em strong' would get automatically converted to '<em> <strong>', but didn't try adding some more details in #description.

Could we please reroll this against 2.0.x and ensure it uses the same implementation as the views functionality mentioned in the parent meta issue? That would make review a lot easier and ensure it's safe :)

Code looks promising so far, great work all!!

re-roll for 2.0.x, based on a patch from a #15.

And here we are, 8 months later :)

re-roll for 2.1.x, based on a patch from a #15.

re-roll for 2.1.0, based on a patch from a #15.

Thanks for the updated patch @Stockticker. It's working for us!

I'm not sure whether this new patch "uses the same implementation as the views functionality mentioned in the parent meta issue?", so not marking RTBC.

MR was updated to reflect the proper branch. Needs updating or re-rolled patch applied. Might have to close and open a new MR.

I rebased onto the 2.1.x branch.

I had to change:

           // https://stackoverflow.com/questions/12824899/strip-tags-replace-tags-by-space-rather-than-deleting-them
           $output = preg_replace('/<(\w+)(?![^>]*\/>)[^>]*>/', ' <\1>', $output);
-          $output = strip_tags($output);
           $output = str_replace('  ', ' ', $output);
+          $output = strip_tags($output, $this->getSetting('trim_options_allowed_html'));
           $output = trim($output);
 
-          $output = Xss::filter($output, $this->getSetting('trim_options_allowed_html'));

Seems to be working so far.

Very helpful functionality that was also requested in #3220684: Add preserve tags option to formatter

I've redone the last MR (68) as the rebase wasn't done correctly and it appeared to contain duplicated code that was already in 2.1.x...

I've done a new clean MR (92) with the patch re-rolled which is working nicely 👍

Did a few more tweaks to the work here in MR-92 to get tests passing, which they are now!

Thanks to all those who have contributed to this issue, however in recent testing after first applying patch from the older MR-68 and then later from MR-92 I've found that allowed tags attributes are removed. So if you allow <a> tags the href="[URL]" (and any other attributes) are removed, breaking the links.

The last working patch was https://www.drupal.org/files/issues/2023-07-05/smart_trim--2901579--allo... (still listed here) from July 2023 against version 2.0.1. It looks like it was actually an unrelated change release (2.1.1) of this module. Changes were from https://www.drupal.org/project/smart_trim/issues/3369954

Code above patch changes in src/Plugin/Field/FieldFormatter/SmartTrimFormatter.php from MR-92

           $output = preg_replace('/<(\w+)(?![^>]*\/>)[^>]*>/', ' <\1>', $output);

Code above patch changes in src/Plugin/Field/FieldFormatter/SmartTrimFormatter.php from patch #25

           $output = str_replace('<', ' <', $output);

Reverting this change after applying the latest patch, or reverting back to version 2.1.0 and using the older patch both restore links in allowed <a> tags.

This patch will specify which elements will be deleted and force them only by not using the default strip after them.

This patch removes the default 'img' from #41

Was looking for a way of doing this via twig, but didn't see any other way.

I think the configuration for listing the allowed tags should be built in a more user friendly way and then converted to a format that is passed to preg_replace(), rather than passing that value directly in - the risk for mucking up the syntax is a little high otherwise.

Here's a new patch that takes MR-92 (which we were using previously), and then fixes the regex issues called out in #39 which came from #3369954: [regression] Space inserted before full stop, comma, etc which was merged in the meantime.

I'm not personally fond of the approach used in #41 and #42 which use some new custom regexes, rather than relying on Xss::filter().

@dsnopek I tested applying your patch (which only applies cleanly to the active development branch for the 2.2 versions - 2.x-dev) and the functionality works as expected.

I also prefer this approach vs. the other approaches using custom regexes.

I haven't done a full code review yet, but some initial thoughts:

1. I would love an MR instead of a patch, but I know how old-school @dsnopek is so I'll let this one slide (for now.)
2. I would much prefer if the "Allowed HTML tags" text field appeared directly under the "Strip HTML" checkbox. Perhaps we can change the order of the checkboxes to move "Strip HTML" to the bottom and then move "Allowed HTML tags" underneath it.

thanks,
-mike

I made a MR out of dsnopek's patch, moved the setting to underneath the option (per ultimike's request), and renamed the variable from "trim_options_allowed_html" to just "allowed_html".

@damienmckenna - thanks for the MR and the change.

I went ahead and changed the order of the checkboxes so that "Strip HTML" is at the bottom.

During DrupalEasy Office Hours, we noticed that there were a couple of PhpStan issues for next minor and previous major - we attempted to fix them all, but ran out of time so I suspect there's still a little more work to do.

One is a bit of a chicken-and-egg issue, where support for annotations is deprecated and will be removed in Drupal 12, but using annotations results in PhpStan issues with previous major. Any ideas?

We didn't have time to try this, but maybe something like this is the solution?

/**
* Test the smart trim tokens.
*
* @group smart_trim
*
* @phpstan-ignore-next-line
*/
#[Group('smart_trim')]

-mike

I believe I have fixed the annotation PHPStan issue in #3545263: Fix PHPStan for previous major. If that fix is valid, I suggest merging that first, then rebasing this one.

All tests are green now.

It appears I missed the issue that the link href etc was stripped with that MR I made last year :facepalm:

MR-115 appears to be working well locally, confirmed the links are still there!

I've made a patch of the MR-115 that applies cleanly against the latest 2.2.0 release to save having to run a dev version of the module on a production site, in case anyone needs it before a new release of the module is cut.

@rossb89, thanks for the patch for the lastest 2.2.0 release! It seems to be working well in my site.

Let's get this finished!

Thanks @loze! Before merging this, it should please have tests to ensure it works as expected.

This is looking pretty good, but still needs work:

• When "Strip HTML" is selected, if nothing is added to the "Allowed HTML tags" box, then all tags are stripped. This is, IMHO, the proper behavior. There needs to be a test for this and something in the help text indicating it.
• The help text for the "Allowed HTML tags" box should also include an example of allowed input (<a> <strong>, for example.) It should also indicate that the list is space separated, not comma separated. IMHO, if there are any commas entered, then it should throw a validation error.
• IMHO, there is no need for a link to "Xss::filter" in the help text.
• Some people might assume that the "Allowed HTML tags" box operates like the "Allowed HTML tags" box on text format configurations (for example: /admin/config/content/formats/manage/restricted_html) This is not the case, because our box doesn't support HTML properties. I think a short note about this should also be included in the help text.
• I 100% echo @Anybody's suggestion for tests. We need full test coverage for this feature and all of the possible crazy things that folks might put in the "Allowed HTML tags" box.

-mike

Thanks for the review. I rebased !115 onto 2.x and went through each point in its own commit.

1. Empty box strips everything: help text now says "Leave empty to strip all HTML tags", and there is a test for it.
2. Example and separator: help text gives <a> <strong> as the example and says the list is space separated. A comma now gives a validation error, with a test for both the accepted spellings and the rejected ones.
3. Removed the Xss::filter() link.
4. Attributes: help text now says "tag names, without attributes". I left the text format comparison out to keep it short.
5. Tests: new AllowedHtmlTest covers an empty list, a normal list, case in the content and in the box, unsafe attributes on kept tags, Strip HTML off, an unknown tag name, and the validation above.

The full help text is now: "Space separated list of tag names to keep, without attributes, for example <a> <strong>. Leave empty to strip all HTML tags."

One thing I found while writing the tests and did not change: "Strip HTML" already removes script and style before the allowed list is applied, so listing them does nothing, and there is a test for that. Other tags have no ceiling, so listing iframe lets it through. Only someone who can edit display settings can do that, and Xss::filter() still strips onclick, style and the other unsafe attributes regardless. If you would rather the box refuse tags like iframe I can add that.

Also fixed on the way: tag names typed in upper case, like <STRONG>, matched nothing because the content side is compared in lower case. They are now lowercased on save.

The follow-up commits were written with AI assistance. I have reviewed them and CI passes.

Commented. Of course the final word here should have @ultimike, but personally I am not a big fan of the current implementation (yet).

Thanks @grevil - I just compared this to the Views implementation: https://git.drupalcode.org/project/drupal/-/blob/main/core/modules/views...

And left some comments accordingly. I'd vote to solve it like Core does, as that is a very mature implementation and should "simply" do the same. So we can rely on it and use the same naming and logics.

In general I think this is fine in 95% ready. Would be great to get feedback from @ultimike so we can finish this.

Thanks for the tests, that's important!

@anybody Also a good idea! Sticking to core makes sense most of the time! :)

Waiting for feedback from @ultimike to finalise this!

I really like the idea of modeling the logic, variable names, and help text after the way that Views implements its similar functionality - wish I thought of it! For reference:

Actually, we might as well also ask your coding agent to model the tests for this after similar tests for Views!

thanks,
-mike

@ultimike thanks for the confirmation.

Thanks @loze!! Is all aligned with views now, including the translation texts and logic?
If yes, I think this is ready for final review and merging afterwards?

@loze - good point about not changing the variable name (to avoid an update hook.)

Everything looks pretty good - my only question is that maybe we should be using a data provider instead of numerous test functions in the kernel test? I'd also like to see a test that takes into account what happens if someone does add something like <div class id> into the list of tags to preserver.

Regardless, thanks so much everybody for your hard work on this!

thanks,
-mike

Thanks @ultimike. Both done in the MR.

PreserveTagsTest is now one test method with a data provider. Each row is the body text, the preserve_tags setting as typed, and the exact trimmed text the formatter produces.

Two new rows cover <div class id> and <div class="x" id="y">. The list goes to strip_tags() as is, the same as the Views option, and PHP only honors entries of the exact form <name>. An entry with attributes preserves nothing: the div is stripped, the text survives, no attribute leaks.

Alright, just did some minor adjustments. Otherwise LGTM! :)

I feel like the views implementation in core is not the cleanest in hindsight, but good enough and lets us work with minimal code here!

RTBC from my side. Feel free to review my last commit.

@loze, @grevil and @anybody - thanks for the quick work on this - I really like the refactored test - much easier to read (and extend)!

I'm cool with this, but I am also going to ping @markie and @lostcarpark to take a quick look at it as well before merging.

Hopefully, we'll be able to get this merged later today!

-mike

Hi,

I have reviewed the change and it makes sense to me. I've also tested locally, and haven't come across any issues.

As it's already RTBC, I'll just say "+1 from me".

Just one thing it would be nice is running the tests against other versions (I think they'd fail against next major ATM, but others would be nice to see. Would be good to run some of those during merge.

Agree with Mike, the new test is nicely done.

Merging - thanks, everyone!

@lostcarpark - I've been working at fixing deprecations and other stuff in #3620918: Drupal 11.5.x test failures (SchemaIncompleteException)

-mike