Child of #2549943: [plan] Remove as much of the SafeMarkup class's methods as possible and blocked by #2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString and (i think) we'll need the Html::encodeEntitiesIfUnsafe() from #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter

Remove SafeMarkup::replace() and in views_pre_render_views_form_views_form() use the new ViewsRenderPipelineSafeString() instead.

CommentFileSizeAuthor
#7 2550055.7.patch9.54 KBalexpott
#6 2550055-6.patch1.03 KBgeertvd
#5 2550055-5.patch1.11 KBgeertvd

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Task » Bug report

So the current use of SafeMarkup::replace() in views_pre_render_views_form_views_form() is extremely problematic. If any implementation of hook_views_form_substitutions() returned an unsafe string in either the placeholder or substitution the output of SakeMarkup::replace would be marked unsafe regardless of whether the replacement has actually taken place.

This makes this issue a bug.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Postponed » Active
geertvd’s picture

geertvd commented
Status: Active » Needs review
StatusFileSize
new 2550055-5.patch1.11 KB
geertvd’s picture

geertvd commented
StatusFileSize
new 2550055-6.patch1.03 KB

Removed that unnecessary enter

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2550055.7.patch9.54 KB

Patch attached removes SafeMarkup::replace() and adds test coverage of the interaction between hook_views_form_substitutions() and views_pre_render_views_form_views_form().

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community 
  1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -268,51 +268,4 @@ public static function placeholder($text) {
-  public static function replace($search, $replace, $subject) {

    \o/ \o/ \o/ 

  2. +++ b/core/modules/views/views.module
@@ -663,12 +664,17 @@ function views_pre_render_views_form_views_form($element) {
+    // Ensure that any replacements made a safe to make.

    s/a/are/ — can be fixed on commit.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/tests/modules/views_test_data/views_test_data.views_execution.inc
@@ -19,8 +19,13 @@ function views_test_data_views_query_substitutions(ViewExecutable $view) {
-function views_test_data_views_form_substitutions(ViewExecutable $view) {
+function views_test_data_views_form_substitutions() {

Just checked, we don't pass the view: core/modules/views/views.module:664

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Fixed the typo on commit.

Committed/pushed to 8.0.x, thanks!

  • catch committed 8cf5b80 on 8.0.x 
    Issue #2550055 by geertvd, alexpott: Remove SafeMarkup::replace() and...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Updated https://www.drupal.org/node/2549395

Status: Fixed » Closed (fixed)

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