Views preview title is double escaped

@rroose thanks for the report.

Views is in Drupal core now, moving this to the right queue.

This is only happening on the preview of the view. I followed these steps:

- Created a view to show articles content
- Add a contextual filter Content: ID and override Title {{arguments.nid}}.
- Then I testest the preview and It was ok, except the first title (you can see it on the image attached).
- I also created a view page from the master view to test the page and it working fine.

Please let me know if I am missing something.

Thank you.

Just tested this and it has nothing to do with the token, if you just set the title of the View to 'double & escaped' it also gets double escaped.

So yeah this is definitely bugged, but setting to minor as it is only the preview.

Hi again,

I'll attach a patch for this issue.

rroose, when you say that happend to you on the frontend, I guess you mean the title of the view not the title of the node. Could you attach a file?, because I can only reproduce on the preview and the frontend (view page) but only with the view title.

Thank you.

Some simple test would be great I'd argue.

I already test it @dawehner, I only reproduce it on the preview and I attached a patch for this.

Ok @rroose.

Thank you!!

@lcontreras thanks for the patch, some feedback:

1. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -688,7 +688,11 @@ public function renderPreview($display_id, $args = []) {
   +                  '#markup' => Xss::filterAdmin($executable->getTitle()),
   

   markup is passed through XSS filter anyway so no need to do that here

2. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -688,7 +688,11 @@ public function renderPreview($display_id, $args = []) {
   +              ]
   

   missing trailing comma

Also @dawehner in #10 was referring to automated tests, not manual tests. This is a bug so we need to add an automated test for this, to make sure it stays fixed.

@Lendude thank you very much for the feedback. I'll keep working on this.

I've attached another patch. I'll try to create the test too.

Removed redundant use statement.

@Jo Fitzgerald thanks!

I'll attach an update on the PreviewTest.php file to test this.

I've updated my patch for the PreviewTest.php file, because the code should be at the final testPreviewUI() function.

@lcontreras thanks for keeping at this!

+++ b/core/modules/views_ui/src/Tests/PreviewTest.php
@@ -114,6 +114,10 @@ public function testPreviewUI() {
+    //Test preview title double escaped

Well we need a test that it isn't double escaped, so I would make the comment
"Test that the preview title isn't double escaped.".
Also the comment needs a space after the // and a period at the end.

Also the test needs an actual assertion at the end, so we can show that it fails without the fix and passes with the fix.

So we'd need to add $this->assertText('Double & escaped'); at the end.

And then we would need a patch that contains both the test and the fix.

@Lendude thank you. I' ll keep working on this.

Add a new patch with test and fix.

re: #23: Using assertNoEscaped seems to be the right choice here.
Patch looks good to me, +1 to RTBC.

Yeah this looks good, we got a fix and we have a test. Nice work

Thanks @Lendude for your support!!

+++ b/core/modules/views_ui/src/Tests/PreviewTest.php
@@ -114,6 +114,11 @@ public function testPreviewUI() {
+
+    // Test that the preview title isn't double escaped.
+    $this->drupalPostForm("admin/structure/views/nojs/display/test_preview/default/title", $edit = ['title' => 'Double & escaped'], t('Apply'));
+    $this->drupalPostForm(NULL, [], t('Update preview'));
+    $this->assertNoEscaped('Double & escaped');

This looks great except I think we should use the positive assertion from #23 here $this->assertText('Double & escaped');

assertNoEscaped() only checks for the absence of text, so if there was an additional bug that titles weren't displaying at all, the test could still pass as it is now.

It's also useful to post two patches - one with only the test additions (that the automated tests will show as failing) and one with both test and fix.

@Cath thanks to see this. Well, I don't think $this->assertText(); is the rigth assertion to use because the test always fails using it: $this->assertText('Double & escaped'). And we can't use it like this $this->assertText('Double & escaped'); because it always pass event when the fix is not applied. With this assertion $this->assertNoEscaped the test fail withouth the fix and pass with the fix.

@lcontreras the point that @catch makes is very valid (and I should have spotted it). Only having a negative assertion isn't really useful. Some other change could completely break the title output, but our test would still pass as long as it didn't change it to 'Double & escaped'.

So we need a positive assertion here. And with the addition of a positive assertion, the negative assertion can be taken out.

$this->assertText('Double & escaped'); always passes (even without the fix), because that text is available elsewhere on the page. So we need a test that specifically targets our preview title.

Since we don't have a lot of css classes to work with here, xpath would probably be the best way to do this:

    $elements = $this->xpath('//div[@id="views-live-preview"]/div[contains(@class, views-query-info)]//td[text()=:text]', array(':text' => t('Double & escaped')));
    $this->assertEqual(1, count($elements));

seems to do the trick

@Lendude, thanks to explain me this. I'll test it using xpath.

OK, I tested and it's working with the xpath and it shouldn't break the title output when something changes.

Nice! Thanks!

Fixing coding standards!

Friendly reminder than not adding interdiffs makes getting a review on your patch a lot harder :)

Here's the interdiff between #35 and #39

Thanks

Thanks Lendude and Manuel.

I missed that the text appears elsewhere on the page, that's a good point and the xpath assertion is great.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!