Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Search results info are double escaped.
From core/modules/search.pages.inc
function template_preprocess_search_result(&$variables) {
$variables['info'] = implode(' - ', $info);
}
Before screenshot:
Proposed resolution
Fix the double escaping.
Remaining tasks
- Write a patch
- Review
User interface changes
Search result info is no longer double escaped
API changes
None
Beta phase evaluation
| Issue category | Bug because the public-facing search results page is showing double-escaped HTML, uck! |
|---|---|
| Issue priority | Major because this is on a public-visible page that will show up on any site using the Core Search module. |
| Unfrozen changes | Unfrozen because it only changes output markup |
| Prioritized changes | This is prioritized because it is a bug fix, also it's about UI. |
| Disruption | Not disruptive. Only changes one line of code in a theme preprocess function, plus a few lines of tests, and no API changes. |
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 2422039-6.patch | 1.72 KB | idebr |
| #6 | interdiff-6-2.txt | 1.76 KB | idebr |
| #6 | 2422039-6.fail_.patch | 1.06 KB | idebr |
| search-result-double-escaped.png | 124.91 KB | idebr | |
Comments
Comment #1
idebr CreditAttribution: idebr commentedComment #2
idebr CreditAttribution: idebr commentedAttached patch fixes the double escaping by applying an inline template.
Comment #3
jhodgdonThanks for the bug report and the patch!
This does look like the correct way to fix the problem, from what I can tell on the change notification mentioned on the parent issue: https://www.drupal.org/node/2311123 I also tested manually and this patch does fix the problem. The existing tests also pass.
So, the patch looks good!
However, I'm setting it to Needs Work because I think we need a new test added however, because obviously our tests didn't catch this problem when it first occurred. Are you interested in writing a test?
I'm also upping the priority to Major because I do not think we want to ship the Core search module with this problem on the search results page. It is pretty ugly, and visible to any site visitor with permission to search.
Comment #4
idebr CreditAttribution: idebr commentedSure, I'll add a test.
Comment #5
jhodgdonExcellent!
When you upload the patch with the new test, it will be helpful if you also upload a second patch with only the new test (which would also effectively be the interdiff, but upload it as a .patch file, with a name like "...-test-only-FAIL.patch"). Then the test bot will run tests on both patches, and we can verify that the new test fails in the expected way without the code patch, and passes with the code patch. Thanks!
Also as a note: you can probably just add a few lines to an existing test rather than writing a completely new test. We have several tests that display search results and look for text in them; just add a few more asserts that will verify the info line to one of those tests.
Comment #6
idebr CreditAttribution: idebr commentedThanks for the pointers, @jhodgdon. I added a test to
SearchNodePunctuationTestthat's already in HEAD.@tstoeckler suggested in a similar issue #2369987: Remove SafeMarkup::set() from 'head' title on template_preprocess_html in #23:
The call to the renderer service was not necessary indeed, so I removed it from this patch as well.
Comment #7
idebr CreditAttribution: idebr commentedComment #9
jhodgdonExcellent work! Thanks very much! The code and test look perfect. The -fail patch fails the way we'd expect it to, and the code+test patch passes. This is ready to go!
Adding beta evaluation to the issue summary.
Comment #10
jhodgdonforgot a couple of HTML comment closures that should have been removed in summary, oops.
Comment #11
alexpottCommitted 584907e and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.