Remove or document SafeMarkup::set in theme()

Thanks @Daniel_Rose!

1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -312,6 +312,8 @@ protected function theme($hook, $variables = array()) {
   +    // It's ok to use SafeMarkup because it's being used for internal use
   +    // and content is already presumed to be sanitized.
   

   Maybe this could be something along the lines of:

   "Theme functions do not go through Twig so the output is not autoescaped. Theme function output is always expected to be 100% safe."

2. +++ b/example.gitignore
   @@ -34,3 +34,6 @@ sites/simpletest
   +
   +# PHPStorm
   +# .idea
   

   Undo these changes :)

Looks ready to go.

-mike

I think the comment should live right above the SafeMarkup::set(), like the other issues have done.

The patch itself looks fine, though I have a slight issue with the wording of the comment -- I'd prefer the "is presumed to be safe" over the "is always expected to be safe", unless there is a test somewhere that confirms the expectation? Are there internal requirements that any theme function is guaranteed to be safe, or are we only presuming that this will be the case?

Just thinking in terms of the precision of uncertainty -- will keep as Needs Review and ping a theme system expert to review.

@akalata very good point I would tend to agree the wording could be more precise.

I've updated our language a bit, which hopefully more clearly indicates the responsibility of the writer of the theme function (since the ones left in core are few and far between). I'm not sure if this is a documented standard or best practice, but it probably should be (even if my proposed standard is completely wrong). Quoting dawehner in IRC: "we output the string here, so we should sanitize it here".

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -315,6 +315,9 @@ protected function theme($hook, $variables = array()) {
+        // is not auto-escaped. However, we can only presume that the theme

I believe we use autoescape as one work, not with a hyphen. Otherwise, looks fine.

"theme engine" is more generic. Theme functions would not render using phptemplate either.

Thanks folks for the patch! I think the current wording is good. However, I actually feel like we should be trying to remove this call.

@alexpott suggested postponing this on #2506581: Remove SafeMarkup::set() from Renderer::doRender, which is the only place that uses theme() directly. So doing that now.

Unpostoponing

So yeah I think the proper way is to return a safe string. This also removes a bit of the memory overhead we do have.

Removed SafeMarkup::set(), and added additional explanation:

Therefore, the theme function itself is responsible for using
SafeMarkup::set() as necessary, not this function, i.e.: render().

@davidhernandez (yelling at someone else):

You better have an interdiff!

We are going to fix this in #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender, so I am marking as duplicate.