Replace remaining !placeholder for Non-URL HTML outputs only in #title in file widgets

Working on the tests that might be needed here

This is part of the critical meta.

@dawehner suggested on irc to use t() and test that the markup is not being escaped instead

In the conversion we want to figure if a) $element['#title'] might already already be (partly) escaped under any circumstances and if b) it's passing in unsafe content that is marked as safe and bypasses escaping.

As to a), #title is wrapped in t(), so ought to by pass escaping, and b) would only happen when a contrib developer does something silly.

So the solution seems fine and the general idea behind the test as well... just some comments about the test itself:

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -372,19 +372,19 @@ public static function validateManagedFile(&$element, FormStateInterface $form_s
+              $form_state->setError($element, t('The file used in the @name field may not be referenced.', ['@name' => $element['#title']]));

+++ b/core/modules/file/src/Tests/FileManagedFileElementTest.php
@@ -154,4 +154,24 @@ function testManagedFile() {
+    $this->assertRaw('The file referenced by the Managed <em>file</em> field does not exist.');

+++ b/core/modules/file/tests/file_module_test/src/Form/FileModuleTestForm.php
@@ -43,7 +43,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $tree = T
-      '#title' => $this->t('Managed file'),
+      '#title' => $this->t('Managed <em>@name</em>', ['@name' => 'file']),

1. In this test case we pass in a t() value, which marks the whole string (including markup) as safe so @ won't escape again. In the Form API we already warn that #title should always be wrapped in t(), so maybe let's just document in the patch that we are expecting a t()'d value there, hence @placeholders are fine?
2. $this->t('Managed <em>@name</em>', ['@name' => 'file']) is equivalent to $this->t('Managed <em>file</em>' as there is nothing to escape which may be a bit confusing to someone reading the test. I like the idea of using yet another placeholder for the test though, so maybe we could think of a slightly more realistic test case than Managed <em>@name</em>? I don't think we need to be supporting markup in there, it's just quotes and ampersands that are a worry?

Haha :)

As to the code comment, I was referring to the other "@name" (we have one one in the title itself and another one in the placeholder value) i.e. explaining there why double escaping is not a worry.

So like this...

Anyway, I think #12 was RTBC, this just adds that comment.

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -384,6 +388,8 @@
+      // We expect the field name placeholder value to be wrapped in t()
+      // here, so it won't be escaped again as it's already marked safe.

Nit: here could be on the previous line, can be fixed on commit ;)

in the issue, check why they were using !placeholder to begin with and if it can ever be a non-integer

So why do we think they were using !placeholder anyway? Currently, as long as the label is wrapped in t(), "@" ought to be always fine?

I don't thinkk #title is a machine name (as in a-z0-9_)?

@stefan.r, re #16, that was for a different issue (the field size one, which doesn't seem to have its own issue yet). See #2566503-9: [meta] Replace remaining !placeholder for Non-URL HTML outputs only.6.

1. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -372,19 +372,25 @@ public static function validateManagedFile(&$element, FormStateInterface $form_s
   -              $form_state->setError($element, t('The file used in the !name field may not be referenced.', ['!name' => $element['#title']]));
   +              // We expect the field name placeholder value to be wrapped in t()
   +              // here, so it won't be escaped again as it's already marked safe.
   +              $form_state->setError($element, t('The file used in the @name field may not be referenced.', ['@name' => $element['#title']]));
   

   +1 for the added comment explaining the expectations for the assertions.

2. +++ b/core/modules/file/src/Tests/FileManagedFileElementTest.php
   @@ -154,4 +154,26 @@ function testManagedFile() {
   +  public function testManagedFileRemoved() {
   

   +1 for the added test coverage.

3. +++ b/core/modules/file/tests/file_module_test/src/Form/FileModuleTestForm.php
   @@ -43,7 +43,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $tree = T
   -      '#title' => $this->t('Managed file'),
   +      '#title' => $this->t('Managed <em>@type</em>', ['@type' => 'file & butter']),
   

   Sounds delicious.

Nice patch all around.

I think @alexpott had done some research on/fighting with #title elements as well as entity labels, but I did not locate the relevant issues. So I asked @alexpott to comment on the issue or to revert the patch if it's problematic. But it looks thorough to me, so committed and pushed to 8.0.x.