Problem/Motivation

The labels for exposed sorts are currently being double-escaped.
Once by core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php (bug)
Once by core/lib/Drupal/Core/Render/Element/Select.php (by design)
So if your label includes special characters like an apostrophe (') those end up rendering as HTML entities inside your select dropdown:

Screenshot of double-escaped exposed sort labels

Proposed resolution

Do not escape the labels going into the select #options, since Drupal\Core\Render\Element\Select already does that for us.

Remaining tasks

  1. Fix the bug (see #11).
  2. Add test coverage.
  3. Upload test-only patch, see #16 -- watch it fail.
  4. Upload test + fix patch, see #17 -- watch it pass.
  5. Further reviews.
  6. RTBC.
  7. Commit.

User interface changes

Labels for Views exposed sorts will not be double-escaped in the select form element when a user chooses what to sort by. With the fix:

Screenshot of properly-escaped exposed sort labels (fix applied)

API changes

-

Data model changes

-

Release notes snippet

-

Original report by @imen ch

label exposed filter special characters (apostrophe) encoded,

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

imen ch created an issue. See original summary.

vakulrai’s picture

Status: Needs work » Needs review
FileSize
836 bytes

@imen ch I am adding a patch which will decode special character coming under exposed filter's

Prashant.c’s picture

@vakulrai

I think we should use https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Utility%21Html.php/function/Html%3A%3AdecodeEntities/8.6.x

MustangGB’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.6.x-dev
Component: User interface » views.module
Priority: Major » Normal

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

lorenzs’s picture

Status: Needs review » Reviewed & tested by the community

Tested and fixes french characters in standard exposed sorting option.

dww’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks for the work here. Agreed it's a bug.

But we need some test coverage of this so we don't re-break it in the future.

It'd hopefully be fairly easy to extend core/modules/views/tests/src/Functional/Plugin/FilterTest.php to add a test for this.

Ideally, we'd see a test-only patch that extends that test, watch it fail, and have a full patch that includes the fix from #3 along with the new test, and watch it pass.

Then this would be RTBC.

Thanks again!
-Derek

dww’s picture

Title: label exposed filter special characters (apostrophe) encoded » Exposed sort label is double-escaping special characters (apostrophe)

Oh, the screenshots and code are only dealing with the options in the exposed sorts drop-down, but the title of the issue was about exposed filters. Changing the title to match what this bug is about.

Also, I can't find any existing test coverage for exposed sorts. :( Hrm. So maybe this issue gets the honor of adding the first coverage for this feature to core. ;) Please do *not* add to core/modules/views/tests/src/Functional/Plugin/FilterTest.php. We should probably add a whole new test class for this.

Thanks/sorry,
-Derek

dww’s picture

Also, now that I'm seeing clearly what the bug is, the proposed fix in #3 is incorrect.
This is a value we're putting into the #options of a '#type' => 'select' form element. Those are already getting escaped. @see core/lib/Drupal/Core/Render/Element/Select.php:

 * - #options: An associative array of options for the select. Do not use
 *   placeholders that sanitize data in any labels, as doing so will lead to
 *   double-escaping. Each array value can be:
...

So we don't want to call Html::decodeEntities(), we simply do *not* want to call Html::escape() on them at all. Attaching a patch with the correct fix.

Still needs tests, so leaving at "Needs work".

Adding a real issue summary.

Cheers,
-Derek

dww’s picture

Issue summary: View changes
FileSize
41.72 KB

Forgot to attach this in my last comment... screenshot after the fix is applied.

dww’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.51 KB

Yay, I found some existing test coverage of exposed sorts. Phew! ;) So here's a small change to an existing test that shows the bug.

This will fail.

dww’s picture

Issue summary: View changes
FileSize
2.47 KB

And here's the test + the fix. This should pass.

The last submitted patch, 13: 2956722-13.test-only.patch, failed testing. View results

dww’s picture

Here's an even better test, since it includes the label <script>alert('unsafe&dangerous');</script> and we definitely want to make sure we handle that correctly. ;) Also includes another assertNoRaw() that the raw, unescaped label doesn't appear anywhere on the page.

This will fail.

dww’s picture

And with the fix. This will pass.

dww’s picture

FileSize
946 bytes

Dang, forgot to attach the interdiff...

The last submitted patch, 16: 2956722-16.test-only.patch, failed testing. View results

dww’s picture

Issue summary: View changes

Great, bot is again happy with expected results. Updating summary to point to the right patches.

RTBC?

Thanks!
-Derek

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

#17 addresses the only feedback I had for #14 so, nice one!

Good fix, good tests, ready to go from my end.

dww’s picture

Issue summary: View changes

Great, thanks!

lorenzs’s picture

Great effort @dww. Applied the latest patch and still fine.

alexpott’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 267186a64a to 9.0.x and b259138818 to 8.9.x. Thanks!

This came from Views 7.x where there was a check_plain.

Will ask for a backport once 8.8.x is unfrozen.

  • alexpott committed 267186a on 9.0.x
    Issue #2956722 by dww, Prashant.c, vakulrai, imen ch: Exposed sort label...

  • alexpott committed b259138 on 8.9.x
    Issue #2956722 by dww, Prashant.c, vakulrai, imen ch: Exposed sort label...
lauriii’s picture

+1 for a backport

alexpott’s picture

Status: Patch (to be ported) » Fixed

  • alexpott committed 071feee on 8.8.x
    Issue #2956722 by dww, Prashant.c, vakulrai, imen ch: Exposed sort label...
dww’s picture

Excellent, thanks!

Status: Fixed » Closed (fixed)

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