Text trimmed by "Trimmed" or "Summary or trimmed" field formatters will get run through Xss::filter() potentially changing their content

Thanks, this works for me.
Drupal version 8.6.13

Reroll for 8.8.

Sorry, wrong patch, This is a correct reroll for 8.8 version.

This almost sounds like a bug, which would need a test case.

This also should be retested in D10 to see if still an issue and we can go from there.

I've reproduced the issue with current 11.x HEAD and updated the steps in issue summary.

I've extended the patch (interdiff_9_19) by a safety check to ensure only markup, that is already passed as filtered, is back-casted from string. I'm not sure if the taken approach is sufficient.

As the ensureMarkupIsSafe renderer method the treats MarkupInterface as safe and TextTrimmedFormatter::preRenderSummary is a public static method and therefore may be called within a different context by contrib/custom modules (e.g. a render element that doesn't preserve the #pre_render callback preRenderText) iv'e tried to find a more generic implementation but as MarkupInterface doesn't dictate the `::create` method i couldn't find a practical approach.

As test coverage is needed anyway i hope to have some time in the next days and extend an existing ones or add a new one if necessary.

Minor remarks, looking good :)

As the implemented solution has been approved during review, i belief the proposed solution and remaining task can be updated in the issue summary. Back to review.

Going to find out if this falls under BC policy as this could change the display on existing sites which might not be desired.

Brought this up in slack #needs-review-queue-initative https://drupal.slack.com/archives/C04CHUX484T/p1708719216555289

I asked if this would be a D11 fix only because of the backwards compatibility impact and @catch replied "It wouldn't make it 11.x only but we might have to put it behind a settings.php switch or feature flag"

So maybe a field formatter setting could cover it? Know smart trim has something like that.

Thanks for taking care of BC.

I think the required change depends on issue classification.
In my eyes this is a bug fix, not a feature request, because as a user (i.e. site builder) i don't expect that my hard-coded markup is filtered out when summarized.

Since the activity (and probably demand) for a fix seems not very high why not make it 11.x only?

So we definitely missed the D11 boat but maybe this can make D12?

With regards to feature flags, we did something similar with layout builder using a module as a flag.

Fixed up the MR for main. I broke out the text_with_summary to it's own test so it can be easily removed when we deprecate text_with_summary

Thanks for updating the MR.

Regarding the feature flag, i'm assuming thats opt-in? If so i'm worried that the fix will be just a dead end. I can't remember when / if i ever looked up if there is something behind a feature flag and instead chose contrib / custom.
I still thing that is a bug fix, but may change the markup in an unexpected way and therefore opt-in is just the most practical solution.

The field formatter settings would make it at least a reachable option, so i would favor that appraoch.

This actually needs a summary update but it’s not an opt in feature as it’s fixing a bug.

As there is no API for feature flags yet (#3423352) the flag would need to be implemented as a text submodule, like done in layout builder in #3432874?

That was just a temporary stop gap. The module is being marked obsolete and deleted

I see, misread that. Then its just a field formatter setting.

I've reviewed the previous commits on the MR, these do look okay.

I'm not sure if that's how its supposed to be, but the behavior of the text_summary() function is not as i would expect. I.e. it doesn't subtract the HTML tag length when determining the output length. On TextFormatterTest::L108 and TextFormatterTest::L149 i would expect that the output is 17 chars (the length of the iframe tag) longer.
A more lengthy tag or heavily nested ones, in combination with a short trim length, may yield an empty output string or just expose the HTML markup without the actual content.

Not sure I agree with the field formatter setting but that now changes trajectory of this ticket

So do we continue or replace it with a module setting instead?

A correction on the text_summary() function note above: All of the lines should be longer, not only the most recently added ones.
Found Strip tags from formatted text before trimming, which already had an MR. Added as related.

Nice find, I do think we are achieving the same thing as the issue mentioned. Lets continue over there. Am giving credit to this issue.