Follow-up to #2570285: Make sure TranslatableMarkup accepts string values only
Follow-up to #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list

Problem/Motivation

In #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list we left the input value problem unaddressed - $this->string() can still be anything.

Ideally we would use an exception/assert/trigger_error to indicate this is wrong but the docs of t() still say:
"You should never use t() to translate variables, such as calling t($text) unless the text that the variable holds has been passed through t() elsewhere (e.g., $text is one of several translated literal strings in an array)."

So we should address that first.

In the mean time we can at least prevent potential new bugs by casting the input to string, as we've never properly tested passing in toStringable objects.

Proposed solution

Cast $this->string to string, like we do in other value objects.

API changes

None

CommentFileSizeAuthor
#4 2579121-4.patch1.92 KBstefan.r
#2 2579121-1.patch903 bytesstefan.r

Comments

stefan.r created an issue. See original summary.

stefan.r’s picture

stefan.r commented
StatusFileSize
new 2579121-1.patch903 bytes
dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
@@ -75,7 +75,9 @@ class TranslatableMarkup extends FormattableMarkup {
-    $this->string = $string;
+    // @todo Decide whether to enforce $string to be a string.
+    //   https://www.drupal.org/node/2570285
+    $this->string = (string) $string;

Can we have some test coverage, you know, this never can be bad.

stefan.r’s picture

stefan.r commented
StatusFileSize
new 2579121-4.patch1.92 KB
dawehner’s picture

dawehner
Primary language German
commented

Here is a different idea. What about checking whether its an object. In case it is, bypass the translation manager and set $this->translatedMarkup directly?

stefan.r’s picture

stefan.r commented

dawehner what do we set it to though? The problem is our docs say we can still do t(t('string')):

"You should never use t() to translate variables, such as calling t($text) unless the text that the variable holds has been passed through t() elsewhere (e.g., $text is one of several translated literal strings in an array)."

stefan.r’s picture

stefan.r commented

OK so that piece of documentation actually means something far easier:

<ksenzee> isn't that just saying that t($var) is Bad, unless you have t('banana') elsewhere, and $var == 'banana'?

So maybe we can just RTBC #2570285: Make sure TranslatableMarkup accepts string values only instead

stefan.r’s picture

stefan.r commented

as to #5, it's a change in behavior if we make it so that t(t(t(t('string')))) == t('string').. that'd need to happen pre-RC :( If we're changing behavior I'd rather just get #2570285: Make sure TranslatableMarkup accepts string values only in.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Closed (duplicate)

Discussed with @berdir - we decided to go with #2570285: Make sure TranslatableMarkup accepts string values only - passing non strings into TranslatableMarkup should be explicit forbidden and putting TranslatableMarkup into TranslatableMarkup really makes no sense.