Theme name in block admin shows HTML entities: excessive check_plain()

Where is the second check_plain() applied? I tried and reproduced this with Garland, so it is in itself a core issue. Note that at other places, we also do check_plain, but we assign "check_plain" as the title callback instead (eg. with node type names). Are those affected as well?

This is an old issue, but I'm still seeing the same effect in Drupal 6.6. An apostrophe/single quote ' in a theme name is displayed as ' and a double quote " becomes "

This only affects /admin/build/block, I think. On all other admin pages, the theme name is displayed correctly

This issue appears in all themes that I've tried, including Chameleon and Garland. The theme with the problem name is a subtheme based on Zen. The .info file for the theme is saved as UTF-8 by the editor.

I tried a few other characters (like `~, and even some Chinese), but only ' and " caused problems - maybe it's something to do with 'smart' quotes?

This is only a cosmetic problem in the blocks admin ui. it doesn't seem to have any impact on the site, so I haven't tried the patch.

(I have the i18n module installed, in case that has some relevance to this).

edit: html was parsing character entities

Here's a patch for 8.x. This will probably need to have tests added.

This was my test proceedure:

Copy Bartik to sites/default/themes.

Change the theme name to something with an HTML escaped character, including renaming the directory to a machine name (cats_dogs), renaming the .info file (cats_dogs.info), and the name property in the .info file (Cats & Dogs).

Enable the theme. It's not necessary to set it as the default.

Check the links in the block admin page.

Apply the patch.

Check that the links are now correct.

#7: block-links-214760-7.patch queued for re-testing.

Bug as described exactly in #7 still exists, patch works as advertised.

The last thing this patch needs is a test, and then it's ready to go in! Test that when a theme uses check_plain() unsafe characters in its name, the links on the block admin page (admin/structure/block/list/foo_bar) did not get mangled. And remember to clear your drupal cache after changing the theme's name before you try to enable it.

What are check_plain() unsafe characters? It's really just a call to htmlspecialchars(), which converts:

• '&' (ampersand) becomes '&'
  
• '"' (double quote) becomes '"' since ENT_NOQUOTES is not set.
  
• "'" (single quote) becomes ''' (or ') since ENT_QUOTES is set.
  
• '<' (less than) becomes '&lt;'
  
• '>' (greater than) becomes '&gt;'

Setting to needs work for a test, but otherwise this is RTBC.

I'll write the Test for this.

In order to write a test for this fix, the theme has to be set to visible (in order for it to show up on the in the links on the block admin page.) Usually, test themes are hidden. Will it be a problem if I have a theme that is not hidden? (I think it doesn't matter for D8 because I don't see the theme showing up outside of testing.)

Also, do we really need a test case for this?

I will try creating a test module which implements system_info_alter then to make it not hidden for that test alone.

#7: block-links-214760-7.patch queued for re-testing.

Reroll. Test Case coming soon.

run simpletest.

working on the testcase.

Added a Test.

remove single quote test

run simpletest

Just testing to see if this test works on the simpletest server.

New theme was in the wrong directory. This patch is just to test that simpleTest finds the theme.

File wasn't attached.

Here is a candidate for review.

Regarding the test:
1. I threw all the special characters into one test. Does that seem reasonable or should they be broken out?
2. Created a new hidden theme. Is that acceptable?
3. I would have preferred to use assertLink(), however assertLink() uses xpath and wasn't able to escape single quotes.

I tested and the issue still exists, patch needs re-roll, looking over the code it looks good apart from below.

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
@@ -145,6 +145,26 @@ function testBlock() {
+    theme_enable(array($theme,'seven','block_test_theme'));

Spaces after the commas?

Rerolled and added spaces as suggested in #31.

module_enable was removed.

Remove "Needs reroll" because new reroll is passing.

Remove "Needs tests" because patch includes tests.

1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -145,6 +145,26 @@ function testBlock() {
   +    module_enable(array('block_test'));
   

   module_enable has been removed.

2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -145,6 +145,26 @@ function testBlock() {
   +    config('system.theme')
   

   Use \Drupal::config()

Yes, the patch actually needed a reroll as of 10/24 where the routing was moved to a .yml file: Issue #2102125: Big Local Task Conversion.

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
@@ -169,6 +169,26 @@ public function testBlockThemeSelector() {
+   * Test block display of theme titles.
+   */
+  function testThemeName() {
+     \Drupal::moduleHandler()->install(array('block_test'));
+     // Explicitly set the default and admin themes.
+     $theme = 'cat_mouse';
+     theme_enable(array($theme, 'seven', 'block_test_theme'));
+     $this->resetAll();
+
+     \Drupal::config('system.theme')
+       ->set('default', 'seven')
+       ->set('admin', 'seven')
+       ->save();
+     menu_router_rebuild();
+     $this->drupalGet('admin/appearance');
+     $this->drupalGet('admin/structure/block');
+     $this->assertText('<"Cat" & 'Mouse'>');
+  }
+
+  /**

This is all indented incorrectly.

Also the issue is now fixed in core, however this does add a valid test for the previous problem so it's still worth adding IMO.

-    $output .= '<p>' . l(t('Demonstrate block regions (@theme)', array('@theme' => $themes[$demo_theme]->info['name'])), 'admin/structure/block/demo/' . $demo_theme) . '</p>';
+    $output .= '<p>' . l(t('Demonstrate block regions (!theme)', array('!theme' => $themes[$demo_theme]->info['name'])), 'admin/structure/block/demo/' . $demo_theme) . '</p>';

This feels like the fix is in the wrong place. If info['name'] is plaintext, why are we converting it to HTML too early?

@benjy - Part of it is fixed in Core. But, the Theme Name that comes after "Demonstrate block regions" is still double escaped. For example: Demonstrate block regions (Cats & Dogs). So, probably an assertLink test should be added on admin/structure/block/list/cats_dogs.

@Damien Tournoud - I'm not not sure how else to fix it. But that fix works. Not sure what you mean.

So, the patch still fixes this issue: admin/structure/block/list/cats_dogs

Before
After:

Fixed the whitespace issues from comment #41.

Couldn't add the assertLink that I mentioned in #44, because the "Demonstrate block regions" text doesn't show up in Simpletest. I only noticed the unescaped text shown in #44 by moving the cat_mouse test theme to /themes (to a Drupal branch without this patch, of course) making it unhidden and going to it's block page.

Just as a heads up for backporting to Drupal 7 once this is set for D8 (I assume that's the plan).

In D7, it's still necessary to handle the local task that was in #34, but removed for #39

-      'title' => check_plain($theme->info['name']),
+      'title' => $theme->info['name'],

Thanks, should I go ahead and start on the D7 patch or wait for this to be reviewed? or is anyone else interested in doing the backport?

48: 214760-48.patch queued for re-testing.

Needs a Reroll.

Re-rolled.

removed a call to menu_router_rebuild().

Probably better to keep the menu rebuild. In runtime code, rebuild() would probably not be called directly but in a simpletest you need it done immediately.

57: 214760-57.patch queued for re-testing.

I don't get that error locally. Retesting.

Updating with reroll. Please review.

Reviewed patch (looks good) and ran the test with success!!

Agree with earlier comments by benjy that even though this problem has been fixed in core, it would be great to have a test for it included as well.

Nice catch! And a test to boot. :)

Committed and pushed to 8.x. Thanks!

Moving down to 7.x.

Hm. I first thought it would be a minor oversight only (hook_system_theme_info() being gone), but after further review, I think we need to revert this and ensure that we get the tests right:

1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -182,6 +182,26 @@ public function testBlockThemeSelector() {
   +    \Drupal::moduleHandler()->install(array('block_test'));
   
   +++ b/core/modules/block/tests/modules/block_test/block_test.module
   @@ -8,6 +8,15 @@
    /**
   + * Implements hook_system_theme_info().
   + */
   +function block_test_system_theme_info() {
   

   hook_system_theme_info() no longer exists.

   No replacement required.

   Due to that, the test also does not have to install block_test module anymore.

2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -182,6 +182,26 @@ public function testBlockThemeSelector() {
   +    $theme = 'cat_mouse';
   

   Can we rename at least the internal theme to properly identify and describe what its use-case is? E.g.:

   block_test_specialchars_theme

3. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -182,6 +182,26 @@ public function testBlockThemeSelector() {
   +    theme_enable(array($theme, 'seven', 'block_test_theme'));
   

   It's not clear to me why block_test_theme is enabled in this test?

4. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -182,6 +182,26 @@ public function testBlockThemeSelector() {
   +    \Drupal::config('system.theme')
   +      ->set('default', 'seven')
   +      ->set('admin', 'seven')
   +      ->save();
   

   Why is the default theme and admin theme changed to Seven in the test?

5. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -182,6 +182,26 @@ public function testBlockThemeSelector() {
   +    $this->drupalGet('admin/appearance');
   

   The Appearance page does not belong to Block module (and nothing is asserted), so this request is unnecessary.

6. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -182,6 +182,26 @@ public function testBlockThemeSelector() {
   +    $this->drupalGet('admin/structure/block');
   +    $this->assertText('<"Cat" & 'Mouse'>');
   

   The admin/structure/block page actually shows the theme name multiple times, and this test does not assert all instances.

   In fact, since the test explicitly changes the default theme to Seven instead of the new test theme, this page only shows the theme name in the tabs, but the "Demonstrate block regions" link does not appear for the test theme.

7. +++ b/core/modules/block/tests/modules/block_test/themes/cat_mouse/cat_mouse.info.yml
   @@ -0,0 +1,16 @@
   +regions:
   ...
   +regions_hidden:
   

   These regions do not seem to be used?

sun: I agree with all points. It might be easier to just patch 8.x? Thanks, the point #6 in that review helped a lot. Led me to figure out why "Demonstrate Regions" did not show up in Simpletest. I needed to add the system_help block.

I think the theme name shows up in 2 places. I added an assertLink to test the one in "Demonstrate Regions".

Thanks!

The patch doesn't fully address #67 yet:

#67.1: the test also does not have to install block_test module anymore.

#67.3: instead of seven + block_test_theme, the test now enables bartik, which is equally unnecessary.

#67.4: The system.theme configuration doesn't have to be changed for this test.

#67.5: /admin/appearance is still requested.

#67.6: The test enables the Help block now (which contains the faultily escaped link), but it still doesn't ensure that the assertion is not a false-positive. The theme name appears more than once on the block admin pages (in tabs). The test needs to request the block admin (tab) page of the test theme, so that the "Demonstrate block regions" link actually appears on the page.

#67.7: The regions_hidden definition is unnecessary.

OK, some of those missing items were due to a patch creation mistake. So, I'm doing an assertText() on the block page to handle the text in the tab. Then, on the block/list/[theme] page, I'm doing an assertLink() to handle the "Demonstrate Regions" text.

Thanks, that looks much better now :-)

1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -185,20 +185,17 @@ public function testBlockThemeSelector() {
        $this->resetAll();
   

   Given that the router is manually rebuilt one line below, is the resetAll() actually needed?

2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -185,20 +185,17 @@ public function testBlockThemeSelector() {
        $this->assertText('<"Cat" & 'Mouse'>');
   ...
   +    $this->assertLink('Demonstrate block regions (<"Cat" & \'Mouse\'>)');
   

   Both assertText() and assertLink() are parsing the raw HTML page content on their own, and the string that is passed to the methods is escaped for HTML.

   In both cases, we should assert the raw page output, like this:

   $this->assertRaw(String::checkPlain('<"Cat" & \'Mouse\'>'));
   

   (same for the link; it is sufficient to assert the link text only)

   This asserts that we find the exact raw string in the raw page output, without any additional layers of HTML parsing/escaping of the testing framework.

sun: I agree that resetAll() is not needed anymore. I also followed your suggestion on the assertRaw() because it minimizes the amount of parsing that is needed. I also tested reversing the "Fix" to verify that the test still fails without the fix.

Thanks! — Looks good, but the patch contains some file chmod/permission changes:

diff --git a/sites/README.txt b/sites/README.txt
old mode 100644
new mode 100755
diff --git a/sites/example.sites.php b/sites/example.sites.php
old mode 100644
new mode 100755

Fixing that mistake by simply removing the hunks from the patch.

RTBC if testbot fails on the test-only patch only.

Whoops, thanks for that. Committing at sprints is dangerous. ;)

Committed and pushed to 8.x. Thanks!