Follow-up to #2280965: [meta] Remove every SafeMarkup::set() call

Problem/Motivation

template_preprocess_views_view_table calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code. (COMPLETED)
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.
  • To test the conversion to a render array: Edit the content admin page view, and edit one of the fields, and customize the field HTML element (under Style settings)

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123 COMPLETED
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.COMPLETED
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.NOT APPLICABLE

Manual testing steps (for double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Visit the content admin page at admin/content
  3. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#71 2502089-71.patch9.59 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,933 pass(es). View
#71 interdiff-66-71.txt3.19 KBstefan.r
#66 interdiff.txt764 bytesMauPalantir
#66 remove-2502089-66.patch9.36 KBMauPalantir
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,884 pass(es). View
#59 interdiff.txt3.9 KBMauPalantir
#59 remove-2502089-57.patch9.34 KBMauPalantir
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,866 pass(es), 3 fail(s), and 0 exception(s). View
remove-2502089-57.patch9.34 KBMauPalantir
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,865 pass(es), 3 fail(s), and 0 exception(s). View
interdiff.txt3.9 KBMauPalantir
#53 interdiff.txt1.49 KBjoelpittet
#53 remove-2502089-53.patch8.75 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,614 pass(es), 1 fail(s), and 0 exception(s). View
#51 remove-2502089-50.patch8.67 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,606 pass(es), 1 fail(s), and 0 exception(s). View
#51 interdiff.txt3.92 KBjoelpittet
#49 remove-2502089-49.patch8.61 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/views/views.theme.inc. View
#49 interdiff.txt8.99 KBjoelpittet
#46 remove-2502089-44.png29.74 KBMauPalantir
#46 orig.png32.81 KBMauPalantir
#44 interdiff-22-44.txt4.94 KBjoelpittet
#44 remove-2502089-44.patch4.85 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,370 pass(es), 290 fail(s), and 2 exception(s). View
#22 interdiff-2502089-5-22.txt812 bytesakalata
#22 remove_safemarkup_set-2502089-22.patch2.57 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,936 pass(es). View
#5 interdiff.txt930 bytesjoelpittet
#5 remove_safemarkup_set-2502089-5.patch2.51 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,844 pass(es). View
#3 remove_safemarkup_set-2502089-3.patch2.51 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,457 pass(es), 273 fail(s), and 2 exception(s). View
#3 interdiff.txt1.53 KBjoelpittet
#1 remove_safemarkup_set-2502089-1.patch3.31 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,458 pass(es), 273 fail(s), and 2 exception(s). View

Comments

joelpittet’s picture

Status: Active » Needs review
FileSize
3.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,458 pass(es), 273 fail(s), and 2 exception(s). View

May have overstepped on one but let's see.

Status: Needs review » Needs work

The last submitted patch, 1: remove_safemarkup_set-2502089-1.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
2.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,457 pass(es), 273 fail(s), and 2 exception(s). View

Let's see without the overstep.

Status: Needs review » Needs work

The last submitted patch, 3: remove_safemarkup_set-2502089-3.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,844 pass(es). View
930 bytes

Made a few typos in the last one.

Cottser’s picture

Title: Remove SafeMarkup::set in core/modules/views/views.theme.inc » Remove SafeMarkup::set in template_preprocess_views_view_table()
akalata’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

manually tested this patch; the HTML output is identical.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: remove_safemarkup_set-2502089-5.patch, failed testing.

The last submitted patch, 5: remove_safemarkup_set-2502089-5.patch, failed testing.

akalata’s picture

Issue summary: View changes
Status: Needs work » Needs review

Manually tested this patch; the HTML output is identical.

Patch replaces SafeMarkup::set with an inline template in one instance, and with SafeMarkup::format in 3 others

Prep for RTBC once testbot starts working again:

  1. +++ b/core/modules/views/views.theme.inc
    @@ -484,8 +484,7 @@ function template_preprocess_views_view_table(&$variables) {
    +          $label = [['#markup' => $label], $tablesort_indicator];
    

    - $label is passed through SafeMarkup::checkPlain
    - content provided by #theme tablesort_indicator is presumed safe

  2. +++ b/core/modules/views/views.theme.inc
    @@ -576,7 +575,7 @@ function template_preprocess_views_view_table(&$variables) {
    +          $field_output = SafeMarkup::format('<@element_type>@field_output</@element_type>', ['@element_type' => $element_type, '@field_output' => $field_output]);
    

    - $element_type comes from Markup::elementType, where user-provided elements are passed through SafeMarkup::checkPlain
    - $field_output passed through HandlerBase::sanitizeValue as part of $handler->getField (FieldPluginBase::render)

  3. +++ b/core/modules/views/views.theme.inc
    @@ -584,17 +583,14 @@ function template_preprocess_views_view_table(&$variables) {
    +              $column_reference['content'] = SafeMarkup::format('@content@separator', ['@content' => $column_reference['content'], '@separator' => $safe_separator]);
                 }
    

    Optional separator is passed through Xss::filterAdmin

  4. +++ b/core/modules/views/views.theme.inc
    @@ -584,17 +583,14 @@ function template_preprocess_views_view_table(&$variables) {
    +          $column_reference['content'] = SafeMarkup::format('@content@field_output', ['@content' => $column_reference['content'], '@field_output' => $field_output]);
    

    - Columns are passed through Table::sanitizeColumns at the beginning of this function

akalata’s picture

Issue summary: View changes
akalata’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Aw, I was hoping this would be another slaying of the preprocess altogether. :) Is there a reason that's not doable here?

akalata’s picture

There's a lot more going on in template_preprocess_views_view_table() than the SafeMarkup::set, so removing the preprocess completely would be out of scope at the very least (and my intuition says is probably impossible).

xjm’s picture

Assigned: Unassigned » joelpittet
Status: Reviewed & tested by the community » Needs review

So I was confusing this issue with #2502781: Remove SafeMarkup::set in template_preprocess_file_widget_multiple() which is the one that got unblocked by the table rendering fix.

I discussed this at length with @akalata and @davidhernandez. They pointed out that because this is a table, converting this logic to template logic would be much more difficult than #2502095: Remove SafeMarkup::set in template_preprocess_views_ui_view_info(), and recommended going forward with this patch (since it's an improvement over HEAD). I asked if there was an existing issue to remove or at least reduce this preprocess function (since I would feel better about committing this with an @todo), and @akalata referenced #1742890: [meta] What will preprocess functions look like with Twig? and #2348381: [META-0 theme functions left] Convert/refactor core theme functions and #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates., but did not find any specific issues about this. @davidhernandez and @akalata also discussed the pros and cons a bit of converting this preprocess to template logic, but agreed that it would be an extensive amount of work.

@joelpittet was talking yesterday in IRC about a long-term hope to improve table theming, so I'd like some more feedback from him on this point as to whether there's a way we can leverage templates more here, or for Views tables in general, or for tables in general, and whether there are followup issues we should reference. Assigning to him at NR for that.

xjm’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/views.theme.inc
@@ -576,7 +575,7 @@ function template_preprocess_views_view_table(&$variables) {
-          $field_output = SafeMarkup::set('<' . $element_type . '>' .  SafeMarkup::escape($field_output) . '</' . $element_type . '>');
+          $field_output = SafeMarkup::format('<@element_type>@field_output</@element_type>', ['@element_type' => $element_type, '@field_output' => $field_output]);

So, hmm. I just noticed this now that I'm reading the patch more closely.

This is not safe. At least in the context of this line, for all we know, @element_type could be script.

At a minimum, we need a comment to document what @akalata said in her review, but could we actually convert this in particular (and possibly other things in this patch as well) to render arrays? We're already using them elsewhere in this preprocess.

xjm’s picture

This is actually a great specific example where "refactor the code flow to make it clear that variables are safe" could also apply.

akalata’s picture

Status: Needs work » Needs review

I do see how the combination of @element_type with @field_output could result in an unsafe string, but I think this might be another case where we bump up against the need to accept what we're getting (similar to #2273925: Ensure #markup is XSS escaped in Renderer::doRender() or #2501403: Document SafeMarkup::set in Xss::filter). See also pwolanin/joelpittet's conversation in the latter (#11 and 12).

There are a few other SafeMarkup issues where we're struggling with the same idea:

I found a few issues around the discussion of theme_table in general, which would affect this preprocess function:

xjm’s picture

A difference here versus the issues listed above is that this SafeMarkup call is not internal to the sanitization APIs. This is a theme preprocess function.

It should be straightforward to convert it to a render array. One line of the patch already uses one.

akalata’s picture

Issue summary: View changes
FileSize
2.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,936 pass(es). View
812 bytes

I had to call drupal_render manually here, since I don't know enough about how the render API delays rendering until later in the process.

I've updated the issue summary to demonstrate how to test customizable $element_type, which is being supplied to Views UI through views.settings.field_rewrite_elements and probably do not need to be escaped. Combined with the santization checking of the actual field value in FieldPluginBase::render, I think we can consider this result of the #type => html_tag to be safe.

YesCT’s picture

what would we call this general approach in words?

In the summary it says
"Remove the call by refactoring the code."

So this is ... using SafeMarkup::format with two placeholders and concatenating two strings?

  1. +++ b/core/modules/views/views.theme.inc
    @@ -484,8 +484,7 @@ function template_preprocess_views_view_table(&$variables) {
    -          $markup = drupal_render($tablesort_indicator);
    -          $label = SafeMarkup::set($label . $markup);
    +          $label = [['#markup' => $label], $tablesort_indicator];
    

    is this one using a render array?

  2. +++ b/core/modules/views/views.theme.inc
    @@ -576,7 +575,12 @@ function template_preprocess_views_view_table(&$variables) {
    -          $field_output = SafeMarkup::set('<' . $element_type . '>' .  SafeMarkup::escape($field_output) . '</' . $element_type . '>');
    +          $tagged_output = array(
    +            '#type' => 'html_tag',
    +            '#tag' => $element_type,
    +            '#value' => $field_output
    +          );
    +          $field_output = drupal_render($tagged_output);
    

    this one is using a render array.

  3. +++ b/core/modules/views/views.theme.inc
    @@ -584,17 +588,14 @@ function template_preprocess_views_view_table(&$variables) {
    -              $safe_content = SafeMarkup::escape($column_reference['content']);
                   $safe_separator = Xss::filterAdmin($options['info'][$column]['separator']);
    -              $column_reference['content'] = SafeMarkup::set($safe_content . $safe_separator);
    +              $column_reference['content'] = SafeMarkup::format('@content@separator', ['@content' => $column_reference['content'], '@separator' => $safe_separator]);
    ...
    -          $safe_content = SafeMarkup::escape($column_reference['content']);
    -          $safe_field_output = SafeMarkup::escape($field_output);
    -          $column_reference['content'] = SafeMarkup::set($safe_content . $safe_field_output);
    +          $column_reference['content'] = SafeMarkup::format('@content@field_output', ['@content' => $column_reference['content'], '@field_output' => $field_output]);
    

    these two are using format() to concatenate two strings.

YesCT’s picture

Status: Needs review » Needs work
FileSize
293.78 KB
283.8 KB

I'm not sure if I'm testing the correct thing, cause this causes double escaped html in both head and the patch.

If this patch is supposed to fix this, maybe that makes this a bug and not a task?

....

Or if this is not what this issue is about, I think I need even more specific instructions on what to test. ... and we need a separate issue to fix that double escaping.

Here is what I did:

  1. installed 8.0.x (once with just head, and another with the patch)
  2. added one article (I think this step might not be necessary for this issue)
  3. went to admin/content
  4. and compared the html with and without the patch (by viewing the source, copying the entire page source, pasting it in a file called front.txt in a directory I made in /tmp/compare, and git init'ing that dir and adding that file, git add and commiting that front.txt. Then I viewed the source with the patch and replaced that file with the patch page source, made a new branch, git add and committed it there. and then did a git diff --color-words. and the only differences were hash-type strings. all other html was the same)
  5. Edited the view for admin/content at admin/structure/views/view/content, and picked a field (title), expanded the style settings, picked Customize label HTML, and chose H2, saved the view.
  6. Reloaded the admin/content. and saw the label for header double escaped.

If this issue is to fix that double escaping that is in head, we should add a test.

akalata’s picture

Assigned: joelpittet » akalata

Addressing #23

This is removing SafeMarkup::set across 4 uses in a single function; the specific approach for each is not necessarily identical across each instance.

  1. Yes, the definition of the render array already exists in HEAD just before the lines affected by this patch.
    $tablesort_indicator = array(
      '#theme' => 'tablesort_indicator',
      '#style' => $initial,
    );
    

Re #24, I think that the double-escaping in HEAD could make this a bug fix rather than only a task, but I don't think the double-escaping fix needs to be a new issue. Your testing approach is spot-on. I will work on that fix this afternoon.

akalata’s picture

Assigned: akalata » Unassigned

Trying to replicate the double-escaping I noticed a detail that I'd missed -- this patch addresses the SafeMarkup::set around the field element, whereas YesCT's testing revealed double escaping in the label element.

This double escaping is likely coming from a line not already included in this patch,
$label = SafeMarkup::checkPlain(!empty($fields[$field]) ? $fields[$field]->label() : '');. So now I'm thinking this bug should be a new issue.

akalata’s picture

Status: Needs work » Needs review
akalata’s picture

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

OK. tested this time with the field html.

head, with no change to view, markup:

                                                                                        
            One
          

with patch, with no change to view, markup:

                                                                                        
            One
          

head, with h2, markup:

                                                                                        
            

One

with patch, with h2, markup:

                                                                                        
            

One

So, no change. As expected.

As far as I understand, this addresses @xjm #21 asking to use a render array.

YesCT’s picture

Issue summary: View changes
FileSize
197.67 KB
188.05 KB

oh, and here is the setting to change to test.

xjm’s picture

xjm’s picture

Title: Remove SafeMarkup::set in template_preprocess_views_view_table() » Remove SafeMarkup::set() in template_preprocess_views_view_table()
xjm’s picture

Assigned: Unassigned » dawehner
Status: Reviewed & tested by the community » Needs work

Hm, so if we need the extra drupal_render() that won't help. The only time that should probably happen for now is as a workaround for #2505497: Support render arrays for drupal_set_message(). I was thinking more along the lines of "rewrite lots of the whole function and put it in a render array" or such.

I really feel like this whole function needs a refactor -- it is doing so much sanitization on its own and concatenating tags together in ways that don't seem correct, especially in a preprocess function. And it's very heavy for a preprocess. The SafeMarkup::set() are just the most obvious part of it.

I pinged dawehner for feedback on what else we can do here.

dawehner’s picture

Puh, so \template_preprocess_views_view_table() is basically just a different version of \template_preprocess_views_view_fields

Currently views_view_table() is such complex because we try to have a usable template itself, while ensuring the configurability,
so we could move more into the template but then this would mean more complexity there.

OT: My vision for those bits.
Instead of using configuration and then runtime code to change things in my perfect world, we would use the configuration in order to provide twig templates for themers
to start with. So by default the template is quite complex, but we could make it possible to have a template with the wrapper element types / css classes already built in ... but I fear this is just a vision, which would be way too difficult to implement.

xjm’s picture

Huh. Thanks @dawehner. So template_preprocess_views_view_fields() is indeed doing many of the same kinda-ishy things with HTML assembly, but without needing all the overuse of SafeMarkup. So this means one or more of three things:

  1. We can refactor this function to be more like template_preprocess_views_view_fields() to nix all the SafeMarkup overuse.
  2. template_preprocess_views_view_fields() has double-escaping bugs.
  3. template_preprocess_views_view_fields() is not as rigorously sanitized locally, but that may be okay because it is covered by Views admin permissions and/or Views is sanitizing things elsehwere.

If point 1 and/or 3 are true, then we could fix this function that way (not just the SafeMarkup::set() but all the SafeMarkup use), file a followup for @dawehner's ideal vision in #35, and add an @todo to the patch referencing that followup.

dawehner’s picture

I'm aware of #2363423: views-view-fields.html.twig gets escaped which makes the _fields() preprocess function way better to read, and fixes some double escaping issues.

xjm’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Postponed

#2363423: views-view-fields.html.twig gets escaped is lovely. Let's postpone this on that.

xjm’s picture

@Cottser clarified in #2363423: views-view-fields.html.twig gets escaped that theme functions must return strings, which explains why @akalata didn't see a way to use a render array without rendering immediately. I looked closely and the existing render array in the earlier patch is actually being passed to the link generator which (turns out) accepts a render array for the first argument (the fact that it's called $text nonwithstanding).

So once #2363423: views-view-fields.html.twig gets escaped is done, we can refactor this using that as a model.

xjm’s picture

stefan.r’s picture

Status: Postponed » Active
MauPalantir’s picture

Assigned: Unassigned » MauPalantir
joelpittet’s picture

@MauPalantir noticed there was a bunch of class concatenation done in template_preprocess_views_view_table(), I've opened a follow-up to clean that up. #2554957: Clean up CSS class concatenation in template_preprocess_views_view_table()

joelpittet’s picture

Status: Active » Needs review
Issue tags: +Needs manual testing
FileSize
4.85 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,370 pass(es), 290 fail(s), and 2 exception(s). View
4.94 KB

Still assigned to @Mau Palantir for review, was working with her on IRC to flesh out some things and though this may not work, I'm hoping it is in the right direction. I await her review!

This takes the markup build-up out of the preprocess and into the template which removes the need to escape/checkplain/set as safe, etc. Also avoids unnatural render arrays.

MauPalantir’s picture

The TBODY fields content fail to render when patched with #44.

MauPalantir’s picture

FileSize
32.81 KB
29.74 KB

Hmm, at second glance it seems to work, I think I forgot to clear cache between patches:D (I havent used drupal in a while, so takes time to get back to old routines)

btw it seems like the original implementation had broken the operations menu stuff too...

Status: Needs review » Needs work

The last submitted patch, 44: remove-2502089-44.patch, failed testing.

joelpittet’s picture

Oh that's the ticket! Classy! I knew duplicating templates would bite me in the ass... d'oh!

Currently in head the separator doesn't work at all, but this patch still has an issue where the other cells are not rendered after the separator. Likely because I've not flattened them and it expects to use the flattened result of the previous field.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.99 KB
8.61 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/views/views.theme.inc. View

This makes the links on the header and the sort indicator render correctly.

Status: Needs review » Needs work

The last submitted patch, 49: remove-2502089-49.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
8.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,606 pass(es), 1 fail(s), and 0 exception(s). View

Fixes my poor typos.

Status: Needs review » Needs work

The last submitted patch, 51: remove-2502089-50.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.75 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,614 pass(es), 1 fail(s), and 0 exception(s). View
1.49 KB

Last failure is due to the stuff I mentioned in #48.
This kinda fixes it but I'm not happy with the approach.

Status: Needs review » Needs work

The last submitted patch, 53: remove-2502089-53.patch, failed testing.

MauPalantir’s picture

The separator doesnt show up with patch in #53.

It seems like $column_reference['content'] remains empty even if I add multiple fields to the same column at the UI, thus separator doesnt get added to field_output. (line 585)

if (!empty($field_output) && !empty($column_reference['content']) && !empty($options['info'][$column]['separator'])) {
MauPalantir’s picture

The label double-escaping mentioned above in the thread is solved.

MauPalantir’s picture

I fixed the missing separator. I did not like the inconsistent naming of content vs field output, so I put field output + separator in 'content' as it was in the original implementation but as proper render arrays.

MauPalantir’s picture

MauPalantir’s picture

FileSize
9.34 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,866 pass(es), 3 fail(s), and 0 exception(s). View
3.9 KB

Umm... where is the patch? D.o thinks its there but not belonging to a comment. weird...

stefan.r’s picture

Just out of interest, what still needs to happen here, just manual testing and review? Do we still have the test failure or other todos in the patch?

stefan.r’s picture

Status: Needs work » Needs review

The last submitted patch, remove-2502089-57.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 59: remove-2502089-57.patch, failed testing.

The last submitted patch, 59: remove-2502089-57.patch, failed testing.

MauPalantir’s picture

FileSize
9.36 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,884 pass(es). View
764 bytes

Fix empty table messages (BTW on manual testing the empty message seems to be duplicated in HEAD…).

joelpittet’s picture

Status: Needs work » Needs review

Testbot engage

MauPalantir’s picture

@joelpittet, otherwise it only needs manual testing.

About the duplication I experienced: it seems that there is a line in views-view.twig that prints {{ empty }} separately, but all the views styles seem to implement printing empty message as the first row.

So either one should be cleared.

The twig output causing this issue was introduced in
https://www.drupal.org/node/2510794

joelpittet’s picture

Cool, maybe @stefan.r can give it a manual test to see if there is anything. Thanks for the solution. I'll also likely do a manual test as well a bit later.

stefan.r’s picture

  1. +++ b/core/modules/views/views.theme.inc
    @@ -521,7 +521,7 @@ function template_preprocess_views_view_table(&$variables) {
    +          $variables['header'][$field]['wrapper_element'] = $element_label_type;
    
    +++ b/core/modules/views/views.theme.inc
    @@ -578,27 +578,25 @@ function template_preprocess_views_view_table(&$variables) {
    +        $column_reference['wrapper_element'] = $fields[$field]->elementType(TRUE, TRUE);
    

    I assume this comes from the style settings, we are sure there is no way there will be a malicious tag in here?

  2. +++ b/core/modules/views/views.theme.inc
    @@ -578,27 +578,25 @@ function template_preprocess_views_view_table(&$variables) {
    +          if (!empty($column_reference['content']) && !empty($options['info'][$column]['separator'])) {
    +            $column_reference['content'][] = [
    +              'separator' => ['#markup' => $options['info'][$column]['separator']],
    +              'field_output' => ['#markup' => $field_output]
    +            ];
               }
    

    In my manual testing this code didn't trigger, how do we test this with content already in it?

stefan.r’s picture

FileSize
3.19 KB
9.59 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,933 pass(es). View

In manual testing the HTML output looked the same, we just missed the title attribute on the header links.

dawehner’s picture

In my manual testing this code didn't trigger, how do we test this with content already in it?

Did you configured the table style settings to include this separator? Note you need to render multiple fields in the same column for that.

stefan.r’s picture

@dawehner thanks. Just tested that with the patch applied and it worked great.

Wim Leers’s picture

@stefan.r: So does this need further manual testing?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Nice!

It always hurts me a bit if we don't share "code" between templates but instead just duplicate the block of template, but well, this is the world of template, in which things are different :)

stefan.r’s picture

@WimLeers I don't think it does, maybe @joelpittet could have a final look at this but could also be after commit as @dawehner seems to think this patch is ready now

joelpittet’s picture

re #70.1 It it should and will get escaped in twig unless view decided upstream to mark it as safe, but with our other meta this is unlikely. But also I think these options are chosen not written by the user and you need views UI admin to get at it, so lots of ways it won't cause problems.

joelpittet’s picture

@dawehner re #75 which "code" are you referring to sharing? I share the same sentiments towards classy duplicating all templates because it needs to add a couple CSS classes... but that's the way things went, waiting for history to decide on that one:P

joelpittet’s picture

+++ b/core/modules/views/views.theme.inc
@@ -491,7 +491,6 @@
         $link_options = array(
-          'attributes' => array('title' => $title),
           'query' => $query,

+++ b/core/themes/classy/templates/views/views-view-table.html.twig
@@ -75,14 +75,14 @@
-                <a href="{{ column.url }}">{{ column.content }}{{ column.sort_indicator }}</a>
+                <a href="{{ column.url }}" title="{{ column.title }}">{{ column.content }}{{ column.sort_indicator }}</a>

Thanks for catching that bit @stefan.r, I missed it!

Could possibly create a column.link_attributes variable. But I think this works well too.

stefan.r’s picture

#77 the reason I asked is when I hardcoded $variables['header'][$field]['wrapper_element'] = 'script'; it did end up the in HTML...

joelpittet’s picture

Totally right to be concerned about that. Though my other points should cover the concern. The plugin would have to provide these values to the user so I think we are safe. Also our Xss::filter would need the <> to strip it out anyway.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

It's great that we're using Twig to solve this one. Nice work everyone. Committed 65d1748 and pushed to 8.0.x. Thanks!

  • alexpott committed 65d1748 on 8.0.x
    Issue #2502089 by joelpittet, MauPalantir, akalata, stefan.r: Remove...

Status: Fixed » Closed (fixed)

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