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:
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
Fix the bug(see #11).Add test coverage.Upload test-only patch, see #16 --watch it fail.Upload test + fix patch, see #17 --watch it pass.Further reviews.RTBC.- 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:
API changes
-
Data model changes
-
Release notes snippet
-
Original report by @imen ch
label exposed filter special characters (apostrophe) encoded,
Comment | File | Size | Author |
---|---|---|---|
#18 | 2956722.13_16.interdiff.txt | 946 bytes | dww |
#17 | 2956722-17.test-and-fix.patch | 2.58 KB | dww |
#16 | 2956722-16.test-only.patch | 1.61 KB | dww |
Comments
Comment #2
vakulrai CreditAttribution: vakulrai as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented@imen ch I am adding a patch which will decode special character coming under exposed filter's
Comment #3
Prashant.c@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
Comment #4
MustangGB CreditAttribution: MustangGB commentedComment #8
lorenzs CreditAttribution: lorenzs at Cegeka commentedTested and fixes french characters in standard exposed sorting option.
Comment #9
dwwThanks 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
Comment #10
dwwOh, 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
Comment #11
dwwAlso, 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. @seecore/lib/Drupal/Core/Render/Element/Select.php
:So we don't want to call
Html::decodeEntities()
, we simply do *not* want to callHtml::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
Comment #12
dwwForgot to attach this in my last comment... screenshot after the fix is applied.
Comment #13
dwwYay, 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.
Comment #14
dwwAnd here's the test + the fix. This should pass.
Comment #16
dwwHere'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.
Comment #17
dwwAnd with the fix. This will pass.
Comment #18
dwwDang, forgot to attach the interdiff...
Comment #20
dwwGreat, bot is again happy with expected results. Updating summary to point to the right patches.
RTBC?
Thanks!
-Derek
Comment #21
Lendude#17 addresses the only feedback I had for #14 so, nice one!
Good fix, good tests, ready to go from my end.
Comment #22
dwwGreat, thanks!
Comment #23
lorenzs CreditAttribution: lorenzs at Cegeka commentedGreat effort @dww. Applied the latest patch and still fine.
Comment #24
alexpottCommitted 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.
Comment #27
lauriii+1 for a backport
Comment #28
alexpottComment #30
dwwExcellent, thanks!