If I use the views-view-fields.html.twig template in my own theme then the whole content get escaped so that "<" will be &lt; and so on for other braces, quotes etc.

This makes the template pretty unusable. I have found in some places here discussions about double escaping and similar problems. But currently I do not have a clue about the status of this escaping theme in Drupal 8 overall and if a solution for this problem is already insight. But for now I would consider this nearly a major bug for the views implementation since the custom templates are one of the great and powerful features of the views module.

To get past this now the easy way I just put it like this turning TWIG autoescape off

{% autoescape false %}

{% for field in fields %}
  {{ field.separator }}

  {{ field.wrapper_prefix }}
    {{ field.label_html }}
    {{ field.content }}
  {{ field.wrapper_suffix }}
{% endfor %}

{% endautoescape %}

Of course this might be surely not the way it is meant to be, but I could not find a clue where to fix this otherwise.

PS: If I use the views-view.html.twig template in my own theme, there is no such problem.

Beta phase evaluation

-->

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because field values are getting inccorectly escaped
Issue priority Major because it's a bug
Unfrozen changes Unfrozen because its a bug.
Prioritized changes The main goal of this issue is resolving escaping
Disruption Disruptive for core/contributed and custom modules/themes because it will require a BC break/deprecation/data model changes/an upgrade path/internal refactoring/widespread changes... (Which? Specify.)
Files: 
CommentFileSizeAuthor
#86 2363423-86.patch12.95 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,264 pass(es). View
#86 interdiff-80-86.txt1.32 KBstefan.r
#83 2363423-80.patch13 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,249 pass(es). View
#83 interdiff-76-80.txt2.71 KBstefan.r
#76 interdiff.txt1.55 KBManuel Garcia
#76 2363423-76.patch12.39 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,252 pass(es). View
#74 interdiff.txt1.72 KBManuel Garcia
#74 2363423-74.patch12.54 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#70 interdiff-2363423-69-70.txt962 bytesdorficus
#70 2363423-70.patch12.55 KBdorficus
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,191 pass(es). View
#69 interdiff-2363423-68-69.txt652 bytesdorficus
#69 2363423-69.patch12.53 KBdorficus
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,200 pass(es). View
#68 interdiff-2363423-63-68.txt2.13 KBdorficus
#68 2363423-68.patch12.44 KBdorficus
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,188 pass(es). View
#63 interdiff-2363423-62-63.txt982 bytesdorficus
#63 2363423-63.patch12.51 KBdorficus
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,189 pass(es). View
#62 2363423-62.patch12.28 KBdorficus
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#61 interdiff-2363423-57-61.txt3.12 KBdorficus
#61 2363423-61.patch12.31 KBdorficus
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#59 interdiff-57-53.txt2.8 KBManuel Garcia
#57 2363423-57.patch13.81 KBdorficus
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,536 pass(es). View
#54 after-theme-vs-twig-diff.png266.64 KBjoelpittet
#54 theme-diff.png155.51 KBjoelpittet
#54 after-patch-twig-template.png193.14 KBjoelpittet
#54 after-patch-theme-function.png193.66 KBjoelpittet
#53 interdiff.txt806 bytesdorficus
#53 2363423-53.patch13.62 KBdorficus
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,595 pass(es). View
#52 2363423-52.patch13.71 KBdorficus
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,242 pass(es). View
#47 missing.png409.76 KBjoelpittet
#32 2363423-32.patch13.63 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,300 pass(es). View
#32 2363423-32-testonly.patch4.2 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,276 pass(es), 1 fail(s), and 0 exception(s). View
#28 2363423-21.patch7.66 KBprajaankit
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,962 pass(es), 34 fail(s), and 6,266 exception(s). View
#20 interdiff.txt556 bytesManuel Garcia
#20 2363423-20.patch9.43 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,272 pass(es). View
#18 interdiff.txt518 bytesManuel Garcia
#18 2363423-18.patch9.51 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,269 pass(es). View
#16 interdiff.txt833 bytesManuel Garcia
#16 2363423-16.patch9.5 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#14 interdiff.txt1.23 KBManuel Garcia
#14 2363423-14.patch9.45 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,120 pass(es), 1 fail(s), and 1,295 exception(s). View
#10 interdiff.txt8.24 KBManuel Garcia
#10 2363423-10.patch9.83 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,107 pass(es), 1 fail(s), and 1,295 exception(s). View
#3 interdiff.txt530 bytesManuel Garcia
#3 2363423-3.patch2.12 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,390 pass(es). View
#2 2363423-2.patch1.52 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,390 pass(es). View

Comments

Cottser’s picture

Version: 8.0.0-beta2 » 8.0.x-dev
Issue tags: +Twig
Related issues: +#2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function

Thanks for the report @stefan.korn!

Manuel Garcia’s picture

Status: Active » Needs review
FileSize
1.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,390 pass(es). View

Here's what we were doing on #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function, taken out so we can at least fix this one.
I've tested it briefly, stuff is still escaped, let's see what the bot thinks.

Manuel Garcia’s picture

FileSize
2.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,390 pass(es). View
530 bytes

Oops, forgot one on the theme function.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Looks good, thanks @Manuel Garcia. Can we get some tests please to show things are getting properly escaped and not double escaped?

Edit: Importantly, we should have a test-only patch that fails without the patch here.

Manuel Garcia’s picture

So forgive me if I'm mistaken, but if we don't remove the theme function (See comment 77 on #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function), this patch then needs to be reworked, so that the sanitization takes place on the theme function instead. That way when you are not using the twig file, stuff still gets sanitized. Does that sound right?

Cottser’s picture

Yes that sounds right, two potential code paths: theme function and template.

Manuel Garcia’s picture

OK I've been doing some testing on this one, and in order for us to fix this bug we have to move the markup generation part out of template_preprocess_views_view_fields, so stuff like:

$object->wrapper_prefix = '<' . $object->inline_html;
        $object->wrapper_prefix .= $attributes;
        $object->wrapper_prefix .= '>';
        $object->wrapper_suffix = '</' . $object->inline_html . '>';

has to be moved into the theme function, and also into the template file.

Otherwise, doing the sanitizing on the theme function will result in escaped html being printed.

How should we do this, on this issue, a new one, or on #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function ?

Cottser’s picture

If it's necessary to fix the escaping then this issue seems appropriate :)

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
9.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,107 pass(es), 1 fail(s), and 1,295 exception(s). View
8.24 KB

Some progress on this.

I'm not sure how to handle sanitizing the content... before the patch, it was done on render() using sanitizeValue(). But now from the theme function not sure how to go about it. Any pointers?

Cottser’s picture

Thanks @Manuel Garcia, I don't have a thought yet for how to fix the theme function not escaping, but here's a small point.

+++ b/core/modules/views/templates/views-view-fields.html.twig
@@ -25,17 +27,24 @@
-<!--
-THIS FILE IS NOT USED AND IS HERE AS A STARTING POINT FOR CUSTOMIZATION ONLY.
-See http://api.drupal.org/api/function/theme_views_view_fields/8 for details.
-After copying this file to your theme's folder and customizing it, remove this
-HTML comment.
--->

This comment should be kept.

Cottser’s picture

Status: Needs review » Needs work

Actually Drupal\views\Plugin\views\HandlerBase::sanitizeValue() is just a fancy wrapper for Xss and SafeMarkup functions, and in this case I think it's just sending the output to SafeMarkup::checkPlain().

The last submitted patch, 10: 2363423-10.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
9.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,120 pass(es), 1 fail(s), and 1,295 exception(s). View
1.23 KB

Good catch Cottser on the comment, restored it now in this patch.

I've done some testing by copying over the twig template into bartik, and if we keep the sanitization on FieldPluginBase::render, the field.content does NOT get double escaped - looks like we can keep it there. So this patch restores that change. Could use with double checking, I suppose.

Status: Needs review » Needs work

The last submitted patch, 14: 2363423-14.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
9.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
833 bytes

This should get rid of all those exceptions. Not sure what's going on with the failing test though.

Status: Needs review » Needs work

The last submitted patch, 16: 2363423-16.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
9.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,269 pass(es). View
518 bytes

More on fixing exceptions..

penyaskito’s picture

  1. +++ b/core/modules/views/views.theme.inc
    @@ -219,12 +198,33 @@ function theme_views_view_fields($variables) {
    +    $wrapper_element = SafeMarkup::checkPlain($field->wrapper_element);
    +    if ($wrapper_element) {
    +      $output .= '<' . $wrapper_element . $field->wrapper_attributes . '>';
    +      if (isset($field->label_element) && !empty($field->label_element)) {
    +        $label_element = SafeMarkup::checkPlain($field->label_element);
    +        $output .= '<' . $label_element . $field->label_attributes . '>';
    +      }
    +      $output .= SafeMarkup::checkPlain($field->label);
    +      if (isset($field->label_suffix) && isset($field->label_suffix)) {
    ...
    +    if ($element_type) {
    +      $output .= '</' . $element_type . '>';
    +    }
    +    if ($wrapper_element) {
    +      $output .= '</' . $wrapper_element . '>';
    +    }
       }
    

    Should we use a template/inline template here instead?

  2. +++ b/core/modules/views/views.theme.inc
    @@ -219,12 +198,33 @@ function theme_views_view_fields($variables) {
    +    // @todo this is not sanitized in render, cant use sanitizeValue from here
    

    If we add a todo, let's create an issue for that.

Manuel Garcia’s picture

FileSize
9.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,272 pass(es). View
556 bytes

Thanks @penyaskito for the review!
We've already discussed it on IRC, but I'll answer here as well for people reading the issue.

  1. See #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function (not getting in because of perf)
  2. Good catch, that's actually not necessary any more, this patch now removes that (see #14).
penyaskito’s picture

Status: Needs review » Needs work

Looks good to me, but still needs the test.

xjm’s picture

Priority: Normal » Major
Issue tags: +SafeMarkup

Oh, this is a nice improvement. This will also help us with #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table().

+++ b/core/modules/views/views.theme.inc
@@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
+    $wrapper_element = SafeMarkup::checkPlain($field->wrapper_element);
...
+        $label_element = SafeMarkup::checkPlain($field->label_element);
+        $output .= '<' . $label_element . $field->label_attributes . '>';
...
+    $element_type = SafeMarkup::checkPlain($field->element_type);
+    if ($element_type) {
+      $output .= '<' . $element_type . $field->element_attributes . '>';
+    }

I think we should not just be checkPlain()ing the tag -- we should be validating it against an allowed and valid tags list. Maybe the best thing to do would be to instead compose the whole markup string without any early sanitization and then run it through SafeMarkup::checkAdminXss().

Also, is there any possibility of converting $output to a render array? Then we could either put it in #markup (which would make it be XSS-filtered automatically) or use more explicit render array structure.

Cottser’s picture

As for converting $output to a render array, if so it would need to be immediately rendered so probably not much point that I can see. Theme functions must return strings.

xjm’s picture

@Cottser, thanks! So I'd go with the first suggestion then: assemble the HTML string, then checkAdminXss() on the whole thing. And if someone wants to use tags that are not allowed in the admin list, they can do that in code rather than through the UI.

xjm’s picture

Mmm, just noticed that the template also uses the elements right there. What's the relationship between $output and the template? How can we ensure the template is fully sanitized for XSS? Maybe we should check against the admin tag list? checkPlain() is a wash if we're putting angle brackets around it.

Cottser’s picture

@xjm yep that's a good point. Anything output in the template that isn't SafeMarkup (or similar with the new things we are going to introduce like SafeString) will be escaped, but script escaped == script.

This may be a "if you want to shoot yourself in the foot" case as long as Views only provides safe options for wrappers and doesn't provide a text input so you can enter whatever you want. Not sure.

ibnkhaldun’s picture

Hi!
I'm newy at D8. So perhaps my comment is wrong. I'm reading twig documentation and found there are 2 filters to avoid double escaped strings:

{% autoescape 'html' %}
    {{ var }}
    {{ var|raw }}      {# var do not escapes #}
    {{ var|escape }}   {# var do not escapes a second time #}
{% endautoescape %}

Thus the sample you wrote should be wroten as

{% autoescape false %}
{% for field in fields %}
  {{ field.separator|escape }}


  {{ field.wrapper_prefix }}
    {{ field.label_html|escape }}
    {{ field.content|escape }}
  {{ field.wrapper_suffix|escape }}
{% endfor %}


{% endautoescape %}

Or you can set the filter "|escape" only where you know you'l have a double-escaped string.

Hope it helps.

Oh! Sorry i was reading on double-escape and came here from a link. And my comment out of branche's target

prajaankit’s picture

FileSize
7.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,962 pass(es), 34 fail(s), and 6,266 exception(s). View
rootwork’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 28: 2363423-21.patch, failed testing.

Cottser’s picture

The patch from #20 is still fine. @prajaankit in the future please leave a comment when uploading a new patch explaining what you're doing.

I'm going to see if I can rough in a test here.

Cottser’s picture

Status: Needs work » Needs review
FileSize
4.2 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,276 pass(es), 1 fail(s), and 0 exception(s). View
13.63 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,300 pass(es). View

It's a start, anyway.

The last submitted patch, 32: 2363423-32-testonly.patch, failed testing.

lauriii’s picture

Issue tags: +Needs beta evaluation
joelpittet’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation

Beta eval added.

joelpittet’s picture

This is such a nice clean up! And it has tests. I'd like to see some manual testing on this.

Steps:

  1. Apply the patch.
  2. Copy the template to your theme's templates folder and flush cache.
  3. Make a view with fields (or use an existing one).
  4. Compare the markup before and after without the patch version of the theme_function and twig template.
  5. Compare the markup before the template move and after the patch
joelpittet’s picture

Issue tags: +Novice

I'll pick this up tomorrow if nobody snags it before then.

davidhernandez’s picture

I coped the template to Classy and check a simple View before and after. Screenshots attached.

I realized something. Is it appropriate that the "THIS FILE IS NOT USED AND IS HERE AS A STARTING POINT FOR CUSTOMIZATION ONLY." text is there as an html comment and not a Twig comment? That makes it visible in the markup.

joelpittet’s picture

@davidhernandez re "THIS FILE IS NOT USED AND IS HERE AS A STARTING POINT FOR CUSTOMIZATION ONLY."

That's a bit of legacy porting, probably should be Twig but if we can get that twig template in, it shouldn't matter. Feel free to open up a follow-up if you want to remove it quickly. I'll RTBC it right away if you ping me.

joelpittet’s picture

Thanks for covering off #36.4 @davidhernandez

davidhernandez’s picture

I guess that is actually 5, since I moved the template. I didn't check before and after without moving the template.

joelpittet’s picture

@davidhernandez so that is "with the patch" test in #38?

davidhernandez’s picture

That is before and after the patch, but with the template moved to Classy in both cases. I did not try things as-is, leaving the template alone, which I assume is what you want for testing the changes to the theme function?

joelpittet’s picture

Thanks for the manual testing @davidhernandez!

I did some too, the scenario was to take the existing admin content, convert from table to unformatted list and join the last 3 columns inline with a |.

Reason I did the testing this way is to make sure the changes that were outside the template didn't break the existing theme function in some way.

Before Patch

With theme function

With twig template

After Patch

With theme function

With twig template

Turns out there is a missing | between the first and second inline item after this patch is applied.

You can see this in the screenshots after Status: Published|

joelpittet’s picture

The good thing is that they are consistent between the output of the theme and twig template after the patch is applied., the bad thing is we lost the separator:S

Likely need to add that to the tests.

davidhernandez’s picture

To be clear, you are talking about losing the pipe, |, between "Published" and "Updated"?

joelpittet’s picture

Issue summary: View changes
Issue tags: -Novice
FileSize
409.76 KB

Yes

davidhernandez’s picture

@joel, what do you use to get those markers on the screenshot?

joelpittet’s picture

dorficus’s picture

FileSize
26.99 KB

I rerolled the patch to be a little more current.

There were conflicts, however all I really needed to do was clean the 3-4 lines of code that were causing the issue. They were found in views-view-fields.html.twig at around line 34

dorficus’s picture

I rerolled the patch to be a little more current.

There were conflicts, however all I really needed to do was clean the 3-4 lines of code that were causing the issue. They were found in views-view-fields.html.twig at around line 34

EDIT: i messed this one up. disregard

dorficus’s picture

FileSize
13.71 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,242 pass(es). View

Reroll without the messup. My apologies.

dorficus’s picture

Status: Needs work » Needs review
FileSize
13.62 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,595 pass(es). View
806 bytes

Interdiff fixing twig comment close

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
193.66 KB
193.14 KB
155.51 KB
266.64 KB

Ok so something was wrong with my install in my tests I think because I've had @dorficus and I redo a manual test without being able to reproduce that issue. Also I believe @davidhernandez also couldn't get that to show up like that either.

Here's the new manual testing for #53

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
    @@ -0,0 +1,61 @@
    +    \Drupal::service('theme_handler')->install(array('views_test_theme'));
    

    Don't we need a test with and without this test theme installed?

  2. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
    @@ -0,0 +1,61 @@
    +    $this->assertNoRaw('&lt;/');
    

    $this->assertNoEscaped('<') is easier on the eye.

  3. +++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
    @@ -0,0 +1,48 @@
    +{#
    +/**
    + * @file
    + * Theme override to display all the fields in a views row.
    + *
    

    I think this test twig file should have a comment stating that it exists because by default the one provided by views is not used.

  4. +++ b/core/modules/views/views.theme.inc
    @@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
    +    $wrapper_element = SafeMarkup::checkPlain($field->wrapper_element);
    +    if ($wrapper_element) {
    +      $output .= '<' . $wrapper_element . $field->wrapper_attributes . '>';
    +      if (isset($field->label_element) && !empty($field->label_element)) {
    +        $label_element = SafeMarkup::checkPlain($field->label_element);
    +        $output .= '<' . $label_element . $field->label_attributes . '>';
    +      }
    +      $output .= SafeMarkup::checkPlain($field->label);
    +      if (isset($field->label_suffix) && isset($field->label_suffix)) {
    +        $output .= $field->label_suffix;
    +      }
    +      if (isset($label_element)) {
    +        $output .= '</' . $label_element . '>';
    +      }
    +    }
    +    $element_type = SafeMarkup::checkPlain($field->element_type);
    +    if ($element_type) {
    +      $output .= '<' . $element_type . $field->element_attributes . '>';
    +    }
         $output .= $field->content;
    -
    -    $output .= $field->wrapper_suffix;
    +    if ($element_type) {
    +      $output .= '</' . $element_type . '>';
    +    }
    +    if ($wrapper_element) {
    +      $output .= '</' . $wrapper_element . '>';
    +    }
    

    The calls to SafeMarkup::checkPlain() should just be htmlspecialchars() as their safeness is immediately lost through string concatenation.

dawehner’s picture

+++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
@@ -0,0 +1,61 @@
+    $this->assertEqual($this->config('system.theme')->get('default'), 'views_test_theme');

IMHO this line is pointless. We should no test that config API works as expected

+++ b/core/modules/views/templates/views-view-fields.html.twig
--- /dev/null
+++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig

+++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
+++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
@@ -0,0 +1,48 @@

Given that this is functional identical to views-view-fields.html.twig i'm curious whether you could just use a twig include here

dorficus’s picture

Status: Needs work » Needs review
FileSize
13.81 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,536 pass(es). View

There's an issue that has some theme functions on the chopping block so that test without a theme install was not added. I did change the `<` to `<` and added the commenting as requested. I also changed the SafeMarkup::checkPlain methods to htmlspecialchars() function.

EDIT: I didn't see your request before I pushed this patch. I'll take care of those others.

davidhernandez’s picture

dorficus, can you post an interdiff?

Manuel Garcia’s picture

FileSize
2.8 KB

Here is the interdiff between #53 and #57.

dorficus’s picture

@davidhernandez I apologize for forgetting the interdiff. Thank you @manuelgarcia for posting it.

dorficus’s picture

FileSize
12.31 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
3.12 KB

Changed to include, removed redundant test, added test text to make sure we're using right template.

dorficus’s picture

FileSize
12.28 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Had a no-newline error due to new IDE. Sorry.

No interdiff due to just one file with one line added to end.

dorficus’s picture

FileSize
12.51 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,189 pass(es). View
982 bytes

Added a test without the theme installed

The last submitted patch, 61: 2363423-61.patch, failed testing.

The last submitted patch, 62: 2363423-62.patch, failed testing.

joelpittet’s picture

Status: Needs review » Needs work

This looks great! Only nitpicks:)

  1. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
    @@ -46,6 +46,14 @@ protected function setUp() {
       public function testViewsViewFieldsEscaping() {
    +
    

    Don't need this extra space after the function start.

  2. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
    @@ -46,6 +46,14 @@ protected function setUp() {
    +    // Assert that there are no escaped '<'s
    ...
    +
    +    // Install theme to test with template system
    

    Periods at the end of sentences.

  3. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
    @@ -62,5 +70,4 @@ public function testViewsViewFieldsEscaping() {
       }
    -
     }
    

    We actually want this space between the function and the ending class for coding standards.

  1. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
    @@ -0,0 +1,73 @@
    +  public static $testViews = array('test_page_display');
    +
    +
    +  /**
    

    One new line here can be removed.

  2. +++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
    @@ -0,0 +1,10 @@
    + * This exists because by default the one provided by Views is not used.
    

    The reason for this template is to override the theme function provided by views. And nit: don't need to capitalize views.

  3. +++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
    @@ -0,0 +1,10 @@
    +{% include '@views/views-view-fields.html.twig' %}
    +May the force be with you!
    

    Wicked.

dorficus’s picture

Handling the code issues

dorficus’s picture

Status: Needs work » Needs review
FileSize
12.44 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,188 pass(es). View
2.13 KB

Glance over this one and let me know if I missed anything.

dorficus’s picture

FileSize
12.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,200 pass(es). View
652 bytes

Changed a comment that I missed in the previous one.

dorficus’s picture

FileSize
12.55 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,191 pass(es). View
962 bytes

Somehow I lost some words. I found them and they're back under control.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for fixing all the nits!

This looks great to me and the tests look great and if the views template changes then we don't have to duplicate them to this copied template.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

htmlspecialchars() needs to be replaced with Html::escape()

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Working on #72.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
12.54 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
1.72 KB

Addressing #72.

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Manuel Garcia’s picture

FileSize
12.39 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,252 pass(es). View
1.55 KB

Oops, Html::escape() only takes one string as an argument...

dawehner’s picture

+++ b/core/modules/views/views.theme.inc
@@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
+        $output .= $field->label_suffix;

Is it okay to not escape $field->label_suffix?

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

Html::escape() replacements look good

+++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
@@ -0,0 +1,72 @@
+    // Assert that there are no escaped '<'s characters.
...
+    // Assert that there are no escaped '<'s characters.

superfluous s

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@dawehner++

+++ b/core/modules/views/views.theme.inc
@@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
+      if (isset($field->label_suffix) && isset($field->label_suffix)) {
+        $output .= $field->label_suffix;
+      }

In fact what is label_suffix? It does not seem to ever exist.

alexpott’s picture

Ah it is introduced by the patch.

+++ b/core/modules/views/views.theme.inc
@@ -149,26 +136,24 @@ function template_preprocess_views_view_fields(&$variables) {
         if ($object->handler->options['element_label_colon']) {

Why not just pass the boolean into the template? Setting the suffix outside means we need to escape. Whereas if the twig template and theme_views_view_fields handled it it'd be obvious it was safe.

The last submitted patch, 74: 2363423-74.patch, failed testing.

davidhernandez’s picture

+++ b/core/modules/views/views.theme.inc
@@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
+      if (isset($field->label_suffix) && isset($field->label_suffix)) {

I'm not seeing things am I? The isset is written twice. Was the second one suppose to be a !empty?

stefan.r’s picture

Status: Needs work » Needs review
FileSize
2.71 KB
13 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,249 pass(es). View
joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views/templates/views-view-fields.html.twig
    @@ -17,7 +17,8 @@
    + *   - has_label_colon: A boolean indicating whether to display a colon after
    

    For twig coding standards we don't use types https://www.drupal.org/node/1823416#datatypes

    Though I think we should update that rule for booleans because we do that type frequently. @Cottser thoughts?

  2. +++ b/core/modules/views/templates/views-view-fields.html.twig
    @@ -40,9 +41,9 @@
    +      <{{ field.label_element }}{{ field.label_attributes }}>{{ field.label }}{% if field.has_label_colon %} :{% endif %}</{{ field.label_element }}>
    

    Minor clean-up but this could be nicer written as

    {{ field.has_label_colon ? ': ' }}

    But also the space is on the wrong side of the colon.

alexpott’s picture

+++ b/core/modules/views/views.theme.inc
@@ -90,6 +90,9 @@
+      // Set up default value of the flag that indicates whether to display a
+      // colon after the label.
+      $object->has_label_colon = FALSE;

@@ -206,8 +209,8 @@
+      if (isset($field->has_label_colon) && $field->has_label_colon) {

Do you need the isset() now? I think not.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
12.95 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,264 pass(es). View
Cottser’s picture

@joelpittet/#84: yeah the special naming, not sure. I thought we had written up those conventions somewhere but maybe not.

Manuel Garcia’s picture

+++ b/core/modules/views/templates/views-view-fields.html.twig
@@ -11,13 +11,16 @@
+ *   - has_label_colon: A boolean indicating whether to display a colon after
+ *     the label.

Well, if has to be themer friendly, how about:

"Whether or not to display a colon after the label."

Status: Needs review » Needs work

The last submitted patch, 86: 2363423-86.patch, failed testing.

Status: Needs work » Needs review

stefan.r queued 86: 2363423-86.patch for re-testing.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

@Manuel Garcia it's not really about being "themer friendly" it's mostly because Twig treats arrays like objects in our their keys are accessed, from what I recall.

That could be a better change but I'd rather not hold up this blocker on a comment that isn't clearly documented.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e76a96f and pushed to 8.0.x. Thanks!

  • alexpott committed e76a96f on 8.0.x
    Issue #2363423 by dorficus, Manuel Garcia, stefan.r, Cottser, prajaankit...
dawehner’s picture

Nice work

sgalindo2388’s picture

I tried this patch with Beta 14 and I get this error

"Fatal error: Call to undefined method Drupal\Component\Utility\Html::escape() in /core/modules/views/views.theme.inc on line 205"

Any ideas on what might be going on?

stefan.r’s picture

@sgalindo this will not work with beta14, you'll need to use HEAD instead

joelpittet’s picture

@sgalindo2388 if you want to play with it that method was added in #2550945: Add Html::escape() But you'd be better off with HEAD like @stefan.r suggested.

sgalindo2388’s picture

@stefan.r @joelpittet Thank you for the suggestions. using HEAD worked.

Status: Fixed » Closed (fixed)

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