Remove SafeMarkup::placeholder(), deprecate drupal_placeholder() and stop drupal_placeholder() from marking safe.

Just nits, looks lovely.

1. +++ b/core/includes/bootstrap.inc
   @@ -950,10 +951,13 @@ function drupal_static_reset($name = NULL) {
   + *   therefore has few valid use-cases. Note also, that this method does mark
   

   'does not' isn't it?

   Are there any valid use cases?

2. +++ b/core/modules/editor/editor.module
   @@ -78,7 +78,7 @@ function editor_form_filter_admin_overview_alter(&$form, FormStateInterface $for
   -    $editor_name = ($editor && isset($editors[$editor->getEditor()])) ? $editors[$editor->getEditor()]['label'] : drupal_placeholder('—');
   

   So just no <em> tags here?

+++ b/core/modules/filter/src/FilterPermissions.php
@@ -60,7 +60,11 @@ public function permissions() {
+            '#prefix' => '<em>',
+            '#markup' => $this->t('Warning: This permission may have security implications depending on how the text format is configured.'),
+            '#suffix' => '</em>'

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -205,7 +205,11 @@ public function revisionOverview(NodeInterface $node) {
+            'prefix' => '<em>',
+            '#markup' => $this->t('current revision'),
+            'suffix' => '</em>',

+++ b/core/modules/update/update.report.inc
@@ -242,7 +243,7 @@ function template_preprocess_update_project_status(&$variables) {
-      $extra_item['data'] = drupal_placeholder($value['data']);
+      $extra_item['data'] = ['prefix' => '<em>', '#markup' => $value['data'], '#prefix' => '</em>'];

Did the 'class="placeholder"' added better accessibility?

• +++ b/core/modules/update/update.report.inc
  @@ -266,13 +267,10 @@ function template_preprocess_update_project_status(&$variables) {
  +        $base_themes[] = $base_theme . " ($base_status_label)";
  

  Does $base_theme lose the emphasis here?

• * - %variable: Escaped to HTML and formatted using self::placeholder()

  Not anymore :)

Sadly #601548: Loosen the dependency between t() and the theming layer never had a justification for the class so yeah I think its totally fine to remove it.

Sadly #601548: Loosen the dependency between t() and the theming layer never had a justification for the class so yeah I think its totally fine to remove it.

I think the justification is at #601548-7: Loosen the dependency between t() and the theming layer. But the cases that this patch removes it from look like they are probably all OK without it (seems good as long as it stays for actual translation placeholders).

+tr.fallback_format td {
+  font-style: italic;
+}

This doesn't look right since it's causing the whole row to be italicized (including things like the "Configure" button).

Also the addition of the fallback_format class is causing the drag and drop handler to disappear from that row. No idea why...

Also the addition of the fallback_format class is causing the drag and drop handler to disappear from that row. No idea why...

Oh I see, I think it's because FilterFormatListBuilder::buildRow() does this at the bottom:

    return $row + parent::buildRow($entity);

That is not going to merge things like $row['#attributes']['class'] correctly, so it winds up without the "draggable" class.

1. +++ b/core/modules/editor/editor.module
   @@ -78,7 +78,7 @@ function editor_form_filter_admin_overview_alter(&$form, FormStateInterface $for
   -    $editor_name = ($editor && isset($editors[$editor->getEditor()])) ? $editors[$editor->getEditor()]['label'] : drupal_placeholder('—');
   +    $editor_name = ($editor && isset($editors[$editor->getEditor()])) ? $editors[$editor->getEditor()]['label'] : '—';
   

   +1 lol, I'm sure that made a difference in Arial or most sans-serifs.

2. +++ b/core/modules/filter/src/FilterFormatListBuilder.php
   @@ -94,24 +95,24 @@ public function buildHeader() {
   -      $row['label'] = SafeMarkup::placeholder($entity->label());
   

   Label looks as though it needs a wrapper attribute class too.

3. +++ b/core/modules/update/templates/update-project-status.html.twig
   @@ -109,7 +109,7 @@
   -      Depends on: {{ basethemes }}
   +      Depends on: {{ basethemes|placeholder }}
   
   +++ b/core/modules/update/update.report.inc
   @@ -266,13 +267,10 @@ function template_preprocess_update_project_status(&$variables) {
   -        $base_themes[] = t('%base_theme (!base_label)', array(
   ...
   -          '!base_label' => $base_status_label,
   

   This will wrap em around both the label and the base theme. So the end result won't be the same.

4. +++ b/core/modules/update/update.report.inc
   @@ -5,6 +5,7 @@
   +use Drupal\Component\Utility\Html;
   
   @@ -242,7 +243,7 @@ function template_preprocess_update_project_status(&$variables) {
   +      $extra_item['data'] = ['#prefix' => '<em>', '#markup' => $value['data'], '#prefix' => '</em>'];
   

   Should the data still be escaped? or the new use statement not needed?

5. +++ b/core/modules/user/src/Tests/UserValidationTest.php
   @@ -136,7 +136,7 @@ function testValidation() {
   -    $this->assertEqual($violations[0]->getMessage(), t('!name field is required.', array('!name' => SafeMarkup::placeholder($user->getFieldDefinition('mail')->getLabel()))));
   +    $this->assertEqual($violations[0]->getMessage(), t('%name field is required.', array('%name' => $user->getFieldDefinition('mail')->getLabel())));
   

   +1 Totally!

@alexpott #18.3 is silly how? it make a visual distinction between the two elements to me, there could be a better way to do that, but I don't think it's silly. Maybe those em tags could be moved into the template in a foreach loop instead of the join we are currently doing.

Ok let's get some screenshots on the bits that have changed markup:

1. +++ b/core/modules/filter/src/FilterFormatListBuilder.php
   @@ -94,24 +95,24 @@ public function buildHeader() {
   +    $row['label'] = $this->getLabel($entity);
   ...
   -      $row['label'] = SafeMarkup::placeholder($entity->label());
   

   Filter Format List

2. +++ b/core/modules/update/templates/update-project-status.html.twig
   @@ -109,7 +109,7 @@
        {% set basethemes = base_themes|join(', ') %}
   ...
   -      Depends on: {{ basethemes }}
   +      Depends on: {{ basethemes|placeholder }}
   

   Project status

And keep in mind that <em> may not be represented the same as <em class="placeholder"> as it has a purpose and should be taken into careful consideration if it's being removed, as it may be purposefully visually strengthened by it's CSS class.

I completely agree regarding the Filter Admin thank you for the screenshots on the Filter admin, those really help.

Also, confirm there is nothing in this patch that is affecting the .placeholder CSS

Still a bit unclear and not sure where the irreverence around the basethemes is found. It's not popping out of the patch for me as clear as it is to you.

Looks great! Found only one problem:

+++ b/core/modules/filter/src/FilterFormatListBuilder.php
@@ -140,6 +141,7 @@ public function getDefaultOperations(EntityInterface $entity) {
+    $form['#attached']['library'][] = 'filter/drupal.filter.admin';

This seems wrong? That library is solely for filter settings, not for a listing of formats.

Removed the Needs screenshots tag, #24 added screenshots.

Do we want to block this on #2338167: Update ProjectInfo class to reflect changes to extension system or not?

1. +++ b/core/modules/update/update.report.inc
   @@ -228,7 +228,7 @@ function template_preprocess_update_project_status(&$variables) {
   +      $extra_item['data'] = ['#prefix' => '<em>', '#markup' => $value['data'], '#prefix' => '</em>'];
   

   Contrary to what #33.2 says, this still appears to be here?

2. +++ b/core/includes/bootstrap.inc
   @@ -949,10 +949,11 @@ function drupal_static_reset($name = NULL) {
   + *   templates instead. Note this method should not be used to only emphasise a
   

   s/emphasise/emphasize/

   (I know, I know, British English is more elegant, but alas.)

NW for point 1.

drupal_placeholder() was going to be deprecated by #2221771: Mark all simple wrappers in bootstrap.inc and common.inc as deprecated.
As there is a better comment in this patch and as deprecating that function here makes that issue more consistent, I asked to remove that deprectation from the other issue.

#36's interdiff is wrong; it only lists #34.2 being fixed, but #34.1 is also fixed :)

Tests are green, code looks good and grepping for ::placeholder, drupal_placeholder and placeholder shows nothing funny, so RTBC+1, just nits:

+++ b/core/modules/filter/src/FilterFormatListBuilder.php
@@ -94,24 +95,27 @@ public function buildHeader() {
+      // Emphasize the fallback roles text since it is not a role and is
+      // important to understand when configuring filter formats.

wonky grammar here

+++ b/core/includes/bootstrap.inc
@@ -947,10 +948,17 @@ function drupal_static_reset($name = NULL) {
+ *   \Drupal\Component\Utility\SafeMarkup::format() with a %variable in an

"a %variable" also reads odd

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -232,7 +232,7 @@ public static function format($string, array $args) {
           // Escaped and placeholder.

Is this still true?

core/modules/editor/editor.module
core/modules/filter/src/FilterFormatListBuilder.php
core/modules/filter/src/FilterPermissions.php
core/modules/node/src/Controller/NodeController.php

SafeMarkup class is unused here

+++ b/core/modules/filter/src/FilterFormatListBuilder.php
@@ -94,24 +95,27 @@ public function buildHeader() {
       $roles_markup = $roles ? implode(', ', $roles) : $this->t('No roles may use this format');

Unrelated to this patch, but this implode and others could be candidates for using the CSS-based comma separated item_list in a followup.

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -206,7 +206,11 @@ public function revisionOverview(NodeInterface $node) {
+            '#prefix' => '<em>',
+            '#markup' => $this->t('current revision'),
+            '#suffix' => '</em>',

+++ b/core/modules/update/update.report.inc
@@ -228,7 +228,7 @@ function template_preprocess_update_project_status(&$variables) {
+      $extra_item['data'] = ['#prefix' => '<em>', '#markup' => $value['data'], '#suffix' => '</em>'];

this is on multiple lines in one chunk of code but on 1 line elsewhere, let's do multiple lines everywhere?

Yes i saw that when reading the code, its just that the wording was unclear -- we used to call the placeholder function specifically. It's fine like this anyway.

Committed/pushed to 8.0.x, thanks!

@alexpott thanks for trying to explain in #28 I guess a picture would have been a bit more useful to me, or to thick to understand.

https://www.drupal.org/node/2302363 did not get updated for this change; we need to make sure to update previous change records as well providing new ones.

I made this update:
https://www.drupal.org/node/2302363/revisions/view/8787151/8837711

Sorry, it's fixed now. :)