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.
Follow-up to #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests
Problem/Motivation
- Test to ensure there is no double-escaped text in
hook_help()
.
Proposed resolution
Code from previous issue:
+++ b/core/modules/help/src/Tests/HelpTest.php
@@ -113,6 +113,11 @@ protected function verifyHelp($response = 200) {
foreach ($admin_tasks as $task) {
$this->assertLink($task['title']);
}
+ // Ensure there are no double escaped '&' or '<' characters.
+ $this->assertNoEscaped('&');
+ $this->assertNoEscaped('<');
+ // Ensure there are no escaped '<' characters.
+ $this->assertNoEscaped('<');
}
Beta phase evaluation
Issue priority | Major because of its relationship to SafeMarkup criticals |
---|---|
Unfrozen changes | Unfrozen because it only changes tests. (Which? Specify.) |
Remaining tasks
TBD
User interface changes
None
API changes
Probably none.
Comment | File | Size | Author |
---|---|---|---|
#13 | interdiff.txt | 1007 bytes | mr.baileys |
#13 | 2568257-13-hook-help-double-escape-test.patch | 832 bytes | mr.baileys |
#12 | Screen Shot 2015-09-19 at 12.36.26.png | 27.39 KB | mr.baileys |
#6 | add_double_escape_test-2568257-6.patch | 828 bytes | lauriii |
Comments
Comment #2
xjmComment #3
xjmComment #4
xjmComment #5
xjmComment #6
lauriiiMaybe something like this
Comment #7
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedThe patch looks Ok to me. The comment describes what the code does. Unless someone wants to add more test, this is RTBC to me.
Comment #8
xjmSo I think the test is a great idea, except for one concern.
If some module does have a usecase to display double-escaped HTML (for example, if it were to display a sample of the characters of an HTML entity), this test will start failing. We could special-case modules in core if we broke this test in the future, but what about contrib/custom modules on a site where this test was run?
It looks like the test is intended to test the modules in Standard, but I didn't actually confirm it wasn't testing all modules (including disabled ones).
Can someone test creating a module that does deliberately have a double-escaped HTML entity in its
hook_help()
--one not part of Standard--and test what would happen with this test?Comment #9
dawehnerI'm sorry but I even think the entire help test should be limited to modules provided by core. Its just so not reliable if core tests also deal with contrib/custom module code. Its such a bummer everytime if some of the non updated contrib modules cause test failures in things like InstallUninstall test ... it is not the responsibility of core to find bugs in contrib modules, IMHO
Comment #10
mr.baileysComment #11
mr.baileysI created a module that does just that, and it had no impact on the test (still succeeded). Only after explicitly adding the module to the Standard profile, the tests started failing.
Isn't this what is already happening by
protected $profile = 'standard';
in \Drupal\help\Tests\HelpTest ? Only core modules mentioned as dependencies for the Standard profile are tested, all others (non-Standard core and contrib) are ignored.Given that you need a) a module that outputs double-escaped code in hook_help and b) explicitly add that module to the standard profile to cause test failures, it seems that me that this patch can go in as-is? Tentatively setting back to RTBC.
Comment #12
mr.baileysActually, just noticed that the assertions are inconsistent, needs work for that:
Comment #13
mr.baileysUpdated patch.
Comment #14
xjm@dawehner, agreed, but looks like it's not actually a problem for this test in any case per @mr.baileys' testing -- this one is indeed only getting modules from Standard.
@mr.baileys, thanks for testing!
I had to think about the updated assertion message texts for a minute, but they look correct.
Comment #15
alexpottCommitted 05ff65b and pushed to 8.0.x. Thanks!