#816642: Two improved field exceptions pointed out that we have a mixture of translated an untranslated exceptions in core. I agree they shouldn't be translated, but t() placeholders are handy. I vaguely remember we had a non-translating function which did basic placeholder replacement but can't remember what or where it is, having to do sprintf() every time feels like a pain, putting variables directly into the string encourages bad habits for XSS even if the individual cases are safe. Either way, opening an issue.

Comments

jbrown’s picture

tstoeckler’s picture

putting variables directly into the string encourages bad habits for XSS even if the individual cases are safe

Are there any cases, where this would be unsafe? Otherwise, I think it should be fine to do that as a standard. We already do that in SimpleTests also.

attiks’s picture

Version: 7.x-dev » 8.x-dev

We discussed this today on irc://irc.freenode.net/drupal-i18n and +1 on not using t() inside exceptions.

tstoeckler’s picture

Regarding variables in exception strings: I personally still don't find anything wrong with simple string concatenation, but in tests we're more or less consistently using the (semi) new format_string().

andypost’s picture

Gábor Hojtsy’s picture

Indeed, format_string() :)

Jose Reyero’s picture

So maybe we should add a better description for this issue and work on a patch for removing t() out of all exceptions?
- About why translating exceptions is actually a bug, it is explained here #1808864: Exception handling is not 'locale safe' (thus not database safe)
- The replacement function for 't()' in this case is 'format_string()', @Gábor #6

However I think we should push this patch here before, because there's no point in making exceptions not using locale when the exception handling code is using it heavily: #1808864: Exception handling is not 'locale safe' (thus not database safe)

chx’s picture

I often wondered whether it is a good idea to translate these and I had no idea why did we attempt: these are not normal user facing strings and I am sure it can cause serious trouble calling such a complicated API as t()/locale() when you are on your way out because of the error handling.

Exceptions (heh) need to be made for user facing exceptions, at least field errors are such.

Sylvain Lecoy’s picture

Symfony 2 uses sprintf without any issues.

When you throw exceptions, you generally knows where your data come from, and using sprintf does seem as bad.

Anyway, I would not use t() at all, exceptions should be in plain english, as the documentation is. Exceptions are for programmers, not users.

Error messages, instead, are generated when catching an exception; they should be translated, because the user will get them displayed.

andypost’s picture

Suppose core should use php function setlocale() to try to init language 'EN' for messages

hass’s picture

I have seen strings from #2137815: InvalidArgumentException: Invalid translation language specified. in UI. Strings visible in UI must be translated. Otherwise users do not understand the messages. There are a lot of messages not translated, see #2137611: Translatable string hidden from potx and hundereds others.

andypost’s picture

Issue summary: View changes

is there a reason to keep this open?

lokapujya’s picture

Yeah, aren't we supposed to remove the t() from core/lib/Drupal/Core/Controller/ExceptionController.php?

<?php
      '#content' => t('The website has encountered an error. Please try again later.'),
?>
Gábor Hojtsy’s picture

No, that is similar to t('The requested page "@path" could not be found.', array('@path' => $request->getPathInfo())), these are user facing messages.

tstoeckler’s picture

I agree 100% with #9 but there is also a techincal issue with this: Calling t() means either one of these things:
- We cannot unit test the class (not really an option)
- We inject the translation manager (cumbersome)
- Once that is in we use the StringTranslationTrait (also not very nice, but the best option)

In other words by translating a string - that no user that is not the developer/site-builder of a site should ever see - we need to import the whole string translation concept into all classes that throw an exception. During early bootstrap, the installer, ... this is especially problematic. It has become easier in comparison to D7 (i.e. we no longer need get_t()/st()) but we still don't use a proper kernel/container everywhere, so this is still not a non-issue.

Please let's stop this weirdness and KISS?

lokapujya’s picture

So, from my understanding, the t() in #13 is ok as explained in #14. Do we want to remove t()'s like the one below?
I didn't get whether that last part from #15 that says, "so this is still not a non-issue," means we should close the issue or what.

<?php
throw new UpdaterException(t('Fatal error in update, cowardly refusing to wipe out the install directory.'));
?>

Most of the t()'s that are within exceptions are found in the Database module.

tstoeckler’s picture

Sorry, what I meant was that depending on where you throw an exception, i.e. very early in the bootstrap, the t() function might not work correctly yet. Therefore it's not always possible/safe to use it. That's just one more reason to stop using t() in exceptions alltogether.

dawehner’s picture

Sorry, what I meant was that depending on where you throw an exception, i.e. very early in the bootstrap, the t() function might not work correctly yet. Therefore it's not always possible/safe to use it. That's just one more reason to stop using t() in exceptions alltogether.

One approach we have been talking is to use some special kernel exception listener which translates the messages, if they can be applied. Therefore we would probably need more typed exceptions or have something
like a String::format object which you can given string + placeholder variables to render it later.

fietserwin’s picture

Status: Active » Closed (duplicate)

Closing this one as a duplicate of #2055851: Remove translation of exception messages. Although this one is older, the other title and issue summary were better and the latter is now even updated with comments from this thread.