Strip tags from formatted text before trimming

Fixed the test case failure issue

Appears to be missing test cases and upgrade path.

Just ran into this while using metatag + glossify.

Glossify provides filters, which automaticall create tooltips / links for words, for which a node or a taxonomy term exists. These links use the linked node body, as the tooltip, which results in a VERY long "title" HTML Tag:

Now, if I would like to use the token [node:summary] inside a metatag or anywhere else, the summary won't be displayed correctly as you can see here:

Now this stems from the same issue, this issue is about. But the current MR doesn't solve it deeply enough. The real root cause of this is the

text_summary()

method from the core "text" module.

In my opinion we should strip the tags there. The method doc says the following:

Generates a trimmed, formatted version of a text field value

I don't think stripping HTML tags would change the documented functionality of this method.

Thanks, I can also confirm this issue and this is definitely a bug, not a feature request. The expected behaviour is to strip the affected text.

For example, using the Metatag module (which uses [node:summary] by default for node meta descriptions!), this bug will break the metatag generation if the shortened text has a long link in the beginning. The text will be stripped right before the link and that's it.

Run into that while working on Text trimmed by "Trimmed" or "Summary or trimmed" field formatters will get run through Xss::filter() potentially changing their content. We may want to merge / or close one of these issues.

@shmy: They're definitely related, but I don't see them as duplicate?

Still both should be solved (and maybe solved together).

But their MR overlap as they eventually try solve the same underlying issue (partly in the same way).

I actually agreed with @shmy that they were duplicates. So lets continue here. Working on the open tags.

Ready for some eyes

I found a couple of minor things that I put in the MR comments. The trimming works for me. I actually run into this problem yesterday while testing other Text module issues!

Applied 1 suggestion and answered another. Thanks for the review @dcam!

I just realized that the update function sets the settings to TRUE. That will cause the output to change for existing fields, which could cause problems. I think that it's appropriate for the setting to default to TRUE for new fields, but existing fields need to have it set to FALSE.

100% right. I’ll update

The FunctionalJavascript tests aren't executing properly on my local environment. And I can't re-execute them on GitLab. Do we know if they're random?

Not sure why it would fail when updating the update hook to be false but ran a new pipeline and still failed so will look into it

Appears to be random https://www.drupal.org/project/drupal/issues/3614445#comment-16715712

Did some code review and slightly adjusted the IS.

I agree with the general approach, but I feel like the hand-parsing of :: truncateHtml() as implemented is fragile (see issues found in the MR) and will end up being complex to maintain over time.

How about we switch the approach there, and cut the raw HTML at the byte offset where we've already detected the visible limit lies, and (accepting that the fragment contains incorrectly-closed HTML), just let core's HTML::normalize() repair it?

Something along the lines of:

 public static function truncateHtml(string $html, int $length): string {
    $visible = 0;
    $offset = 0;
    $in_tag = FALSE;
    $total = strlen($html);
    while ($offset < $total && $visible < $length) {
      $char = $html[$offset];
      if ($char === '<') {
        $in_tag = TRUE;
      }
      elseif ($char === '>') {
        $in_tag = FALSE;
      }
      elseif (!$in_tag) {
        $visible++;
      }
      $offset++;
    }
    // The fragment may end inside unclosed elements; normalize() repairs it.
    return Html::normalize(substr($html, 0, $offset));
  }

Should we have a Change Record for this? There is a new setting + new behavior + new public method, etc.

Also, I manually tested this on this issue's Tugboat, and I'm seeing the text example from the Issue Summary get truncated at character 71 instead of 100. Am I testing this wrong or the test coverage is not picking the scenario of the example?

Definitely will need a change record I believe

Disclosure I did use AI to help with the feedback but reviewed the changes. Manually testing still but thoughts?

Added a few comments to the review, and re-worded the Change Record to be more comprehensive.

AI disclosure: I have used an LLM to help analyze this issue, whose output I reviewed and filtered.

Think ready for eyes again.

I think there is one minor issue with the test, but apart from that I can't find anything else! 👍

Tested MR !16499. While testing found two cases where truncateHtml() with exclude_html_tags breaks.

First one. If the body has an HTML comment with a > in it before the real close, the cutoff can land inside it. Something like <!-- note: a > b --> or an old style <!--[if IE]>...<![endif]--> block. Then the teaser comes out empty. Tested with trim_length 20, both cases gave <p></p>, no text at all.

Second one. If the body has a <script> or <style> block, the cutoff can land inside it too. Then the raw code shows up as visible text on the page instead of getting dropped. Tested with trim_length 10, got <p>Hi</p>if (a > b) { for script, and <p>Hi</p>.x { con for style.

Both come from the same thing. truncateHtml() flips its tag state on every > character, so it does not know about comments or script/style blocks. A > inside either one ends that state too early.

Checked this on a real page load too, not just direct calls.

I noticed a few months ago (as far as I remember it was on a drupal 10) that something was wrong with the count when input contains \r\n: it should count as 1 but it counted as 2.
I'm not sure whether it was due to custom code or something from core, but it's worth keeping that in mind and checking this here too.

Thanks for continuing here @smustgrave.

I'm not convinced that a custom parser is the best approach here. It may become a continues maintenance burden to handle the next edge case and eventually providing a dedicated HTML parser or declaring that only a subset is supported. Some where already found by @meeni_dhobale, but there more. E.g.:
- a open/close tag might appear in the content itself (e.g. a <code> tag while doc on truncateHtml() declare HTML input the value might not have passed `htmlspecialchars()` and therefore might not have been decoded to `</>`; does the method needs to be public? afaik a private method would to the job as well)
- a closing tag may be present (escaped by ' or ") in a any attribute (e.g. data-something="who-knowns-why->" or an onClick attribute might contain `() => do something` (not sure why someone would pass that into a field, but there is nothing that prevents them from doing that when filters are disabled))

Im wondering what/if there is an the actual usecase for including the HTML tags lengh when calculating the length? In my option its just counterintuitive, but i might not have met that requirement yet. Im just worried we are committing to support a case that is very rarely, if ever, used.

Without the need to track the tag length it might be a lot easier process and manipulate DOMText values as HTML::normalize will eventually anyway hand over the content to that parser.

I guess the use-case for the length count is for BC. Did forget about that.

I feel sorry for disrupting the current approach.
And im not sure if, in the end, the mentioned DOM processing would more or less replicate TruncateHtml of smart trim module and/or there is a middle ground.

I've also did see the explicitly tested broken HTML output (mentioned in the summary) in TextSummaryTest but didn't had any time to check why that has been the accepted/intended behavior in the first place.