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('&amp;');
+        $this->assertNoEscaped('&lt;');
+        // Ensure there are no escaped '<' characters.
+        $this->assertNoEscaped('<');
       }

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sutharsan created an issue. See original summary.

xjm’s picture

Title: Add double escape test » Add double escape test for hook_help()
Issue summary: View changes
xjm’s picture

Priority: Normal » Major
Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
lauriii’s picture

Status: Active » Needs review
FileSize
828 bytes

Maybe something like this

Sutharsan’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks Ok to me. The comment describes what the code does. Unless someone wants to add more test, this is RTBC to me.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

So 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?

dawehner’s picture

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?

I'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

mr.baileys’s picture

Assigned: Unassigned » mr.baileys
mr.baileys’s picture

Assigned: mr.baileys » Unassigned
Status: Needs review » Reviewed & tested by the community

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?

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

I'm sorry but I even think the entire help test should be limited to modules provided by core.

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.

mr.baileys’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
27.39 KB

Actually, just noticed that the assertions are inconsistent, needs work for that:

Simpletest assertions inconsistent

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
832 bytes
1007 bytes

Updated patch.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 05ff65b and pushed to 8.0.x. Thanks!

  • alexpott committed 05ff65b on 8.0.x
    Issue #2568257 by mr.baileys, lauriii, xjm: Add double escape test for...

Status: Fixed » Closed (fixed)

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