Remove SafeMarkup::set() in drupal_render_children() and replace it with SafeString

This documentation seems correct.

Thanks @Cottser. So looking at this comment, I think we should be a little more explicit about the purpose of the call that render() is not magically making everything sanitized -- saying it is "safe" is a little unspecific. Similarly to #2273925-269: Ensure #markup is XSS escaped in Renderer::doRender() and... whatever other issue it was I bumped back yesterday for the same thing. #2501403: Document SafeMarkup::set in Xss::filter. Kind of blurring together. :) We might want a longish comment on what the render system has done up until that point and how it interacts with the... thingy. Twig autoescaping.

Here's the comment from the doRender() issue to save that page load:

+    // \Drupal\filter\Plugin\FilterInterface. $text is not guaranteed to be
+    // safe, but it has been passed through the filter system and checked with
+    // a text format, so it must be printed as is. (See the note about security
+    // in the method documentation above.)

Here's a try for updated documentation, though I'm not sure it's exactly what you're looking for. I'm thinking we don't need to provide details on the render system since we've documented the specific class that should be handling the checking of markup (maybe?).

Also good with that/can't think of anything better to describe it. Thanks @akalata

So, I started to post a slightly tweaked text for this. Then, I was going to suggest it be merged with the patch for docs in Renderer::doRender() so that we could explain it consistently.

Then I realized that this function (unlike doRender()) is not just marking XSS-filtered #markup as safe -- it's marking the entire render array output as safe and returning it, which means (as far as I know) that any unsafe strings in the render array will never get sanitized by Twig either.

What's more, it actually doesn't appear to be a part of the main render API any more. It's only called in four places and it sort of circumvents the rest of the render system actually. I think this function should be replaced entirely in the code that calls it, and deprecated.

I pinged @Wim Leers for feedback on the status of drupal_render_children().

Killing drupal_render_children() would be splendid. It is a very, very confusing function anyway.

I'd like input from somebody with very deep Theme system knowledge to make sure that that is actually okay though. Assigning to Joël.

Thanks Wim!

I wish assignments notified me somehow:) I just happened to be following this one.

I think we can kill it's use in core almost entirely if not totally. But not sure we can do that in contrib, that I'd definitely need a check on grepping all contrib.

It's not as confusing as drupal_render() which needs some factoring love ;-)
It loops through the element children, renders each child element and concatenates them. Which is actually also done in drupal_render/Renderer::doRender:
/lib/Drupal/Core/Render/Renderer.php:391

    if ((!$theme_is_implemented || isset($elements['#render_children'])) && empty($elements['#children'])) {
      foreach ($children as $key) {
        $elements['#children'] .= $this->doRender($elements[$key]);
      }
      $elements['#children'] = SafeMarkup::set($elements['#children']);
    }

The only difference is that the one in doRender is sorted, and the one in drupal_render_children() is not sorted and can take arbitrary keys.

Currently it's used in the following places:

1. core/modules/system/system.admin.inc:355
2. core/modules/toolbar/src/Element/Toolbar.php:101
3. core/modules/views/views.module:695
4. core/modules/views_ui/views_ui.theme.inc:175

Maybe we need a compliment to |without() to be |with() to get this feature in twig?
Currently haven't had any reason to use this feature in core.

system.admin.inc and core/modules/views_ui/views_ui.theme.inc are on the theme function chopping block.
#1938930: Convert theme_system_modules_details() to #type table
#1963978: Convert theme_views_ui_build_group_filter_form() to Twig

The views.module one may not even need that extra call at all, but worth testing that theory out. Same with preRenderToolbar. Those two are the wildcards.

So +1 to create a follow-up to deprecate that function and find all it's use in contrib.

Great, so here's what I'd recommend:

1. A major meta a.k.a. plan to deprecate drupal_render_children() and remove its core usages. Document in the beta eval that it's a blocker for part of the SafeMarkup critical.
2. Make the two issues @Cottser linked part of the meta. (Though the table one sounds scary; if that takes too long we might want to look at an interim fix.)
3. Child issues to remove the other two core uses and replace them with whatever.
4. A child issue to mark drupal_render_children() as deprecated in 8.0.x and to be removed before 9.0.0, with what to do instead and strong warnings about the downsides of using it.
5. Postpone this issue on the above, and change the comment to also include dire warnings about the deprecation.

Created the follow-up and will work on that: #2544156: Deprecate drupal_render_children(). drupal_render_children() has the only SafeMarkup::set() call in common.inc so postponing this on the child issue.

Clarifying what this is postponed on in the issue summary.

Let's swap the SafeMarkup::set() for a SafeString::create() - it'll work fine.

In discussion with @xjm, @joelpittet, @effulgentsia, @cilefen, @lauriii, @davidhernandez, @yesct and @pwolanin it was concluded that regardless of the outcome of #2544156: Deprecate drupal_render_children() we should do the replacement of SafeMarkup::set() with SafeString::create() as this is consistent with Renderer::render().

RTBC if this passes.

Tested the few places where render_children is used manually and it seemed to work.

RTBC++

Committed/pushed to 8.0.x, thanks!