text_summary() has this code at the end:
// If the htmlcorrector filter is present, apply it to the generated summary.
if (isset($format) && $filters->has('filter_htmlcorrector') && $filters->get('filter_htmlcorrector')->status) {
$summary = Html::normalize($summary);
}
TextSummaryTest has tests for the above to ensure it works.'filter_htmlcorrector' is redundant with 'filter_html' so is not used on formats that have the latter.text_summary() is not normalizing the summary for formats that use 'filter_html' instead of 'filter_htmlcorrector'. That includes the Basic HTML and Restricted HTML formats installed with Standard profile.Fix that if statement to also include a check for the 'filter_html' filter.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
oknateI'm having trouble recreating the issue where it breaks the markup. For me, if I set the display of a processed text render element with a Remote Video (using a YouTube url) to 'trimmed', it just disappears. If you get a chance, could you add a little more detailed steps to reproduce.
Comment #3
effulgentsia commentedIf I add https://www.youtube.com/watch?v=BNoCn6T9Xf8 as my video.
Then, create an Article with just that media entity in it, which after saving that Article, I confirm by clicking the "Source" button in the CKE toolbar and see that all it says is:
Now, when I view the Article on it own page, I see the video, and when I view the page's source, the corresponding article tag that this media got rendered as is:
This is just slightly over 600 characters. When I view this article on the frontpage as a teaser, then text_summary() trims it to 600, it cuts off the last 3 characters and just returns:
Then, Xss::filter() runs on it, and does two things:
1) Removes the iframe.
2) Removes the final
</articsince that's an incomplete closing tag.The result is:
That's what appears in my frontpage source. Which means there's an
<article>tag that's opened and never closed.Comment #4
oknateHere's a patch and test coverage. I originally created the test using the example in #3, but I think it's better to simplify it. I'm including the original for reference (3067116-4--oembed-example.patch).
Comment #10
naresh_bavaskarComment #11
naresh_bavaskarRerolling patch for latest branch 9.3.x
Comment #15
nishantghetiya commentedChanges make on above patch remove deprecated functions assertContains() and assertNotContains() replace with assertStringContainsString() and assertStringNotContainsString
Comment #16
phenaproximaThis looks good to me. I only have one nitpicky piece of feedback. We have a fail patch in #4, so that box is checked. I think, once my one tiny bit of feedback is addressed, I'd be very happy to mark this RTBC.
Comment #17
phenaproximaAdding to the Bug Smash Initiative.
Comment #19
dwwMade @phenaproxima's suggested changes. Back to NR.
Comment #20
phenaproximaThanks, @dww! RTBC on the assumption that tests will pass.
Comment #21
larowlanCrediting @effulgentsia who did a lot of the original legwork here in #3
I think given this generates broken markup it qualifies as Major
Comment #23
larowlanMerged this to 9.3.x
Going to get a second opinion on backporting to 9.2.x, which I'm in favour of, but because this will change markup on sites (albeit to correct it), going to err on the side of caution.
Comment #24
larowlanDiscussed with @xjm who agreed with the risk of disruption here
Leaving as fixed and adding a change record