Replace SafeMarkup::format() in template_preprocess_html with placeholders in the template

What is wrong to use SafeString::create() - more effort?

Otherwise +1, SafeString did not exist at that time of writing this.

Okay, agree.

+++ b/core/lib/Drupal/Core/Render/SafeString.php
@@ -36,6 +36,19 @@ class SafeString implements SafeStringInterface, \Countable {
   /**
+   * Constructs a new SafeString instance.
+   *
+   * Better use SafeString::create() as the string might not be an actual
+   * string.
+   *
+   * @param string $string
+   *  (optional) The string.
+   */
+  public function __construct($string = '') {
+    $this->string = $string;
+  }

I'm not yay about adding a constructor but at the very least we should enforce when it should be used. An exception should be thrown if $string is not a string and when it is empty. Also the docs need improving to say when to use the constructor and when to use create.

This looks good now.

+++ b/core/includes/theme.inc
@@ -1290,10 +1291,9 @@ function template_preprocess_html(&$variables) {
+    // We can construct a SafeString as we know that both $type and $token is

Nit: are

I think we need to decide on #2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString first.

Let's have @alexpott have another look at this

reroll

I think a completely different and maybe more honest approach is available. Let's just print the raw variables in the twig template and attach everything to the html render. 4 less calls to SafeMarkup::format and 4 less calls to render.

Are we sure the data we receive in the template comes from template_preprocess_html(), will this still be OK if a theme has its own preprocess implementation?

We can even do this which makes it way more obvious what is going on in the template.

That seems clearer, so realistically themes would never touch that anyway

Fixing the template docs. I still think we should print the raw token - if a developer changes the token and adds raw html imo that is just silly and we shouldn't use auto-escape to prevent that. What auto-escape should be for is ensuring user generated input is escaped when printed in templates.

Wow, this sure is a very interesting approach!

I have a few concerns about going in this direction:

1. it makes the template look super scary
2. it makes it impossible to add other things to those variables.

Yet both of these concerns are arguably good things: both help ensure themers don't accidentally break CSS or JS loading. In that sense, it even clarifies things: because {{ styles }} and {{ scripts }} and {{ scripts_bottom }} make themers think they are printing CSS/JS, but they're not: they're printing placeholders.

But in that case, I'd propose some renaming to make it at least less cryptic:

+++ b/core/modules/system/templates/html.html.twig
@@ -33,10 +28,10 @@
+    <drupal-html-response-attachment-placeholder type="head" token="{{ token|raw }}"></drupal-html-response-attachment-placeholder>
...
+    <drupal-html-response-attachment-placeholder type="styles" token="{{ token|raw }}"></drupal-html-response-attachment-placeholder>
+    <drupal-html-response-attachment-placeholder type="scripts" token="{{ token|raw }}"></drupal-html-response-attachment-placeholder>

@@ -45,6 +40,6 @@
+    <drupal-html-response-attachment-placeholder type="scripts_bottom" token="{{ token|raw }}"></drupal-html-response-attachment-placeholder>

<css-attachments-placeholder token="{{ attachments_token|raw }}">

So: "head", "css", "js" and "js-bottom" is what I would propose.

And renaming the "token" variable to "attachments_token".

After discussing with @Wim Leers a bit more on IRC we went for the following approach...

Looks good! Let's see what testbot says.

One important remark: every D8 theme that has its own html.html.twig template will need to be updated.

Fancy! No objections from me. Just want to confirm this docs removal is because something is being removed (or is just out of date/wrong/etc.):

+++ b/core/modules/system/templates/html.html.twig
@@ -7,9 +7,6 @@
- * - css: A list of CSS files for the current page.

Great! With front-end/themer sign-off, I think this is RTBC.

#20: That css variable is indeed out of date/wrong: $variables['css'] is not being set anywhere.

Committed/pushed to 8.0.x, thanks!

Posthumous priority change.

Added https://www.drupal.org/node/2565021

Filed a follow-up: #2565033: Follow-up for #2538950: simplify syntax.

Initially this worked in my theme, however after a recent update (no idea which one) all I get now is this:

<head-placeholder token="GyB5DIbKTxnBAR1qiGzevTSOCsi4EQ9dLK3mMCYAANc6HDRx1EA4MMiqsw7Y9_f7KKFOo8Xa5A" />

...so no styles, js etc is output, only what you see above. Anyone have an insight into why this might be?

Ok, so this is patently obvious now I have looked at this again - for some reason I have treated these like self closing html tags and used a forward slash, this caused them to fail - they should look like this:

<head-placeholder token="{{ placeholder_token|raw }}">

Not...

<head-placeholder token="{{ placeholder_token|raw }}" />

edit - I updated this in case any one else made the same silly mistake. Apologies for the noise.

+++ b/core/modules/system/templates/html.html.twig
@@ -33,10 +28,10 @@
+    <head-placeholder token="{{ placeholder_token|raw }}" />
...
+    <css-placeholder token="{{ placeholder_token|raw }}" />
+    <js-placeholder token="{{ placeholder_token|raw }}" />

@@ -45,6 +40,6 @@
+    <js-bottom-placeholder token="{{ placeholder_token|raw }}" />

+++ b/core/themes/classy/templates/layout/html.html.twig
@@ -39,10 +34,10 @@
+    <head-placeholder token="{{ placeholder_token|raw }}" />
...
+    <css-placeholder token="{{ placeholder_token|raw }}" />
+    <js-placeholder token="{{ placeholder_token|raw }}" />

@@ -51,6 +46,6 @@
+    <js-bottom-placeholder token="{{ placeholder_token|raw }}" />

We try to avoid |raw to discourage it's use. Unfortunately this got in to RC release. Any chance we can wrap this placeholder in a Markup/SafeString type object?

I just noticed this when trying to get basic theme upgraded to D8

Made a follow-up to get those out. #2603074: Remove |raw from use in core templates