Problem/Motivation

At some point of the SafeMarkup refactoring we broke the custom text fields in Views so that they are unable to output markup without getting escaping problems.

Proposed resolution

Field value should be XSS filtered and marked as safe afterwards.

Remaining tasks

  • Write patch (DONE by laurii)
  • Review patch (done by joel.pittet)
  • Manual testing (done by ibullock)
  • RC Evaluation (done by bigjim)

User interface changes

API changes

Data model changes

Why this should be an RC target

This is an important feature for basic site building. The use of custom markup is likely needed on a high % of new Drupal 8 sites.
Possible disruptions to current Drupal 8 sites that worked around this issue using escaping, though seems a relatively minor, easily fixed issue.

Before

After


Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

lauriii’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
829 bytes

Status: Needs review » Needs work

The last submitted patch, 2: outputting_markup_from-2579427-2.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
833 bytes

Last one wont fly

The last submitted patch, 2: outputting_markup_from-2579427-2.patch, failed testing.

mbaynton’s picture

Fixes the issue for me. Does this warrant a test?

LoMo’s picture

@mbaynton It has the "Needs tests" issue tag and I would say that the need is warranted (to help prevent regressions)

lauriii’s picture

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/field/Custom.php
@@ -63,7 +65,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    */
...
-    return $this->options['alter']['text'];
+    return Markup::create(Xss::filterAdmin($this->options['alter']['text']));

This should likely use ViewsRenderPipelineMarkup as it's in the pipeline so we can find these later when/if we find a better solution.

The last submitted patch, 8: outputting_markup_from-2579427-8-test-only.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
3.77 KB

How about this one?

joelpittet’s picture

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

Well it is rendering so it would be nice if it was a value object... but there is precedent to it like this with a ['#markup' => 'render array']

A couple of examples that use #markup already.
\Drupal\views\Plugin\views\field\Dropbutton
\Drupal\views\Plugin\views\field\LinkBase

If the tests fail, blame Lauri:P

joelpittet’s picture

Issue tags: +rc target triage

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: outputting_markup_from-2579427-11.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.03 KB

The last submitted patch, 8: outputting_markup_from-2579427-8-test-only.patch, failed testing.

The last submitted patch, 11: outputting_markup_from-2579427-11.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 15: outputting_markup_from-2579427-15.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.03 KB
lauriii’s picture

FileSize
584 bytes
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This was the approach I was thinking of from the start. It marks the ViewsRenderPipelineness for later review.

The last submitted patch, 8: outputting_markup_from-2579427-8-test-only.patch, failed testing.

The last submitted patch, 11: outputting_markup_from-2579427-11.patch, failed testing.

The last submitted patch, 15: outputting_markup_from-2579427-15.patch, failed testing.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary.

bigjim’s picture

Issue summary: View changes
Issue tags: +rc eligible

Evaluated the issue for RC eligibility, felt it should be :D.

Made changes @xjm noted above to issue summary

ibullock’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update
FileSize
220.16 KB
228.51 KB
158.47 KB

I tested the patch manually and confirmed the issue is resolved by the patch. Added screenshots to the issue summary.

ibullock’s picture

Added a retest for php5.5_mysql5.5

mradcliffe’s picture

Issue summary: View changes
Issue tags: -rc eligible

I don't think this is rc eligible, but it should definitely be a candidate for rc target because it is a bug fix and not a coding standard, doc update, testing improvement or library update as written in https://www.drupal.org/core/d8-allowed-changes#rc.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -rc target triage +rc target

Agreed. One-line bug fix (+tests), fixing a regression that was introduced just before RC. Moving to RC Target and asking for forgiveness. ;)

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 1497905 on 8.0.x
    Issue #2579427 by lauriii, ibullock, joelpittet, bigjim: Outputting...

Status: Fixed » Closed (fixed)

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

FiNeX’s picture

Hi, I've just discovered that even inline CSS styling is stripped from the global custom text field inside views. Is it a desidered behaviour?

lauriii’s picture

We run that field through Xss:filterAdmin() which removes everything related to styles and javascript so it is expected behavior.

FiNeX’s picture

Ok, thanks :-)