[meta] Remove every SafeMarkup::set() call

That seems reasonable, this issue then just has to do a sweep of all existing calls when either that issue is resolved or the critical count overall is much lower.

So what is actually critical is to either document or remove every call. Let's recategorize this as a meta issue, working first through sub-issue #2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible.

Tagging for https://drupaltwig.org.

So, as @dawehner and others have pointed out, it's weird to have a critical postponed on a major, and it's resulting in the issues not getting the focus they need. Therefore, I'm merging the two metas and unpostponing this.

Child issues of this should be filed as major, unless they include something that makes them critical on their own, like #2273925: Ensure #markup is XSS escaped in Renderer::doRender().

Started updating the summary to the new scope and some more specific action items.

#2273925: Ensure #markup is XSS escaped in Renderer::doRender() is the place to help right now. There's 3-4 questions on it that I had that could use feedback, and then it needs someone to do a final code review to potentially re-RTBC.

For now, I'd suggest doing any research for the rest of the issues with the patch from that issue applied.

Adding the remaining calls to a list in the summary.

Existing issues related to each call.

The latest patch in #2296929: Remove system_requirements() SafeMarkup::set() use is interesting and worth review and discussion for how we proceed with other issues as well.

https://www.drupal.org/node/2311123 lists inline_template as option 2 and SafeMarkup::format() as option 4, but IMO, we should reverse that priority and favor SafeMarkup::format() over inline_template. There are some benefits to inline_template over SafeMarkup::format(), such as ability for alter hooks to manipulate the variables and the ability to use Twig control structures, but IMO, use cases where those benefits matter should probably be promoted to full Twig file templates (which is, and should remain, option 1).

Any thoughts from folks here on that before we update that CR?

IMHO inline template should be used for cases, where things really actually belong into a template, so complex cases, with a lot of HTML inside

We should be removing first, documenting if we can't.

At the sprint we are going to work on the call in drupal_check_module() in core/includes/install.inc, I'm claiming it here because I want to show creating the issue as well.

Added "Common Issue Patterns" section to issue description.

Adding a few that use the implode pattern but don't have issues yet.

Related to pattern #4: #2501975: Determine how to update code that currently joins strings in SafeMarkup::set()

Here is the workflow used during NHDevDay #2 to tackle the list of child issues in this ticket.

Adding #2502095: Remove SafeMarkup::set in template_preprocess_views_ui_view_info()

Tentatively testing our new "Plan" issue type now that #2504039: Adjust critical issue counter/link at /drupal-8.0/get-involved to account for "Plan" issue category is in.

Just a note that #2502095: Remove SafeMarkup::set in template_preprocess_views_ui_view_info() is an excellent example of addressing the issue with best practices for themeable output. :) Go team!

Added security documentation draft to the the issue summary as a google doc, feel free to edit it there.

https://www.drupal.org/node/2280965#docs

I merged two of the issues; updating the summary appropriately.

#2501949: Use the ternary operator for $description_build in Drupal\node\Plugin\views\row\RSS::render() was fixed in another patch and has changed into a minor cleanup.

Nice to see a growing number of fixed issues in the currently-48-item list that's in the summary. At some point, it'll probably be helpful to split the list into two (i.e., "Remaining" and "Done") for easier scanning of what's not done yet. But I leave it to you all to decide on when the right time for that is.

Shuffled

Moar shuffle.

Shuffling down.

Marking this as one of the better D8 criticals in terms of issue summary, docs, etc. for people to work on.

#2501701: Remove SafeMarkup::set in template_preprocess_color_scheme_form() is fixed

Could someone give me a hint why http://cgit.drupalcode.org/google_analytics/tree/src/Form/GoogleAnalytic... shows now <div class="description"> and how I should fix it now, please?

@hass Wrap it in a Safemarkup::format() to not have the combined string escaped.

#2502009: Remove SafeMarkup::set in SearchExtraTypeSearch::execute() is fixed!

#2539300: Remove SafeMarkup::set in \Drupal\Tests\Core\Form\FormCacheTest::testSetCacheWithSafeStrings()

Moving

core/lib/Drupal/Core/Render/Renderer.php

• #2273925: Ensure #markup is XSS escaped in Renderer::doRender()
• #2506581: Remove SafeMarkup::set() from Renderer::doRender

to Done.

taking out the location
core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php

• #2501319: Remove SafeMarkup::set() in Error::renderExceptionSafe() and _drupal_log_error() and improve backtrace formatting consistency in _drupal_log_error(), Error::renderExceptionSafe(), and DefaultExceptionSubscriber::onHtml ()

from the list since there is no call to set in there.

(the other locations are still listed)

@YesCT, @alexpott, @effulgentsia, @joelpittet, @cilefen, @lauriii, @davidhernandez, @joelpittet, and I discussed all the outstanding children of this meta today. Notes from the discussion are here:
https://docs.google.com/document/d/1wBSwpCa0tm_CKAV1_R0xTAdKZSsVSUc2fQuS...

woohoo one more issue is closed #2502781: Remove SafeMarkup::set in template_preprocess_file_widget_multiple()

core/modules/rest/src/Plugin/views/display/RestExport.php also contains SafeMarkup::set() call, shouldn't it be added here ?

@harjotsingh yes please, though it's documented it may be nice to have it here as well. It's referencing #2501313: Discuss how to support non-HTML output in the render system

core/modules/user/src/Tests/UserBlocksTest.php
core/modules/shortcut/src/Tests/ShortcutSetsTest.php
core/modules/system/src/Tests/Form/ValidationTest.php
core/modules/filter/src/Element/ProcessedText.php
core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php
core/lib/Drupal/Core/Form/FormErrorHandler.php
All these contains SafeMarkup::set() call but aren't added over here.

Adding known blockers to the issue summary.

#2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString has introduced the possibility module specific versions of SafeString for the rare instance when a module injects itself into the render pipeline and needs to communicate safeness back to the render pipeline. That patch introduces ViewsRenderPipeLineSafeString for Views and #2547741: Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module introduce FilteredString for the Filter module. This offers new ways to remove SafeMarkup::set()'s in things like View's RestExport plugin.

updating blocker list

I have created all issues for #115.

I did not find calls in:
core/modules/filter/src/Element/ProcessedText.php
core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php

Added following children:
#2550985: Remove SafeMarkup::set in _batch_test_finished_helper()
#2550991: Remove or document SafeMarkup::set in ViewListBuilder::buildRow()

We should actually be able to remove SafeMarkup::set(). I think this issue should be retitled. Adding a new issue to deal with AllowedTagsXssTrait.

Haven't cross referenced this with the issue list, but here is the current usage report from PhpStorm for `git rev-parse --short HEAD` == 'e415ad4'.

Moved Fixed items in IS to done

The following issues are now fixed:

1. core/modules/shortcut/src/Tests/ShortcutSetsTest.php
   • #2550963: Remove or document SafeMarkup::set in ShortcutSetsTest::testShortcutSetSwitchNoSetName
2. core/modules/rdf/rdf.module
   • #2501697: Remove SafeMarkup::set in rdf_preprocess_comment()

Moving blocked below to help see the colors better.

Suffle the fixed and unblocked

$link_options = array(
    'attributes' => array('class' => array($link_class)),
    'query' => $params,
);
$image = '<img src="/images/arrow.png" title="down" >';
//$link_image = \Drupal::l(SafeMarkup::set($image), new Url('<current>', [], $link_options));
$link_image = \Drupal::l($image, new Url('<current>', [], $link_options));
debug($link_image);

How to display a image link without SafeMarkup::set ?

@realdream so at least the title "down" need to be translatable. But also adding an image such as an arrow to a link should be done with CSS.

I propose that we re-title this issue to be "[meta] Remove every SafeMarkup::set() call" and add #2554889: Remove SafeMarkup::set() from the codebase as the final patch to commit.

All issues unblocked :) Down to a single list of issues to tackle!

Retitling.

#2555825: Remove SafeMarkup::set call from TranslationManager::translate

Updating issue credit to include everyone who has commented.

@alexpott and I agreed to move #2554889: Remove SafeMarkup::set() from the codebase to the other meta on account of the fact that it includes an API break.

I assume it is time for one last search of the code base?

Here is a usage report from PhpStorm against HEAD == f82e0a3.

Looks like the single outstanding "usage" is covered by #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages(). Can we move the critical to it and close this meta? #2554889: Remove SafeMarkup::set() from the codebase is open to remove the functionality of set, so I can't think of any reason to keep this open. With that in mind, I'm marking this RTBC.

I think #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages() was just finished.

4 sprints, 444 days, and ohsomany discussions and debates and forays into Drupal's dark corners later.... this issue is fixed!

Every SafeMarkup::set() call in core is gone, aside from those in the class itself and its tests. (Plus a couple of dangling comment references that should also be removed in #2554889: Remove SafeMarkup::set() from the codebase).

Well done all!

AMAZING work, all!! :D

Woohoo! Congrats to everyone who poured so much time and energy into this! The Drupal community owes you. :-)

:D

Awesome news!!!

Good work, everyone. Thanks, in particular, stefan.r for working really hard the past couple weeks to finish this.

Good work, everyone. Thanks, in particular, stefan.r for working really hard the past couple weeks to finish this.

+1 — thanks, Stefan!

A lot of the movement on this really got underway at a D8 Accelerate funded code sprint!