Remove PlaceholderTrait

Given people really don't seem to like it in #2576533: Rename SafeStringInterface to MarkupInterface and move related classes (and it's blocking the issue), maybe we could just get rid of it instead? PlaceholderTrait would realistically only be used in TranslatableString and FormattableString

+1 for this direction, much clearer. I think it's clear this does more than pure documentation though, so adjusting the title accordingly.

1. --- a/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
   +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
   

   So we keep this, but make it an empty shell?

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
   @@ -24,7 +23,7 @@
   +class TranslatableString extends FormattableString {
   

   This makes a ton of sense to me.

Yes, some didn't like us making a trait out of something with just a static method in #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list anyway.

This wasn't turned into a trait because others might want to use it (they can just call FormattableString::format() or extend FormattableString if they want their own string value object), I think the worry was putting too much logic into a class defining a value object. Let me check with @dawehner as I think he came up with the idea.

Re #5.1 I had just cut & pasted the code and forgot to remove the PlaceholderTrait file :)

1. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
   +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
   @@ -12,167 +12,4 @@
   
   @@ -12,167 +12,4 @@
     */
    trait PlaceholderTrait {
    
   

   So we keep an empty placeholder trait?

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
   @@ -24,7 +23,7 @@
   -class TranslatableString implements SafeStringInterface {
   +class TranslatableString extends FormattableString {
   

   This is the question, is a translatable string semantically similar to a formatable string or are they more of a sibling or even something completely different? Inheritence for the sake of code sharing is always a bad sign, if you ask me.

On the other hand I can see how they are parent/child, especially if you look at the signature which is used at the end of the day.

IMO a translatable string is exactly the same as a formattable string (markup), except it's also translatable

IMO a translatable string is exactly the same as a formattable string (markup), except it's also translatable

Yeah I think I agree now.

Adapted the title to not require people to look it up in a dictionary.

+1 to this direction.

Yeah this direction makes way more sense, +1.

This is ready now. Let's get this in, that will unblock #2576533: Rename SafeStringInterface to MarkupInterface and move related classes.

These has been all usages of the word "PlaceholderTrait" in core.

Does someone mind opening up a new follow up to move the test coverage away from \Drupal\Tests\Component\Utility\SafeMarkupTest::testFormat ?

+++ b/core/lib/Drupal/Component/Utility/FormattableString.php
@@ -56,12 +54,12 @@ class FormattableString implements SafeStringInterface {
-   * @param array $args
+   * @param array $arguments

+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
@@ -159,16 +157,6 @@ public function __sleep() {
-   * Returns a representation of the object for use in JSON serialization.
-   *
-   * @return string
-   *   The safe string content.
-   */
-  public function jsonSerialize() {
-    return $this->__toString();
-  }

This doesn't seem in scope.

we inherit that from the parent

we inherit that from the parent

#21 therefore was correct.

1. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -56,12 +54,12 @@ class FormattableString implements SafeStringInterface {
   -   * @param array $args
   +   * @param array $arguments
   
   @@ -95,4 +93,166 @@ public function jsonSerialize() {
   +   * @param array $args
   ...
   +  protected static function placeholderFormat($string, array $args) {
   ...
   +    foreach ($args as $key => $value) {
   

   @alexpott and I discussed this--it's kinda borderline scope, but we need to fix it in other places too to make it consistent, so let's do that in a followup.

2. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -95,4 +93,166 @@ public function jsonSerialize() {
   +   * Formats a string by replacing variable placeholders.
   

   This is almost the same as, but not the same as, the class's one-line summary.

3. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -95,4 +93,166 @@ public function jsonSerialize() {
   +   * This trait is designed for formatting messages that are mostly text, not as
   ...
   +   * rather than this trait.
   

   Not a trait anymore. :)

4. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -95,4 +93,166 @@ public function jsonSerialize() {
   +   * - The passed in string should contain no (or minimal) HTML.
   +   * - Variable placeholders should not be used within the "<" and ">" of an
   ...
   +   * To build non-minimal HTML, use an HTML template language such as Twig,
   

   Some of this stuff is now duplicated between the class and method documentation. (In the previous issue, #2570431: Document that certain (non-"href") attribute values in t() and SafeMarkup::format() are not supported and may be insecure, we had to split it up and do some duplication because it was in two places. Another reason this change just makes more sense.)

5. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
   @@ -159,16 +157,6 @@ public function __sleep() {
   -   * Returns a representation of the object for use in JSON serialization.
   -   *
   -   * @return string
   -   *   The safe string content.
   -   */
   -  public function jsonSerialize() {
   -    return $this->__toString();
   -  }
   

   Based on this hunk being in scope (which also makes sense), @alexpott and I discussed the other class members and determined that $arguments also does not need to be duplicated. The rest made sense to "override" the docs, or were not members of FormattableString.

Haha, I was re-uploading #21 also in #28, but then cross-posted with #27, which made me decide it was probably better to not re-upload :P

@alexpott also filed #2578377: Make translatable docs consolidated and better for developers.

Removal of PlaceholderTrait, which was rushed in a week ago and never was mentioned in a CR

Thence no need for a change record. ;)

I reviewed this in dreditor, with git diff --color-words, and by comparing the moved lines, plus read the updated classes in toto.

1. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -24,12 +24,33 @@
   - * This class is designed for formatting messages that are mostly text, not as
   - * an HTML template language. As such:
   + * This class is designed for formatting messages that are mostly text, not
   + * as an HTML template language. As such:
   

   An "as" got rewrapped... can be fixed on commit.

2. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -95,4 +114,136 @@ public function jsonSerialize() {
   +   *     @code
   +   *       $string = "%output_text";
   +   *       $arguments = ['output_text' => 'text output here.'];
   +   *       $this->placeholderFormat($string, $arguments);
   +   *     @endcode
   +   *     makes the following HTML code:
   +   *     @code
   +   *       <em class="placeholder">text output here.</em>
   +   *     @endcode
   

   Not introduced here but this hunk is a bad example for two reasons ($variable in the first parameter is bad for t() parsing, and that's going to inherit these docs. Note to self: followup.

3. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -95,4 +114,136 @@ public function jsonSerialize() {
   +  protected static function placeholderFormat($string, array $args) {
   

   Still needs a followup to rename the method and the parameter.

4. +++ b/core/lib/Drupal/Component/Utility/FormattableString.php
   @@ -95,4 +114,136 @@ public function jsonSerialize() {
   +          // within HTML attributes, JavaScript, or CSS. See
   +          // \Drupal\Component\Utility\SafeMarkup::format().
   

   This "see" makes no sense now. Can be removed on commit.

5. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatableString.php
   @@ -73,7 +70,7 @@ class PluralTranslatableString extends TranslatableString {
   -   * @see \Drupal\Component\Utility\PlaceholderTrait::placeholderFormat()
   +   * @see \Drupal\Component\Utility\FormattableString:placeholderFormat()
   

   Caught this with a git diff --color-words -- we are losing a colon here. Can also be fixed on commit.

6. --- a/core/tests/Drupal/Tests/UnitTestCase.php
   +++ b/core/tests/Drupal/Tests/UnitTestCase.php
   @@ -12,7 +12,6 @@
    use Drupal\Component\Utility\SafeMarkup;
    use Drupal\Core\Cache\CacheTagsInvalidatorInterface;
    use Drupal\Core\DependencyInjection\ContainerBuilder;
   -use Drupal\Component\Utility\PlaceholderTrait;
    use Drupal\Core\StringTranslation\TranslatableString;
    use Drupal\Core\StringTranslation\PluralTranslatableString;
    
   @@ -24,8 +23,6 @@
   
   @@ -24,8 +23,6 @@
     */
    abstract class UnitTestCase extends \PHPUnit_Framework_TestCase {
    
   -  use PlaceholderTrait;
   -
   

   So this was weird... I double-checked and this was not used within the class itself, plus if any unit test ever used one of the methods on it, obviously that test would fail now. So this is fine.

Commit diff:

diff --git a/core/lib/Drupal/Component/Utility/FormattableString.php b/core/lib/Drupal/Component/Utility/FormattableString.php
index e226973..c069da2 100644
--- a/core/lib/Drupal/Component/Utility/FormattableString.php
+++ b/core/lib/Drupal/Component/Utility/FormattableString.php
@@ -24,8 +24,8 @@
  * examples where translation is not applicable and using this class directly
  * directly is appropriate.
  *
- * This class is designed for formatting messages that are mostly text, not
- * as an HTML template language. As such:
+ * This class is designed for formatting messages that are mostly text, not as
+ * an HTML template language. As such:
  * - The passed in string should contain no (or minimal) HTML.
  * - Variable placeholders should not be used within the "<" and ">" of an
  *   HTML tag, such as in HTML attribute values. This would be a security
@@ -197,8 +197,7 @@ protected static function placeholderFormat($string, array $args) {
           // \Drupal\Component\Utility\SafeMarkup\SafeMarkup::isSafe() may
           // return TRUE for content that is safe within HTML fragments, but not
           // within other contexts, so this placeholder type must not be used
-          // within HTML attributes, JavaScript, or CSS. See
-          // \Drupal\Component\Utility\SafeMarkup::format().
+          // within HTML attributes, JavaScript, or CSS.
           $args[$key] = static::placeholderEscape($value);
           break;
 
diff --git a/core/lib/Drupal/Core/StringTranslation/PluralTranslatableString.php b/core/lib/Drupal/Core/StringTranslation/PluralTranslatableString.php
index c7d8d58..f8fb07b 100644
--- a/core/lib/Drupal/Core/StringTranslation/PluralTranslatableString.php
+++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatableString.php
@@ -70,7 +70,7 @@ class PluralTranslatableString extends TranslatableString {
    * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
    *   (optional) The string translation service.
    *
-   * @see \Drupal\Component\Utility\FormattableString:placeholderFormat()
+   * @see \Drupal\Component\Utility\FormattableString::placeholderFormat()
    */
   public function __construct($count, $singular, $plural, array $args = [], array $options = [], TranslationInterface $string_translation = NULL) {
     $this->count = $count;

Thanks!

#2580505: Improve FormattableMarkup documentation will do some of the follow-ups @xjm asked for.

I'm going to go through the comment thoroughly, and see if any additional ones are needing, or if more should be done in that issue to address all the concerns.

I made #2580525: Rename placeholderFormat() to replacePlaceholders() and the argument args to arguments for #35 3.

I think that combined with #2580505: Improve FormattableMarkup documentation will address all the concerns. removing needs followup tag.