The text_summary() function has an optional parameter to pass in the text format of the text whose summary is being calculated. If this is passed in, the function will do all sorts of special processing depending on which (core) filters happen to be in the text format.
This entire functionality makes no sense and should be removed.
The reason is that when text_summary() is called, check_markup() has already been run on the text that is passed in to it. (See the code in text_field_formatter_view() for evidence of this.) Thus, it does not matter what filters were used to generate it, because they've already run, so in the end we are just working with regular HTML regardless.
In particular, going through the text_summary() code:
// We check for the presence of the PHP evaluator filter in the current
// format. If the body contains PHP code, we do not split it up to prevent
// parse errors.
if (isset($format)) {
$filters = filter_list_format($format);
if (isset($filters['php_code']) && $filters['php_code']->status && strpos($text, '<?') !== FALSE) {
return $text;
}
}
This makes no sense. Any PHP code will have already been evaluated when check_markup() was called. If for some reason there is a '<?' left over, it does not indicate anything meaningful anymore and should not trigger any specific kind of behavior.
// If no complete paragraph then treat line breaks as paragraphs.
$line_breaks = array('<br />' => 6, '<br>' => 4);
// Newline only indicates a line break if line break converter
// filter is present.
if (isset($filters['filter_autop'])) {
$line_breaks["\n"] = 1;
}
Also makes no sense. If the line break filter is present, newlines will have already been converted to <br />. If it isn't, we don't want to treat them specially at all. (See also #1235062: text_summary() ignores filter status which shows that this code is being run even when the line break filter isn't present, which can break things even more.)
// If the htmlcorrector filter is present, apply it to the generated summary.
if (isset($filters['filter_htmlcorrector'])) {
$summary = _filter_htmlcorrector($summary);
}
This one almost makes sense, but not really. We do want to run the HTML corrector, but it shouldn't matter whether or not the text format used to generate the content had it. If text_summary() break things that would otherwise have been valid HTML, it is responsible for fixing what it broke. Example: If I type "<p>This is a test</p>" into a textarea, regardless of whether or not that textarea had the HTML corrector run on it to guarantee that the output is valid, the fact that I have provided it with valid HTML means it should stay valid. If the generated summary comes out as invalid HTML such as "<p>This is a" with no closing tag, that is a bug, and text_summary() should never let that happen.
So the conclusion is that text_summary() should run the HTML corrector if the text it winds up with contains any HTML at all. One might be concerned about the performance impact of that, but:
Patch coming up in a second.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | text_summary_checks-1347920-14.patch | 7.22 KB | mgifford |
| #14 | text_summary_checks-1347920-14.patch | 7.22 KB | thijsvdanker |
| #12 | text_summary_checks-1347920-12.patch | 6.51 KB | thijsvdanker |
| #1 | text-summary-1347920-1-TESTS-ONLY.patch | 5.68 KB | David_Rothstein |
| #1 | text-summary-1347920-1.patch | 14.07 KB | David_Rothstein |
Comments
Comment #1
David_Rothstein commentedThe first patch contains new tests only, which should fail. (They are sort of testing #1235062: text_summary() ignores filter status more than this issue directly, but the two issues are very closely related.)
The second patch contains the complete set of changes, and the tests should hopefully pass.
Comment #2
effulgentsia commented+1. Patch looks good to me, but should have at least one other reviewer before RTBC.
Changing the function signature is not backportable to D7, but some of the other cleanup here is, and IMO, should be, so tagging accordingly.
Comment #3
sunIt looks like you're not aware of #221257: text_summary() should output valid HTML and Unicode text ?
Comment #4
sunHappy to discuss possible options here.
My personal conclusion was to replace it entirely with a DOMDocument-based implementation; i.e., diving into the actual HTML markup and counting the actual text nodes contained within.
The only tidbit that would still be format/filter-specific is the check for the PHP code filter — cutting/trimming PHP code would result in a fatal error.
That said, #1235062: text_summary() ignores filter status can be fixed and backported, but whatever we're going to do here cannot be backported.
Comment #5
David_Rothstein commentedI actually linked that issue to this one in #221257-179: text_summary() should output valid HTML and Unicode text :) Does it affect anything here though? (It's been a while since I looked at this issue.)
I don't see why, since as described above check_markup() has already run and executed any PHP code before this. I checked the current D8 code and it still seems to be the same. text_summary() is called via this code:
And _text_sanitize() is basically a simple wrapper around check_markup().
Comment #8
jhedstromComment #9
David Hernández commentedI'm working on this
Comment #10
David Hernández commentedThis patch is too old. Is this issue still valid?
Comment #11
jhedstromThe php filter stuff is gone, but from what I can tell, points 2 and 3 in the issue summary are still present in the
text_summary()function.Comment #12
thijsvdanker commentedI am using the text_format as an api function to make a summary of some html text, and then it makes sense to have the format as an argument of the function.
Point 3 still stands: the summary function can mess up perfectly valid html, so it should always fix this (regardless of format settings).
This patch always normalizes the html at the end of the summary and I've updated the test to reflect this.
Comment #14
thijsvdanker commentedThe failing test tests if a formatter is applied to a field.
$this->assertEqual(strlen(html_entity_decode($rendered_field)), 3);It expected a string with
<p>8randomchars</p>/n to be trimmed to 3 charactars (<p)As the patch fixes this html, it returns
<p></p>I've changed the test to test for 7 characters (
<p></p>).Comment #15
mgiffordRe-uploading last patch for the bots.
Comment #27
smustgrave commentedThe code in question has gone through several changes since this patch. Can you verify it's still an issue on 9.5?
Comment #29
smustgrave commentedClosing as outdated since there has not been a follow up in 7 months.