Make sure TranslatableMarkup accepts string values only

Working on this at the DrupalCon BCN extended sprints.

First patch which just throws an InvalidArgumentException when the first parameter to the TranslationWrapper constructor is not a string.

Instead of doing this with an InvalidArgumentException, can we use assert() for this? I think this'd be a really good use-case for that.

Most of the failures are caused by Constraint validation (DrupalTranslation is used to translate the validation messages, but those have been set with t() earlier). Not sure if the approach in the patch is the correct one, since it leaves translation for constraint validation in two separate places.

Running past the testbot to see what other failures remain.

The $id passed to Drupal\Core\Validation\DrupalTranslator->trans() can be a TranslationWrapper object so need to cast to string to ensure it is passed to the magic __toString() trait.

* Switched to an assertions instead of an exception
* Fixed the remaining failure.

@cpj

The $id passed to Drupal\Core\Validation\DrupalTranslator->trans() can be a TranslationWrapper object so need to cast to string to ensure it is passed to the magic __toString() trait.

I crossposted, sorry. My patch was a temporary workaround to see what other failures would appear, so most likely not the correct approach. However, something feels fishy about casting a TranslatedWrapper to string, and then through t() again, since we are basically translating the translation then (with the placeholders already replaced in the first time)?

@mr.baileys - are you here in Barcelona at the sprint ? Then we can discuss face-to-face ?
If not... I debugged this with assistance from @joelpittet and that was the simple solution we came up with

Just to see if this approach works...

@baileys - are you here in Barcelona at the sprint

I am! You can find me near the "Safe Markup" whiteboard :)

The patch from #12 is the better fix.
Note that this solution only addresses the situation when the $id passed to Drupal\Core\Validation\DrupalTranslator->trans() is a TranslationWrapper. As mentioned in the proposed solution section of #1845546: Implement validation for the TypedData API

As a follow-up, we could then allow using symfony validation constraints directly from fapi also.

This would indeed be a nice feature, but right now there are no examples in Drupal that do this.

Just for clarity, reposting the patch from #12

+++ b/core/lib/Drupal/Core/Validation/DrupalTranslator.php
@@ -28,7 +29,9 @@ class DrupalTranslator implements TranslatorInterface {
+    // If a TranslationWrapper is passed as $id, return it (since the messag
+    // has already been translated).

maybe put object after the class name and remove the parenthesis statement.

We may be able to explain this a bit more.

#24 saves a bit of processing if we don't cast then rebuild a new TranslationWrapper, so I'm in total agreement that this is the correct approach to resolve the problem.

Thank you for resolving this @mr.baileys and @cpj

I could be wrong but as I recall watching the debugger with @cpj that it wasn't always translated before when coming from yaml but when created in php.

Thanks for the extra asserts and extra tests. That should go a long way in enforcing the t() doc best practices are adhered to.

Back to needs work, the patch in #29 has an unneeded change in index.php that should be removed. This is the reason of the failures mentioned in #31.

Reverted index.php

Here's a regex to find the problems in core t\('[@%!]\w+[\W']*'

This patch removes the assert that is causing all the errors and we're opening a follow-up issue to add that assert & fix the issues.

+++ b/core/lib/Drupal/Core/Validation/DrupalTranslator.php
diff --git a/core/modules/system/system.module b/core/modules/system/system.module
index a8cc12d..1970a9d 100644

+++ b/core/modules/system/system.module
@@ -1311,7 +1311,7 @@ function system_time_zones($blank = NULL) {
+      $zones[$zone] = t(str_replace('_', ' ', $zone));

Should be fixed in a separate issue since this does not cause the remaining two assertions in this issue to fail.

2570285-FINAL.Last-definite-wont-need-any-changes-can-go-straight-in-no-questions-asked.patch

Seems we should also rewrite the assertions to be more secure (even though user-entered input should not be translated in the first place): see https://www.drupal.org/node/2569049

Needed a re-roll since TranslationWrapper has been renamed to TranslatableString. Since the assertion on empty string also causes numerous failures, and after talking with @joelpittet and @stefan.r we're going back to just one single assert. Additional assertions are introduced (and their failures addressed) in follow-up issues.

There are follow-ups to ensure TranslationString is more sane but this goes far enough to ensure we at least have is_string() coverage for the the values and it fixes a bug where we were double translating an existing translation with the same values.

Follow-up issue for the empty string assertion: #2571677: Ensure no empty strings are fed to TranslatableMarkup

+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
@@ -79,6 +79,7 @@ class TranslatableString implements SafeStringInterface {
+    assert(is_string($string), '$string must be a string.');

Afaik this is the wrong type of assertion... I think this is supposed to be assert('is_string($string)', '$string must be a string.');

I've asked @AkiTendo to comment.

Nope, we agreed with AkiTendo to not use eval style asserts. https://www.drupal.org/node/2569049#comment-10345567

We sort of lose either way. Eval is dangerous. Asserts executing their arguments even when turned off is a bug, but one PHP 7 corrected. Which is more dangerous - the eval string or the erroneous execution? The former could compromise the site if misused, the latter just means performance loss but the site will still work. In light of the fact the bug is fixed in PHP 7 I'm inclined to avoid the eval string style for safety and just be careful to make sure that any performance affecting assertions don't run in production by directly checking the assert.active flag.

EDIT:

Note the following - very simple assertions wouldn't have cost us much processor time anyway - things like boolean expressions, datatype checks, instanceof. The more complex assertions of the Inspector can be hotwired so they aren't performed when assert.active is set false - we just have to have the function make the check directly in PHP 5 and PHP 7 will skip over the whole process at the assert() statement itself for us. With eval we are asking the contrib authors to be careful not to expose the system to an attack - with the check of assert.active they don't have the opportunity to make that mistake. I think that will be easier.

There are still a strong reason to use assert over exception - context is a VERY important thing when reading unfamiliar code. By it's nature assert() (once you are familiar with it) screams "This should not be happening", where an exception could have a legitimate cause. There's no way of knowing for sure without carefully stepping through the code.

Also, most of the performance gain can be preserved by manually checking the flag, and PHP 7 will give us all of what we want once it becomes baseline.

Reroll of #47.

This is exactly the case where an assert makes no sense and an exception is correct! Please read #2569049-48: Add a hook_requirements() that warns if assertions are turned on and discourage/remove double quotes in assert() .

@chx: Based on Aki Tendo's feedback in #2569049-49: Add a hook_requirements() that warns if assertions are turned on and discourage/remove double quotes in assert() I think this should be kept as an assert.
@Stefan.r: the message portion of assert() is also evaluated in PHP 5 at all times, are we ok with the performance penalty of calling __toString() each time a TranslatableString() is constructed?

+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
@@ -75,6 +75,7 @@ class TranslatableMarkup extends FormattableMarkup {
+    assert(is_string($string), '$string ("' . (string) $string . '") must be a string.');

A quick idea: If we introduce this here, we should provide a little bit more helpful message. Don't pass in an already translated string into translations.

Quick question - why are we blocking out objects with __toString implemented? Unless there is a reason for this the assertion should be

assert(is_string($string) || is_object($string) && method_exists('__toString', $string))

I should have made a shorthand method for that. New ticket.

Ok - just checking on that. Still, I did go ahead and add the shorthand method for situations where such clarification is not necessary. #2579095: Create Inspector::assertStringable - a shorthand for (is_string($string) || (is_object($string) && method_exists($string, '__toString')

We could at least trigger_error something or maybe log something. Its a clear developer error IMHO if this happens.

Title change because things got renamed...

+1 to not accepting non-strings. @alexpott just closed #2579121: Cast TranslatableMarkup input values to string, it is inconsistent with FormattableMarkup in favor of this.

If you actually have locale enabled, then this results in a very cryptic database exception when you e.g. run into one of the constraint messages. So alexpott and I just discussed this should probably always throw an exception, not just doing an assert.

Can confirm that the patch in 80 is working. Unfortunately i found the patch only after creating a duplicate: #2581939: Translated FieldItemList produces fatal error

Apparently this is causing exceptions in core, patch idea via Alex Pott not 100% sure this was what he meant.

The fact that with locale this break forms means that this is would cause a WSOD's and presents the user with no chance of recovery during form entry - this is therefore critical for me.

Patch attached includes test in #84 and changes assert to an exception because there is no point in production of just throwing a less helpful exception later on.

Here's the test-only patch @catch meant to upload...

+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
@@ -72,9 +72,16 @@ class TranslatableMarkup extends FormattableMarkup {
+      $message = $string instanceof TranslatableMarkup ? '$string ("' . $string->getUntranslatedString() . '") must be a string.' : '$string ("' . (string) $string . '") must be a string.';

Do we not want to say "$string is an instance of TranslatableMarkup, you should cast to a string before passing as a parameter"?

Otherwise it's going to look "foo must be a string"? Type seems more helpful than which string, although I guess they get the backtrace too.

I think this is fine though and if we want a different exception message we can do that in a follow-up.

It is possible that this is going to cause exceptions on contrib/custom code that are currently working 'OK' (because the nested object doesn't end up anywhere that causes the exception, or because they don't have locale module enabled etc.), but it will not take much for the site to get broken later on.

So a hard break here seems the best option, and it's an easy fix if your code does get broken - just string cast the argument first.

RTBC pending the bot.

Also glad we did beta 16 last week might not have realised how bad this was without that (although stefan_r clearly did, just rest of us didn't).

No issue credit for me uploading a patch to completely the wrong test :P

Patch looks good to me. Am about to commit. Looks to me like every participant here made a useful contribution to getting this figured out, so ticking credit boxes for everyone. I'll leave @catch off the commit message per his request, but drupal.org doesn't let me untick his checkbox.

Pushed to 8.0.x. This does need a change record per #89, so "Needs work" for that. Passing a variable (as opposed to a PHP string literal) at all to the first parameter of t() is generally bad, and passing a variable that's already been t()'d even more so. So I'm not worried about strictly disallowing the latter. But we should inform people of it.

Working on this CR.

Added one

Change record is done so marking this fixed.

One thing that would improve it is a "doing it wrong vs. doing it right" example. I couldn't really find one in the patch. Presumably there was an actual bug we were attempting to avoid with this patch though, so if that exists and there's a before/after snippet we could put there, that'd be helpful for people understanding if this affects them or not.

#2576765: Bring SafeMarkup::format()/t() docs up to date with the final state of the sanitization API is also open for improving the docs. One line we should change there is:

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). It is especially important never to call t($user_text) where $user_text is some text that a user entered - doing that can lead to cross-site scripting and other security problems.

Note 'unless'.