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

  1. Write a patch
  2. Review

User interface changes

Search result info is no longer double escaped

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.
Files: 
CommentFileSizeAuthor
#6 2422039-6.patch1.72 KBidebr
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,737 pass(es). View
#6 interdiff-6-2.txt1.76 KBidebr
#6 2422039-6.fail_.patch1.06 KBidebr
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,632 pass(es), 1 fail(s), and 0 exception(s). View
search-result-double-escaped.png124.91 KBidebr

Comments

idebr’s picture

idebr’s picture

Status: Active » Needs review
FileSize
730 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,203 pass(es). View

Attached patch fixes the double escaping by applying an inline template.

jhodgdon’s picture

Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks 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.

idebr’s picture

Assigned: Unassigned » idebr

Sure, I'll add a test.

jhodgdon’s picture

Excellent!

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.

idebr’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,632 pass(es), 1 fail(s), and 0 exception(s). View
1.76 KB
1.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,737 pass(es). View

Thanks for the pointers, @jhodgdon. I added a test to SearchNodePunctuationTest that's already in HEAD.

@tstoeckler suggested in a similar issue #2369987: Remove SafeMarkup::set() from 'head' title on template_preprocess_html in #23:

Is the explicit call to the renderer needed at all? AFAIK Twig checks whether a thing is a render array before printing it and calls the renderer itself. That would also allow for alterability in other preprocess functions.

The call to the renderer service was not necessary indeed, so I removed it from this patch as well.

idebr’s picture

Assigned: idebr » Unassigned

The last submitted patch, 6: 2422039-6.fail_.patch, failed testing.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Excellent 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.

jhodgdon’s picture

Issue summary: View changes

forgot a couple of HTML comment closures that should have been removed in summary, oops.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 584907e and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 584907e on 8.0.x
    Issue #2422039 by idebr: Double escaping in search result info
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.