HTML double-escaping in revision messages

This is caused by concatenation with revision log message. Concatenation converts SafeMarkup from t() to string.

$row[] = array('data' => $this->t('!date by !username', array('!date' => ..., '!username' => ...))
            . (($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : ''),
            'class' => array('revision-current'));

$row[] = $this->t('!date by !username', array('!date' => ..., '!username' => ...))
            . (($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : '');

A quickfix for it.

We do now have #2311123: New inline_template render element for HTML code in PHP

Thanks!
Updated patch that uses 'inline_template'.

Sorry missed that we don't need use Drupal\Component\Utility\SafeMarkup; anymore.

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -173,9 +173,17 @@ public function revisionOverview(NodeInterface $node) {
+                'revision_log' => Xss::filter($revision->revision_log->value),

@@ -183,8 +191,16 @@ public function revisionOverview(NodeInterface $node) {
+                'revision_log' => Xss::filter($revision->revision_log->value),

Maybe I am wrong, but wasn't the point of twig autoescape, that you don't have to escape manually? I would love to get a clarification for that. Otherwise this looks fine and actually better to read than before.

chx clarified that XSS checks have to be done.

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -173,9 +173,17 @@ public function revisionOverview(NodeInterface $node) {
+              '#template' => '{{ revision_info }} {% if revision_log %} <p class="revision-log">{{ revision_log }}</p> {% endif %}',
...
+                'revision_info' => $this->t('!date by !username', array('!date' => $this->l($this->dateFormatter->format($revision->revision_timestamp->value, 'short'), 'node.view', array('node' => $node->id())), '!username' => drupal_render($username))),

@@ -183,8 +191,16 @@ public function revisionOverview(NodeInterface $node) {
+              '#template' => '{{ revision_info }} {% if revision_log %} <p class="revision-log">{{ revision_log }}</p> {% endif %}',
...
+                'revision_info' => $this->t('!date by !username', array('!date' => $this->l($this->dateFormatter->format($revision->revision_timestamp->value, 'short'), 'node.revision_show', array('node' => $node->id(), 'node_revision' => $vid)), '!username' => drupal_render($username))),

Let's break this up and put more in the inline template since we will then delay translation to render time which possibly opens up multi-loading in locale cache.

So lets replace !date with ... as well.

@alex meant to say: So lets replace !date with <a href="@url">...</a> as well.

I started looking into this and unfortunately got stuck. If we're to break this up into a more complex template, we need to be able to generate the link that l() is currently building from the routing path 'node.view' and the node->id().

I started to ask @Crell about this, to get some tips on how that link could be built without calling l().

Ultimately he suggested and I would tend to agree, that rather than creating a bunch of inline_templates, this page output should be converted to one twig template.

@jerdavis, which part you think needs to be implemented via template file? The page is already generated as a table and what we need is to make a way to safely process the HTML without any security issues.

Let's discuss on this and create a fix.

Thanks!

Can any one confirm with the latest code checked out and with no patches applied this is fixed or not? I think it's fixed in core.

The issue is not fixed and possible to replicate. #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render may have fixed the issue but with a fresh install of Drupal 8 (8.0.x) brakes those. Changing the status as it needs work.

Hi @LinL,
Just delete the old codebase and database also, then install Drupal via GIT (git clone --branch 8.0.x http://git.drupal.org/project/drupal.git). Once installed create 2 nodes with revisions and then view the revisions.

Let me know if you are able to re-produce this or not.

Thanks!

Guys,
Uploading a patch with implementing a template file and not using inline_template. Not complete though with comments and stuff but worth a review.

Thanks!

Tagging for Amsterdam.
For someone who's a novice to contributing but experienced enough/willing to dive into how Twig+Drupal works, reviewing the patch could also be a novice task. Suggest to decide / try to find someone, among yourselves.

The problem wasn't actually related to Quickedit, it was caused by the way revision log message was added:

$this->t('!date by !username', array('!date' => …, '!username' => …)
. (($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : '')

Concatenation causes the string to become NOT a safe markup.
My proposition is to insert it using t() function replacements, attaching the patch with corresponding changes and some line breaks added.

The patch worked for me.

lokeoke's patch on comment #27 worked for me as well.

Patch worked for me also.

Also checked that nothing else changed, so I'm putting this to "reviewed and tested".

(housekeeping)

I think, if possible, we should be using an inline twig template here.

@alexpott, I do agree with you. Initially this issue had some fixes with inline templates. But then I was trying to introduce one twig template instead of inline template as it was too clumsy in the code and for a better UX. Can you have a look at patch #22 and share your views?
Thanks!

@alexpott I think using inline twig template will be an overkill here. We are using table twig template to render the revisions table. How can a twig template embedded inside another twig template.

Patch using inline twig template.

Have now tested patch #41 on simplytest.me.
1. Without patch - created new test article clicking new revision and then edited content and added new revision. When I click on revision tab I can see the original error.
2. With the patch above applied on drupal 8 build on simplytest.me - doing exact thing does not result in this issue.
This tells me that patch #41 works.

Patch #41 works as expected.

@morad @vurt Thanks for testing. Please change status to RTBC if this is tested and working properly.

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -168,18 +168,51 @@ public function revisionOverview(NodeInterface $node) {
   -            'class' => array('revision-current'));
   -          $row[] = array('data' => String::placeholder($this->t('current revision')), 'class' => array('revision-current'));
   +          $row[] = array(
   +            'data' => array(
   ...
   +          $row[] = array(
   +            'data' => array(
   

   The 'current-revission' class got lost in translation on both rows.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -168,18 +168,51 @@ public function revisionOverview(NodeInterface $node) {
   +              '#type' => 'inline_template',
   +              '#template' => '<em class="revision-current">{{ current_revision_operation }}</em>',
   +              '#context' => array(
   +                'current_revision_operation' => $this->t('current revision'),
   

   We may need to make sure String::placeholder, passes along the save string if it isn't but the conversion should not be needed regardless.

3. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -168,18 +168,51 @@ public function revisionOverview(NodeInterface $node) {
   +              '#type' => 'inline_template',
   +              '#template' => '<div>{{ old_revision_message }}</div>',
   +              '#context' => array(
   

   Is this div needed, it wasn't there before. And if not can we remove the inline_template?

Thanks @joelpittet for the nice review! Patch corrected.

Before @joelpittet's review:

After @joelpittet's review:

Much closer, thanks for the fixes.

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -182,18 +182,53 @@ public function revisionOverview(NodeInterface $node) {
+              '#type' => 'inline_template',
+              '#template' => '{{ current_revision_message }}',
...
+              '#type' => 'inline_template',
+              '#template' => '{{ current_revision_operation }}',
...
+              '#type' => 'inline_template',
+              '#template' => '{{ old_revision_message }}',

We should really avoid inline_template, especially when the results is one variable being printed.

It's a lot of overhead, complexity for little gain.

All of these because we are pumping them into $this->t() now, are marked as safe, they can be added to 'data' of the table cell as is and shouldn't escape.

Agree with @49 i don't find a difference between before applying patch and after a patch .

I agree with @joelpittet. I have also said this in #39. If that is the case then we can use the patch in #27 without any problem.

*bump* anybody please do a review

Both the patch in #27 and the patch in #48 did not fix the escaping, did I miss something?

Url: /node/2/revisions

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -182,18 +182,53 @@ public function revisionOverview(NodeInterface $node) {
+                  array(
...
+                    '!revision-log' => ($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : '',

#2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument This issue makes all these ! passthroughs not work if the values passed in to the variable isn't marked as safe. I don't agree with that issue for DX reasons... but that's why.

renamed the tag by mistake..

@joelpittet SafeMarkup::set() is not desirable here. How about check string using SafeMarkup::isSafe() before rendering?

See @alexpott comment here.

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -182,18 +182,37 @@ public function revisionOverview(NodeInterface $node) {
+            '!date' => $node->link($this->dateFormatter->format($revision->revision_timestamp->value, 'short')),
+            '!username' => drupal_render($username),
+            '!revision-log' => ($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : '',
...
+          $old_revision_message = $this->t('!date by !username !revision-log', array(
+            '!date' => $this->l($this->dateFormatter->format($revision->revision_timestamp->value, 'short'), new Url('node.revision_show', array('node' => $node->id(), 'node_revision' => $vid))),
+            '!username' => drupal_render($username),
+            '!revision-log' => ($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : '',
+          ));

This already ensures that the HTML is safe, so I guess #markup will work in this case.

Needs tests

Haha, what a cheater, but yes that should work:)

Just a thought, @subhojit777 can we replace drupal_render() with \Drupal::service('renderer')->render() call? drupal_render() is now deprecated anyway.

Thanks!

;) @joelpittet I hope it passes RTBC

Thanks @aneek for the review. Patch changed as per #63.

@subhojit777 - missing file upload on comment #69? If you have the patch still, please upload :)

Sorry #69 is a mistake. I am working on the tests right now. Will upload a complete patch soon. The patch will have the change as suggested in #63.

I am adding tests in NodeRevisionsUiTest.php ( Ithought this would be the best place for adding tests). The class extends NodeTestBase. Any idea how can I debug tests there as $this->debug() is not working.

Please check for the logic used in tests, and check whether the tests have been written in right place.

- Test logic taken from #2394951: Page title escaped with HTML markup when editing content translation
- Code snippets referred from NodeRevisionsTest.php

As expected test is failing for patch including only tests.

Looks fine to me as-is. There's an argument to be made for the test to assert against a static string rather than relying on the output of t() (which could break without breaking this test), but not sure of the feelings around that in the Drupal community especially for something so widely used.

alexpott asked a couple of times for an inline Twig template here. #39 explained the rationale for not doing that, but re-assigning to Alex to see if this addresses his concerns.

Also, does this recent change affect this patch at all? https://www.drupal.org/node/2445441

@LinL This seems to be working and does not needs a reroll.

Also, does this recent change affect this patch at all? https://www.drupal.org/node/2445441

@Cottser Since it is working and the messages are not escaped that means the messages with !-prefix have already been marked as safe. Does this make sense?

@subhojit777 - it seems the patch fails to apply.I shall test it manually and post feedback here.

This one should work!

I'm test / reviewing this one as apart of Midcamp.

Without this patch, the verbose markup is stringified. With the patch the markup is back to normal. RTBC.

Without patch:

With patch:

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -182,18 +182,37 @@ public function revisionOverview(NodeInterface $node) {
   -          $row[] = array('data' => $this->t('!date by !username', array('!date' => $node->link($this->dateFormatter->format($revision->revision_timestamp->value, 'short')), '!username' => drupal_render($username)))
   -            . (($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : ''),
   -            'class' => array('revision-current'));
   -          $row[] = array('data' => String::placeholder($this->t('current revision')), 'class' => array('revision-current'));
   +          $current_revision_message = $this->t('!date by !username !revision-log', array(
   +            '!date' => $node->link($this->dateFormatter->format($revision->revision_timestamp->value, 'short')),
   +            '!username' => \Drupal::service('renderer')->render($username),
   +            '!revision-log' => ($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : '',
   +          ));
   +          $row[] = array(
   +            'data' => array(
   +              '#markup' => $current_revision_message,
   +            ),
   +            'class' => array('revision-current'),
   +          );
   ...
   -          $row[] = $this->t('!date by !username', array('!date' => $this->l($this->dateFormatter->format($revision->revision_timestamp->value, 'short'), new Url('node.revision_show', array('node' => $node->id(), 'node_revision' => $vid))), '!username' => drupal_render($username)))
   -            . (($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : '');
   +          $old_revision_message = $this->t('!date by !username !revision-log', array(
   +            '!date' => $this->l($this->dateFormatter->format($revision->revision_timestamp->value, 'short'), new Url('node.revision_show', array('node' => $node->id(), 'node_revision' => $vid))),
   +            '!username' => \Drupal::service('renderer')->render($username),
   +            '!revision-log' => ($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : '',
   +          ));
   +          $row[] = array(
   +            'data' => array(
   +              '#markup' => $old_revision_message,
   +            ),
   +          );
   

   Using #markup here looks very problematic. I still think that using an inline template is the way to go here and #39 does not give a good reason for not doing it.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -182,18 +182,37 @@ public function revisionOverview(NodeInterface $node) {
   +            'data' => String::placeholder($this->t('current revision')),
   

   This has recently been deprecated - use SafeMarkup::placeholder instead.

Re-rolled the patch and applied changes suggested by #88. I couldn't provide interdiff because it was re-rolled, sorry.

Forgot one comma...

I agree with #2309215-50: HTML double-escaping in revision messages that just wrapping things in an inline template is not a proper fix at all IMHO. It adds quite some overhead we should try to avoid, if possible.

Hi all,
it seems to be fixed in the last 8.0.x branch, cause the String::placeholder has been replaced by SafeMarkup::placeholder
(see the deprecated annotation : https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Utility!Stri...)

I didn't reproduce the issue.

I've rolled the patch I made for this bug in #2470998: Revision information displayed with HTML tags with the tests added from #91.

To answer #94 this is still a problem in 8.0.x

I fixed the test case and make it so that it actually tests the bug (we need a revision message for the bug to appear).

Based on feedback from @alexpott in #2470998: Revision information displayed with HTML tags that I closed as a duplicate:

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -182,8 +182,8 @@ public function revisionOverview(NodeInterface $node) {
+          $row[] = array('data' => SafeMarkup::set($this->t('!date by !username', array('!date' => $node->link($this->dateFormatter->format($revision->revision_timestamp->value, 'short')), '!username' => drupal_render($username)))
+            . (($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : '')),

@@ -192,8 +192,8 @@ public function revisionOverview(NodeInterface $node) {
+          $row[] = SafeMarkup::set($this->t('!date by !username', array('!date' => $this->l($this->dateFormatter->format($revision->revision_timestamp->value, 'short'), new Url('node.revision_show', array('node' => $node->id(), 'node_revision' => $vid))), '!username' => drupal_render($username)))
+            . (($revision->revision_log->value != '') ? '<p class="revision-log">' . Xss::filter($revision->revision_log->value) . '</p>' : ''));

We should not be using SafeMarkup::set() here. I'm not sure we need to use the !. We desperately need to update https://www.drupal.org/node/2296163 :( - #2273923: Remove html => TRUE option from l() and link generator. contains some of the latest thinking on safe markup issues. But tldr; #2273923-192: Remove html => TRUE option from l() and link generator.

I've rethought this. I think String::format() is a perfectly adequate API for combining literals and variables into a safe HTML string.

I talked with Alexpott about this in IRC.

We want to limit the use if SafeMarkup::set as much as possible, inline templates are ok for more than one variable.

So I remade the patch with that in mind.

Rerolled the patch.

The only change was the route name for the node revision (node.revision_show -> entity.node.revision)

I am removing the Novice tag from this issue because All novice tasks in issue summary are marked completed.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

@devingd great! I'm not at con, but you can catch me in IRC if you have any questions.

I have tested the patch and it solves the issue I saw with double-escaped Revision comments

Good work on the patch and I'm happy to see something that is working here! We still need to fix some code style things on the patch, otherwise this start looking good!

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -182,18 +182,47 @@ public function revisionOverview(NodeInterface $node) {
   +          $row[] = array(
   

   This could be made a lot cleaner by using the new array syntax.

2. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -17,26 +20,44 @@
   +   * Web user.
   +   */
   +  protected $webUser1;
   ...
   +  /**
   +   * Web user.
   +   */
   +  protected $webUser2;
   

   Still missing the type

3. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -73,6 +94,53 @@ function testNodeFormSaveWithoutRevision() {
   +
   +
   

   Double line change

I addressed the issues from #112.

I think we are getting closer on this one! Thanks again for working on this! :) Here's some more comments for this. Sorry for not pointing these on my first review.

1. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -17,26 +20,44 @@
   +      array(
   

   Short array syntax ftw!

2. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -17,26 +20,44 @@
   +
   

   Why extra line change?

3. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -73,6 +94,52 @@ function testNodeFormSaveWithoutRevision() {
   +    $node->body = array(
   

   We could use the short array syntax here because its awesome!

4. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -73,6 +94,52 @@ function testNodeFormSaveWithoutRevision() {
   +    $node = Node::load($node->id()); // Make sure we get revision information.
   

   The comment could be on its new line.

5. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -73,6 +94,52 @@ function testNodeFormSaveWithoutRevision() {
   +    $old_revision_message = t('!date by !username', array(
   

   $this->t

6. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -73,6 +94,52 @@ function testNodeFormSaveWithoutRevision() {
   +      '!date' => \Drupal::l(format_date($nodes[0]->revision_timestamp->value, 'short'), new Url('entity.node.revision', array('node' => $nodes[0]->id(), 'node_revision' => $nodes[0]->getRevisionId()))),
   +      '!username' => $web_user,
   +    )) . (($nodes[0]->revision_log->value != '') ? '<p class="revision-log">' . $nodes[0]->revision_log->value . '</p>' : '');
   +    $this->assertRaw(SafeMarkup::set($old_revision_message));
   ...
   +    $current_revision_message = t('!date by !username', array(
   +      '!date' => $nodes[1]->link(format_date($nodes[1]->revision_timestamp->value, 'short')),
   +      '!username' => $web_user,
   +    )) . (($nodes[1]->revision_log->value != '') ? '<p class="revision-log">' . $nodes[1]->revision_log->value . '</p>' : '');
   +    $this->assertRaw(SafeMarkup::set($current_revision_message));
   

   Is there any way this could be made simpler? This is _very_ hard to read.

More fixes as per review.

I tried to improve readability of 6. some by creating some variables etc.

Fixed the test, also $this->t doesn't seem possible in the test.

I tested #120 and it worked as expected.

I manually tested this and it resolves the issue. I think #120 has addressed the remaining feedback from #114. I've added a beta phase evaluation to the issue summary.

Sorry for nitpicking this:

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -182,18 +182,41 @@ public function revisionOverview(NodeInterface $node) {
   +            'class' => array('revision-current'),
   

   old array syntax in new array is not nice.. It will break some editors auto tabbing.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -182,18 +182,41 @@ public function revisionOverview(NodeInterface $node) {
   +          $row[] = array(
   ...
   +            'class' => array('revision-current'),
   

   Lets change these one for the new array syntax for consistency.

3. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -7,7 +7,10 @@
   +use Drupal\node\Entity\Node;
   

   This should be before NodeType

@laurii I fixed new array syntax the placed you should, some places you missed and in some surrounding for consistansy and since I was changing stuff anyways.

Also fixed the order of use statements in test.

So all should be good now.

Forgot status

RTBC in #123 with manual test.
Nitpicks fixed in #125

I've reviewed the code once more and don't want to block on any further short array syntax nits. This looks very well done.

Thank you all!

+++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
@@ -73,6 +89,60 @@ function testNodeFormSaveWithoutRevision() {
+    $this->assertRaw(SafeMarkup::set($old_revision_message));
...
+    $this->assertRaw(SafeMarkup::set($current_revision_message));

Sorry I should have spotted this before but I can't see why we need to call SafeMarkup::set() here.

@alexpott you're right, SafeMarkup::set was not needed, fixed and hopefully we are all good to go now.

Oh nice spot. The tests really don't need that, back to RTBC. Less SafeMarkup::Set()!

We can make this significantly simpler to maintain by doing something like this.

This looks good, we like simple solutions. RTBC imo.

The fails were due to revisions not properly linking to the revision, but to the current node.

After applying patch revisions are no longer double escaped.

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -173,28 +173,40 @@ public function revisionOverview(NodeInterface $node) {
+        $link = $vid != $node->getRevisionId()
+          ? $this->l($date, new Url('entity.node.revision', ['node' => $node->id(), 'node_revision' => $vid]))
+          : $node->link($date);

Hate to bring it up but what's the deal with this? I guess it's not in our coding standards either way. I do see about 3 others like this in core, and a bunch in vendor.

I went with a ternary operator since it seemed simpler than an if/else. I went to multiple lines since it was unwieldy as a one-liner.

Our coding standards don't mention ternary operators at all. The closest mention I could find was https://www.drupal.org/node/2353219.

Thanks @laurii looks good.

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -172,53 +172,68 @@ public function revisionOverview(NodeInterface $node) {
+      if (!$revision = $node_storage->loadRevision($vid)) {

I realise this check is already there, but when would this ever be not be loaded?

I realise this check is already there, but when would this ever be not be loaded?

Looking at NodeStorage::revisionIds(), I think we can safely remove the conditional here, and just change it to

$revision = $node_storage->loadRevision($vid);

Removed the condition, fixed the missing block on an if statement, made the array() to [] consistent in the blocks we were working on and new test and brought a 2 space fix back to 1.

Assuming #150 goes green, I think this is back to RTBC.

Just one more test: I applied the patch and can confirm that it works as expected.

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -165,60 +165,73 @@ public function revisionOverview(NodeInterface $node) {
   +      if ($vid == $node->getRevisionId()) {
   +        $row[0]['class'] = ['revision-current'];
   +        $row[] = [
   +          'data' => SafeMarkup::placeholder($this->t('current revision')),
   +          'class' => ['revision-current'],
   +        ];
   

   why set row 0 class to revision-current .. and then set it again (with the data).

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -165,60 +165,73 @@ public function revisionOverview(NodeInterface $node) {
   +      // Link to old version if not viewing current revision.
   

   would it be more appropriate to say "processing current revision" instead of viewing current revision? I dont think at this point a user is "viewing" a particular revision. we are in a loop going through each revision.

3. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -7,6 +7,9 @@
   +use Drupal\Component\Utility\SafeMarkup;
   

   is this SafeMarkup use really needed in the test?

4. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -17,26 +20,39 @@
      /**
   +   * @var \Drupal\user\Entity\User
   +   */
   +  protected $webUser1;
   ...
   +  /**
   +   * @var \Drupal\user\Entity\User
   +   */
   +  protected $webUser2;
   

   why did we need 2 users? it doesn't look like the 2 are interacting.

   or did the new test, just need a user with more permissions (to view revisions and profiles).

   they could probably be named better to indicate their purpose if it is not just that they are two different users.

   maybe we could use the same user, and add a role with the additional permissions. or just make the user we want in the test and not in the setup.

5. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -73,6 +89,60 @@ function testNodeFormSaveWithoutRevision() {
   +  function testNodeRevisionDoubleEscapeFix() {
   

   we should have the public specifier for visibility on all functions. (or protected, but here it is public)

   https://www.drupal.org/node/608152#visibility

   (I know many existing test methods do not, but we are adding a new function here, so we can do it correctly. No need to fix the other function, that would be scope creep.)

6. speaking of scope creep,

   +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -165,60 +165,73 @@ public function revisionOverview(NodeInterface $node) {
   -    $delete_permission =  (($account->hasPermission("delete $type revisions") || $account->hasPermission('delete all revisions') || $account->hasPermission('administer nodes')) && $node->access('delete'));
   +    $delete_permission = (($account->hasPermission("delete $type revisions") || $account->hasPermission('delete all revisions') || $account->hasPermission('administer nodes')) && $node->access('delete'));
   

   is not needed to be changed in this issue.

7. also, I think we can should get a tests-only patch to verify the test fails in the correct way we expect it to.

#153

Answering a few of the reviews

1. You are misreading the code. We set the class for row 0 and then add a new row which also has that class.
6. #114 asked for new array syntax, I don't care either way can remove if it's a blocker but would vote for consistancy (which means we should keep it) - ah it's not array syntax but a code style cleanup (double space). Seems a bit crazy that we have to make a separate issue for that but oh well, I guess that's just how it has to be.

6. is not about array syntax, it is just replacing a double space on a nearby line

@googletorp
1. Ohhhh, each element in $row is a column. and there are two columns per row, revision and operations.

So I made some fixes

2. Reworded comment, to make more sense
3. Deleted this as it was not used anymore
4. Created a single used with the required permissions, since the extra permissions are fine for the other test.
5. Added missing function
6. Reverted the change (scope creep)
7. Uploaded a test only patch.

1 should be fine.

So we are all set to go :)

Note the TEST-ONLY patch should fail tests.

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -165,60 +165,73 @@ public function revisionOverview(NodeInterface $node) {
-    $delete_permission =  (($account->hasPermission("delete $type revisions") || $account->hasPermission('delete all revisions') || $account->hasPermission('administer nodes')) && $node->access('delete'));
+    $delete_permission =  (($account->hasPermission("delete $type revisions") || $account->hasPermission('delete all revisions') || $account->hasPermission('administer nodes')) && $node->access('delete'))

oops. missing semi-colon.

Thanks, fixed.

The test only patch should still be good.

Note to self, remember to run tests.

A new batch of patches, based from #150

I think the additional feedback has been addressed in #161.

wow. thanks for the super fast responses!

----
I checked the fails, and there was only one, but I kind of expected them both to fail.

As noted by @LinL in #24, one of the asserts passes because it did not have a revision log (so it didn't get concatenated, so it didn't trigger the double escape).

----

but something else was bothering me about the test...

1. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -73,6 +79,60 @@ function testNodeFormSaveWithoutRevision() {
   +    $node->revision_log->value = 'Revision <em>message</em> with markup.';
   ...
   +    $current_revision_message .= $nodes[1]->revision_log->value == '' ? '' : '<p class="revision-log">' . $nodes[1]->revision_log->value . '</p>';
   

   We have the text of the revision log in the test. I think the assert should use that text, not load the log from the node and assert that the log message was whatever was in the node.

2. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
   @@ -73,6 +79,60 @@ function testNodeFormSaveWithoutRevision() {
   +    $old_revision_message .= $nodes[0]->revision_log->value == '' ? '' : '<p class="revision-log">' . $nodes[0]->revision_log->value . '</p>';
   +    $this->assertRaw($old_revision_message);
   

   this assert does not fail in the test only patch, because it is checking the original entry from when the node was created. which does not have a revision log, so the concatenation was not causing a double escape.

   ....
   which is fine, but I think we dont have to have a conditional getting a log message that we know should not be there.

Worked with @YesCT and addressed her concerns. Ran tests locally and confirmed this passes and fails appropriately.

Yeah, that feels more accurate for the tests.

Re-read the whole patch. RTBC.

Ok this has definitely sat at RTBC for long enough that if someone had concerns they could've raised them. :) I'm also tired of this coming up during demos. :D

Committed and pushed to 8.0.x. Thanks!