Forum - "new replies" message is escaped

:)

Looks great, pretty much only nitpicks :) Hopefully it comes back green!

1. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
   @@ -17,6 +17,20 @@
   +   * User account with limited access to forum pages
   ...
   +   * User account with limited access to forum pages
   

   Nit: Missing trailing periods.

   More important: they have different sets of permissions, but have the same description.

2. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
   @@ -27,14 +41,15 @@ protected function setUp() {
        // Create a test user.
   

   Now 2 test users are being created.

3. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
   @@ -68,4 +83,42 @@ function testForumIndexStatus() {
   +    // Login as the forum user to create a reply
   ...
   +    // Verify the new post link is not raw
   

   Missing trailing period.

4. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
   @@ -68,4 +83,42 @@ function testForumIndexStatus() {
   +    // Mockup a comment
   +    $edit = array();
   +    $edit['comment_body[0][value]'] = $this->randomMachineName();
   +
   +    // Create comment
   

   Just saying "Create comment." once should be sufficient.

5. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
   @@ -68,4 +83,42 @@ function testForumIndexStatus() {
    }
   +
   

   Extraneous newline addition.

Excellent, thanks!

This blocks #2082317: Forum history markers ("new" and "updated" markers, "x new posts" links) forces render caching to be per user, which in turn blocks #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).

+++ b/core/modules/forum/src/Tests/ForumIndexTest.php
@@ -68,4 +82,41 @@ function testForumIndexStatus() {
+    // Login as the forum user to create a reply.

Could be fixed on commit but I think s/Login/Log in would be more accurate (verb instead of noun).

RTBC + 1

+++ b/core/modules/forum/src/Tests/ForumIndexTest.php
@@ -68,4 +82,41 @@ function testForumIndexStatus() {
+  public function testHistory() {

nit:Needs a doc block

(Embedding the screenshot, fixing the patch files order, adding reviewer credit.)

Thanks for the patch and screenshots. I verified in context that the text within this markup is composed of strings properly sanitized earlier in the code flow:

        $variables['topics'][$id]->new_text = '';
        $variables['topics'][$id]->new_url = '';

        if ($topic->new_replies) {
          // ...
          $variables['topics'][$id]->new_text = \Drupal::translation()->formatPlural($topic->new_replies, '1 new post<span class="visually-hidden"> in topic %title</span>', '@count new posts<span class="visually-hidden"> in topic %title</span>', array('%title' => $variables['topics'][$id]->label()));
          $variables['topics'][$id]->new_url = \Drupal::url('entity.node.canonical', ['node' => $topic->id()], ['query' => $query, 'fragment' => 'new']);
        }

      // ...
          $new_replies = '';
          if ($topic->new_replies) {
            $new_replies = '<br /><a href="' . $topic->new_url . '">' . $topic->new_text . '</a>';
          }

          $row[] = array(
            'data' => array('#markup' => $topic->comment_count . $new_replies),
            'class' => array('forum__replies'),
          );

I'm slightly concerned in general that using #markup is proliferating a bit and could be used as a pattern that people apply to keep their stuff from being escaped without thinking about sanitization, especially here where the content is being composed with concatenations stretched out across numerous lines, but that's probably too wide a scope to address here, especially given that this is blocking other work.

However, I found a couple more minor issues with the patch, which I think with #20 and #22 are enough that we should probably clean them up before committing:

1. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
   @@ -68,4 +82,41 @@ function testForumIndexStatus() {
   +    // Verify the new post message link is not escaped.
   

   So, this is very minor, but the assertions aren't actually verifying a message not escaped. They're verifying the correct markup for the message is present, or that said markup is not escaped. (I misunderstood at first and thought that we were deliberately not escaping the text itself, which seemed wrong.)

2. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
   @@ -68,4 +82,41 @@ function testForumIndexStatus() {
   +  public function testHistory() {
   

   Why is this method called testHistory()? It has nothing to do with the history as far as I can see.

So let's clean those things up here. Thanks!

+++ b/core/modules/forum/forum.module
@@ -506,7 +506,7 @@ function template_preprocess_forums(&$variables) {
-            'data' => $topic->comment_count . $new_replies,
+            'data' => array('#markup' => $topic->comment_count . $new_replies),

I agree with xjm here,

Lets use:

SafeMarkup::format('%count!replies', [
  '%count' => $topic->comment_count,
  '!replies' => $new_replies,
]);

Using #markup would soon be unsupported anyway ...

In my RTBC + 1 I mainly looked at the tests ... Should've caught that ...

Ever since effulgentsia fixed SafeMarkup::format() to work like an auto-escape filter, it is a wonderful way to use to fix double-escaping issues in a secure way.

Marked #2636074: forum-list.html.twig replies are double escaped as a dup of this issue. Removed the blocker tag since we managed to release 8.0.0 without this fix... :)

I've also opened #2642450: template_preprocess_forums generates too much HTML to address further cleanup of the forum preprocess functions -- there's a lot of HTML generated in the preprocess functions which makes it harder for custom themes to override.

This patch is from #2636074-4: forum-list.html.twig replies are double escaped and moves the verification into an existing tests which seems cleaner to me. Otherwise, the fix is nearly identical (this patch uses #prefix while #14 concatenates the two strings, see #2636074-7: forum-list.html.twig replies are double escaped for details).

(FYI: credit to @joelpittet for this patch, not me...)

Pushing to major as #2636074: forum-list.html.twig replies are double escaped was major.

Thanks @mikeker:)

Would it be possible to get test only patch for #26? Otherwise this looks RTBC for me!

Tests-only patch from #2636074-4: forum-list.html.twig replies are double escaped.

Tests-only patch from #2636074-4: forum-list.html.twig replies are double escaped.

Wow, double-posted and still managed to forget the file attachment somehow... :)

Back to NR as this was from the tests-only patch...

Thanks @mikeker. Here's the patch again.

Thanks Joel!

I'm not sure that this fix is the best way to fix this BUT fixing forum markup is a much bigger and more complicated issue therefore I'm going to commit this bug fix as an interim fix. Ideally all the markup added in template_preprocess_forums() would be in twig templates.

Committed b422873 and pushed to 8.0.x and 8.1.x. Thanks!

Thank you @alexpott here's a follow-up #2650966: Forum - move markup from template_preprocess_forums() in twig templates

Thanks @joelpittet closing that in favour of the one @mikeker opened in #26, #2642450: template_preprocess_forums generates too much HTML.

Ah I missed that in my quick up-skim. Good catch.