Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list

The patch doesn't apply at all anymore, so better start with a minimal approach again.

@dawehner once SafeMarkup::checkPlain() (or even before) we can just take over the whole SafeMarkup class :)

And now SafeMarkup::format also delays escaping to the last possible moment making it compatible with the work in #2509218: Ensure that SafeString objects can be used in non-HTML contexts

Once this is in and #2569699: Remove SafeMarkup::checkPlain() for Drupal 9.0.x lands we can completely remove the safe list and their associated methods - yay!

Working on the test fails

alexpott is working on this instead.

So we cast to array in the entity query system and that is interesting for objects.

Also we need #2568977: Replace SafeMarkup::format() in the link generator - it's a bad example to everyone so the patch includes that.

+++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
@@ -135,7 +135,7 @@ public function challengeException(Request $request, \Exception $previous) {
     $challenge = SafeMarkup::format('Basic realm="@realm"', array(
       '@realm' => !empty($site_name) ? $site_name : 'Access restricted',
     ));
-    return new UnauthorizedHttpException($challenge, 'No authentication credentials provided.', $previous);
+    return new UnauthorizedHttpException((string) $challenge, 'No authentication credentials provided.', $previous);
   }
 

The interdiff to read missed this.

Locale.Drupal\locale\Tests\LocalePluralFormatTest is fixed by #2570107: Make format_plural() return a PluralTranslatableString object to remove reliance on a static, unpredictable safe list

Rolling with #2570107: Make format_plural() return a PluralTranslatableString object to remove reliance on a static, unpredictable safe list

Getting fixing from #2570107: Make format_plural() return a PluralTranslatableString object to remove reliance on a static, unpredictable safe list

1. +++ b/core/lib/Drupal.php
   @@ -546,16 +546,15 @@ public static function linkGenerator() {
   +  public static function l($text, Url $url) {
   

   Removing the parameter probably requires also changes for the docs also

2. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatedString.php
   @@ -0,0 +1,160 @@
   +   *   themed. See \Drupal\Component\Utility\SafeMarkup::format(). Note that you do
   

   Over 80 chars

3. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatedString.php
   @@ -0,0 +1,160 @@
   +   *   An associative array of additional options. See t() for allowed keys.
   +   */
   

   @return missing

4. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatedString.php
   @@ -0,0 +1,160 @@
   +        // the second plural form as a fallback (which allows for most flexibility
   

   Over 80chars

5. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatedString.php
   @@ -0,0 +1,160 @@
   \ No newline at end of file
   

   Missing newline

6. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -8,10 +8,12 @@
   +use Drupal\Core\GeneratedUrl;
   

   Unused

7. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1531,19 +1533,20 @@ protected function renderAsLink($alter, $text, $tokens) {
   +    return $this->getRenderer()->render($render);
    
   -    return $value;
   

   Space should be before the return

8. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -7,7 +7,10 @@
   +use Drupal\Core\Render\RendererInterface;
   

   Unused

9. +++ b/core/tests/Drupal/Tests/Core/Form/FormErrorHandlerTest.php
   @@ -130,3 +130,11 @@ public function testSetElementErrorsFromFormState() {
   \ No newline at end of file
   

   Newline missing

Re-roll needed before addressing feedback in #31 since the following issue was fixed: #2568977: Replace SafeMarkup::format() in the link generator - it's a bad example to everyone

#31.1 was addressed in #2568977: Replace SafeMarkup::format() in the link generator - it's a bad example to everyone

This patch addresses all other feedback in #31.

The reason why the patch is much smaller in #32 than in #29 is because the issue #2568977: Replace SafeMarkup::format() in the link generator - it's a bad example to everyone implemented some of the same changes as the patch in #29.

1. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatedString.php
   @@ -0,0 +1,162 @@
   +   * Constructs a new class instance.
   

   s/new class instance/new PluralTranslatedString object

2. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatedString.php
   @@ -0,0 +1,162 @@
   +   *   (optional) The string translation service.
   

   I don't think this is tied for string translation being a service

3. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -195,22 +209,57 @@ public static function checkPlain($text) {
   +    // @todo temporary hack whilst !placeholder exists.
   

   If we are going to commit this one, maybe it would be a good idea to point the exact issue which allows fixing this @todo

This needs #2570107: Make format_plural() return a PluralTranslatableString object to remove reliance on a static, unpredictable safe list so postponing.

We should make #2570037: Remove the ability to return unsafe string from Translation->translate() do @todo and re-title it do be to clean up both translation and safemarkup format

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -228,13 +242,48 @@ public static function checkPlain($text) {
+    // @todo temporary hack whilst !placeholder exists.

We could add a link for the issue where this should be removed to ensure it gets done. Otherwise this is RTBC for me.

Created FormattableString class. Also probably fixed the test failure.

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -228,13 +228,24 @@ public static function checkPlain($text) {
-      static::$safeStrings[$output]['html'] = TRUE;

#54: Because this was removed2

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -493,8 +493,12 @@ protected function doPostSave(EntityInterface $entity, $update) {
      protected function buildPropertyQuery(QueryInterface $entity_query, array $values) {
        foreach ($values as $name => $value) {
   -      // Cast scalars to array so we can consistently use an IN condition.
   -      $entity_query->condition($name, (array) $value, 'IN');
   +      // Make scalars and objects to array so we can consistently use an IN
   +      // condition.
   +      if (!is_array($value)) {
   +        $value = [$value];
   +      }
   +      $entity_query->condition($name, $value, 'IN');
        }
   

   This is just wrong. We need to get #2571909: CommentForm selects using the user formatted name in.

2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -112,10 +112,13 @@ protected function createAttributeValue($name, $value) {
        if (is_array($value) || $name == 'class') {
   -      // Cast the value to an array if the value was passed in as a string.
   +      // Cast the value to an array if the value was passed in is not.
   +      if (!is_array($value)) {
   +        $value = [$value];
   +      }
   

   This needs a test

3. +++ b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
   @@ -441,9 +441,7 @@ public function testSetCacheWithSafeStrings() {
   -    $form_state_data['build_info']['safe_strings'] = [
   -      'a_safe_string' => ['html' => TRUE],
   -    ];
   +    $form_state_data['build_info']['safe_strings'] = [];
   

   There was a better fix for this earlier - we should be using SafeMarkup::setMultiple() until we remove it. Because SafeMarkup::checkPlain() is still a thing.

1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -228,13 +228,24 @@ public static function checkPlain($text) {
   +    $string = (string) $string;
   

   assert for string

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -493,8 +493,12 @@ protected function doPostSave(EntityInterface $entity, $update) {
   +      // Make scalars and objects to array so we can consistently use an IN
   +      // condition.
   +      if (!is_array($value)) {
   +        $value = [$value];
   +      }
   

   What about using if (instance oSafeStringInterface($value)) { $value = (string) $value; }

3. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -112,10 +112,13 @@ protected function createAttributeValue($name, $value) {
   +      // Cast the value to an array if the value was passed in is not.
   +      if (!is_array($value)) {
   +        $value = [$value];
   +      }
   

   Explain why and cast to string + explicit test coverage in \Drupal\Tests\Core\Template\AttributeTest

Over in #2571909-32: CommentForm selects using the user formatted name when hook_user_format_name_alter() returns a SafeStringInterface, Drupal\user\Tests\Views\HandlerFilterUserNameTest ends up failing so I expect Drupal\user\Plugin\views\filter\Name will also need updating here once the @todo in user_hooks_test_user_format_name_alter() (which points here) is resolved.

+++ a/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -493,12 +493,8 @@
+      // Cast scalars to array so we can consistently use an IN condition.
+      $entity_query->condition($name, (array) $value, 'IN');
-      // Make scalars and objects to array so we can consistently use an IN
-      // condition.
-      if (!is_array($value)) {
-        $value = [$value];
-      }
-      $entity_query->condition($name, $value, 'IN');

Won't that cause issues with objects again?

+++ b/core/lib/Drupal/Component/Utility/FormattableString.php
@@ -0,0 +1,72 @@
+class FormattableString implements SafeStringInterface {

What about just DynamicString?

Let me try to fix that failure...

I didn't pay attention to the clock, sorry

Taking a look at the failures.

The 2 fails here are related to this change:

+++ b/core/modules/comment/src/CommentForm.php
@@ -263,16 +257,27 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
-
-    if (!$this->currentUser->isAnonymous()) {
-      // Assign the owner based on the given user name - none means anonymous.
-      $accounts = $this->entityManager->getStorage('user')
-        ->loadByProperties(array('name' => $author_name));
-      $account = reset($accounts);
-      $uid = $account ? $account->id() : 0;
-      $comment->setOwnerId($uid);
+    if ($comment->id() && $this->currentUser->hasPermission('administer comments')) {
+      // Admin can leave the author-ID blank to revert to anonymous.
+      $author_id = $author_id ?: 0;
+    }
+    if (!is_null($author_id)) {
+      $account = $this->entityManager->getStorage('user')->load($author_id);
+      if ($author_id || !$form['author']['name']['#access']) {
+        $comment->setAuthorName($account->getUserName());
+      }
+      else {
+        // Anonymous user - use the author name value.
+        $comment->setAuthorName($author_name);
+      }
+    }
+    else {
+      $author_id = $this->currentUser->id();
     }
+    $comment->setOwnerId($author_id);

since $author_id is always null unless the user has the administer comments permission.

I think #2571909: CommentForm selects using the user formatted name is attempting to address this same area of code though, so perhaps this should be postponed?

+++ b/core/modules/comment/src/Tests/CommentPreviewTest.php
@@ -50,13 +51,13 @@ function testCommentPreview() {
-    $this->assertEscaped('<em>' . $this->webUser->id() . '</em>');
+    $this->assertEscaped('<em>' . $this->webUser->getUsername() . '</em>');
...
-    $this->assertRaw('<em>' . $this->webUser->id() . '</em>');
+    $this->assertRaw('<em>' . $this->webUser->getUsername() . '</em>');

The verbose output from the test displays the uid and not the username on the comment form, so I'm wondering why this change was made.

Ok, thanks for the clarification @stefan.r. Then we have to wait for that issue to get fixed.

Postponing this on #2571909: CommentForm selects using the user formatted name. Applied also the patch that we can keep working on after this has been unblocked.

Rerolled using #95.

+++ b/core/lib/Drupal/Component/Utility/FormattableString.php
@@ -0,0 +1,45 @@
+/**
+ * Provides a formattable string class.
+ */
+class FormattableString implements SafeStringInterface {

That comment doens't explain anything

I think we should add any new assertions in followups. Also if we use SafeStringTrait we get a ::create() method we don't want so reverting that change.

I've moved all the docs so the placeholdering docs are on the PlaceholderTrait and all the standard places point to it.

Some minor problems with the code docs:

1. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -0,0 +1,98 @@
   +   * Creates a FormattableString object if necessary.
   

   "if necessary" seems wrong here.

2. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -0,0 +1,98 @@
   +   * If $string is equal to a blank string then it is not necessary to create a
   +   * SafeString object.
   

   This comment doesn't make sense here

3. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -0,0 +1,98 @@
   +   * @return string|\Drupal\Component\Utility\SafeStringInterface
   +   *   A formattable string.
   

   This doesn't seem right - we get back an instance of this class

4. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
   @@ -15,18 +15,62 @@
   +   * unexpected output.
   

   unexpected or unsafe (or both)?

5. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
   @@ -15,18 +15,62 @@
   +   *   - !variable: Inserted as is, with no sanitization or formatting. Only
   

   should we add a note now that this is about to be removed?

I had a change of heart about FormattableString::create() whilst rolling the patch - oops - thanks for the review @pwolanin.

1. Fixed
2. Fixed
3. Fixed
4. Fixed
5. I don't think so

I'm not 100% convinved that the FormattedString is the best place for this, but well, the FormattedString will appear in many places.

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -162,79 +162,41 @@ public static function checkPlain($text) {
+   * @see \Drupal\Component\Utility\FormattableString

but at least for have the @see here

Thanks for the doc fixes.

@stefan.r - it doesn't have a create() method, I think alexpott was just commenting as to why the docs seemed mismatched.

A create() method wouldn't be useful anyhow since you can't give it args according to that interface?

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -162,79 +162,41 @@ public static function checkPlain($text) {
+   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
+   *   Use \Drupal\Component\Utility\FormattableString.

@stefan.r yep hence the deprecate here... :)

I decided against the ::create() method to keep this aligned with TranslatableString...

@stefan.r so the strtr in HEAD does the cast to string... so SafeMarkup::format() supports objects. The reason to cast to string when creating the Formattable string is so that if there is any execptions thrown then it works as expected rather then throwing the exception in FormattableString::__toString() - which would just break.

I'm fine with dropping any kind of asserts, give the problems we have with asserts in the first place, sadly.

1. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -0,0 +1,92 @@
   + * rather than this object, since it will translate the text (on
   

   s/it/they

2. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -0,0 +1,92 @@
   +  public function count() {
   

   Wouldn't ::length() be more appropriate?

3. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
   @@ -15,18 +15,62 @@
   +   *   - @variable: Escaped to HTML using Html::escape() unless the value is
   +   *     already HTML-safe. Use this as the default choice for anything
   +   *     displayed on a page on the site, but not within HTML attributes.
   +   *   - %variable: Escaped to HTML just like @variable, but also wrapped in
   +   *     <em> tags, which makes the following HTML code:
   +   *     @code
   +   *       <em class="placeholder">text output here.</em>
   +   *     @endcode
   +   *     As with @variable, do not use this within HTML attributes.
   

   I think we are supporting @ and % into attributes with the changes introduced in #2571673: Convert Views t() usage where it is used as an attribute value, aren't we? The result will be converted to plain text and then escaped. We will need to update these docs over there if this lands before that one, as it appears likely.

@plach the count behaviour is a Twig hack

@stefan.r:

URLs are still a special beast, but with the changes over there we should have built-in support for the normal attribute values, I think

Note that this will conflict with #2570431: Document that certain (non-"href") attribute values in t() and SafeMarkup::format() are not supported and may be insecure because it moves the docs that is updating.

Also I don't see the change record linked in the sidebar?

Working on the CR

There it is, also adapted https://www.drupal.org/node/2296163

Fixed #113.1 locally and committed/pushed to 8.0.x, thanks!