Remove translation of exception messages

Exception messages shouldn't be translated.

Is there an issue somewhere that explains why not, and whether it covers all exception messages or only some? For example, in authorize_filetransfer_form_validate(), we have throw new Exception(t('The connection protocol %backend does not exist.', array('%backend' => $backend)));, which on first glance, seems like a reasonable thing to translate, unless there's a reason for not translating any exception message. On the other hand, #1987602-79: Convert ajax_form_callback() to a new style controller points out throw new HttpException(500, t('Internal Server Error'));, and given that "Internal Server Error" is a standard reason phrase defined in http://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html#sec6.1.1, I could see us not wanting to translate that, though even that document says, "The Status-Code is intended for use by automata and the Reason-Phrase is intended for the human user. The client is not required to examine or display the Reason- Phrase.", which makes me think translation of even an HTTP reason phrase could be useful.

So, https://drupal.org/node/608166 totally contradicts what I learned, and I think it's wrong for several reasons:

• "Additional uncaught exception while handling exception." Calling a whole huge codepath in our exception is liable to obscure the real problem in many situations.
• Adds a dependency on common.inc everywhere we want to throw (or test) an exception. Ugggghh.
• Exception messages are developer-facing strings, and as such, there is no real reason to translate them. As in SimpleTest, we don't need to pollute l.d.o with hundreds upon hundreds of strings no one will care about.
• In production, exception messages should not be displayed anyway, as a general best practice.

Now, if we catch the exception message and then pass it to other error handling, we could translate it then, I suppose. But putting t() inside an exception seems quite unwise.

Only relation to WSCII conversions is that some controllers throw exceptions, but so does lots of other code, so untagging.

Is there a final/official point of view here? Should we translate or not exception messages?

@xjm, if NO, can you update https://drupal.org/node/608166 to reflect this new policy?

+1 for no longer translating exception messages. All the reasons from #3, plus that developers,e ven if they don't (exactly) understand the message can still search for it and use google translate to translate the pages found that hopefully describe a solution.

How many times I have tried to search for a translated message and not finding any useful...

Closed #817160: Don't translate exceptions as duplicate and updated the issue summary to incorporate comments from this and the other issue.

I couldn't help someone in IRC today because their exception message was translated.
+1

Started to convert all beside the one in the installer, and some of the http ones.

Ups.

After applying your patch I found these remaining ones.

Besides the installer and updater ones, which indeed seem to be user facing (system administrator, bu that may be a starter who wants to play with Drupal ), there are:
- drupal\core\lib\Drupal\Core\Database\Driver\fake\FakeMerge.php, 40
- drupal\core\modules\rdf\rdf.module, 118

However, I also found this piece of code in drupal\core\lib\Drupal\Core\Database\Install\Tasks.php, 150-:

    // Check for failed results and compile message
    ...
    if (!empty($message)) {
      $message = '<p>In order for Drupal to work, and to continue with the installation process, you must resolve all issues reported below. For more help with configuring your database server, see the <a href="http://drupal.org/getting-started/install">installation handbook</a>. If you are unsure what any of this means you should probably contact your hosting provider.</p>' . $message;
      throw new TaskException($message);
    }

Thus 10 lines above we have one of the exceptions to the rule, but directly below we have a not-translated user facing string. This should be made consistent. Personally, I prefer to translate the $message on line 157, and consequently the task messages online 34-71 as well. BTW: the message should be formatted and the link should be extracted as a parameter.

I din't search for other - more obscured, more difficult to find than these - translated exceptions, but I might do a brute search and quick scan later on.

I found 2 more translated exceptions (using regexp /throw new .*Exception\([^'"S)]/ (all throw's except with a plain message (', "), a String::format message,or no message [)] (and ignoring everything under vendor):
- drupal\core\modules\image\src\Controller\ImageStyleDownloadController.php, 133:

        throw new ServiceUnavailableHttpException(3, $this->t('Image generation in progress. Try again shortly.'));

- drupal\core\lib\Drupal\Core\Extension\InfoParser.php, 44:

          $message = format_plural(count($missing_keys), 'Missing required key (!missing_keys) in !file.', 'Missing required keys (!missing_keys) in !file.', array('!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));
          throw new InfoParserException($message);

This issue needs a re-roll since String::format has been eliminated in preparation for PHP7.

Rerolled and took care of the String::format as pointed by @Lars Toomre at #14

Now it will pass tests. It was missing a class aliasing in one file.

Mhh, do we really want to remove the possibility in the installer to translate those error messages?

I agree, the installer can translate a lot of things. Some of the reasons listed in the summary are totally valid though. Are these messages actually internal and developer-facing or are we using exceptions to let frontend users know about things?

@Gábor Hojtsy @dawehner whenever it is throw new InstallerException() then it means it is non-interactive install. For interactive install the exception title and message must be rendered.

As the non-interactive install is usually done by more advanced users, then I think it is ok to not translate. The only reason I see for translating them is in case of a custom installer which runs the command line under the hood and catches these exception messages. Do we need to worry about it?

whenever it is throw new InstallerException() then it means it is non-interactive install. For interactive install the exception title and message must be rendered.

That sounds contradicting. If its only thrown in non-interactive, how can an interactive install display its title and message?

@Gábor Hojtsy for non interactive it is not thrown directly. It has to be rendered, such as in the file install.core.inc:

  catch (InstallerException $e) {
    // In the non-interactive installer, exceptions are always thrown directly.
    if (!$install_state['interactive']) {
      throw $e;
    }
    $output = array(
      '#title' => $e->getTitle(),
      '#markup' => $e->getMessage(),
    );
  }

The conditional for non-interactive throws directly, while for interactive install it prepares a renderable array.

Right, so should it be rethrown then with a FormattableMarkup wrapper (getting the string and replacements out of t()) then? So it works for both cases? Or does that not help the cause of the issue?

@Gábor Hojtsy I am not sure if the exception massage for that piece of code (#22) is a static string. Probably not.
I think we should not care about it in this issue.

Looks it needs change record or documentation updates to explain how to use formatting in exceptions

https://www.drupal.org/docs/develop/coding-standards/php-exceptions
https://www.drupal.org/docs/8/api/database-api/error-handling

Instead of a reroll I made this patch again from scratch, I didn't use FormattableMarkup though, I used string interpolation instead. I used the regex from #13 to find all Exceptions to replace.

We probably need to grep it once again as core may have got more translated exceptions since the last patch was submitted.

Rerolled from #32

Thanks for work here. Here some minor points:

1. +++ b/core/includes/common.inc
   @@ -1205,7 +1205,7 @@ function archiver_get_archiver($file) {
   +    throw new Exception("Archivers can only operate on local files: $file not supported");
   

   According to the documentation in section about exceptions, we should wrap variables into single quotes:

   throw new Exception("Archivers can only operate on local files: '$file' not supported");
   

   https://www.drupal.org/docs/develop/coding-standards/php-exceptions

   We should apply that rule to all instances. I think it really can make the exception messages a bit easier to read.

2. +++ b/core/includes/install.inc
   @@ -521,10 +521,7 @@ function drupal_install_config_directories() {
   +    throw new Exception('The directory ' .   config_get_config_directory(CONFIG_SYNC_DIRECTORY) .  ' could not be created. To proceed with the installation, either create the directory or ensure that the installer has the permissions to create it automatically. For more information, see the <a href="https://www.drupal.org/server-permissions">online handbook</a>.');
   

   Here we can get a bit better result with sprintf:

   throw new Exception(sprintf('The directory \'%s\' could not be created. To proceed with the installation, either create the directory or ensure that the installer has the permissions to create it automatically. For more information, see the <a href="%s">online handbook</a>.', config_get_config_directory(CONFIG_SYNC_DIRECTORY), 'https://www.drupal.org/server-permissions'));
   

3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -380,10 +380,10 @@ protected function createKeySql($fields) {
   +      throw new SchemaObjectExistsException("Cannot rename $table to $new_name: table $new_name already exists.");
   
   +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -546,10 +546,10 @@ public function findTables($table_expression) {
   +      throw new SchemaObjectDoesNotExistException("Cannot rename $table to $new_name: table $table doesn't exist.");
   ...
   +      throw new SchemaObjectExistsException("Cannot rename $table to $new_name: table $new_name already exists.");
   

   sprintf will be better here:

   throw new SchemaObjectDoesNotExistException(sprintf('Cannot rename \'%1$s\' to \'%2$s\': table \'%1$s\' doesn\'t exist.', $table, $new_name));
   

4. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -546,10 +546,10 @@ public function findTables($table_expression) {
   +      throw new SchemaObjectDoesNotExistException("Cannot rename $table to $new_name: table $table doesn't exist.");
   

   The same here

5. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -546,10 +546,10 @@ public function findTables($table_expression) {
   +      throw new SchemaObjectExistsException("Cannot rename $table to $new_name: table $new_name already exists.");
   

   and here

6. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
   @@ -262,10 +262,10 @@ public function getFieldTypeMap() {
   +      throw new SchemaObjectDoesNotExistException("Cannot rename $table to $new_name: table $table doesn't exist.");
   

   also here

7. +++ b/core/lib/Drupal/Core/Updater/Updater.php
   @@ -291,7 +291,7 @@ public function install(&$filetransfer, $overrides = []) {
   +      throw new UpdaterFileTransferException('File Transfer failed, reason: ' . strtr($e->getMessage(), $e->arguments));
   

   I think that sprintf will be a bit better here

8. +++ b/core/modules/field/field.api.php
   @@ -111,7 +111,7 @@ function hook_field_storage_config_update_forbid(\Drupal\field\FieldStorageConfi
   +      throw new \Drupal\Core\Entity\Exception\FieldStorageDefinitionUpdateForbiddenException("A list field ({$field_storage->getName()}) with existing data cannot have its keys changed.");
   

   +1 to sprintf

9. +++ b/core/modules/field/field.purge.inc
   @@ -157,7 +157,7 @@ function field_purge_field_storage(FieldStorageDefinitionInterface $field_storag
   +    throw new FieldException("Attempt to purge a field storage {$field_storage->getName()} that still has fields.");
   

   +1 to sprintf

10. +++ b/core/modules/options/options.module
    @@ -113,7 +113,7 @@ function options_field_storage_config_update_forbid(FieldStorageConfigInterface
    +      throw new FieldStorageDefinitionUpdateForbiddenException("A list field ({$field_storage->getName()}) with existing data cannot have its keys changed.");
    

    +1 to sprintf

11. +++ b/core/modules/rdf/rdf.module
    @@ -117,7 +117,7 @@ function rdf_get_namespaces() {
    +        throw new Exception("Tried to map $prefix to $namespace, but $prefix is already mapped to {$namespaces[$prefix]}.");
    

    +1 to sprintf

12. +++ b/core/modules/system/system.install
    @@ -364,7 +364,7 @@ function system_requirements($phase) {
    +        throw new \Exception('Tried to generate 10 random bytes, generated ' . strlen($bytes));
    

    +1 to sprintf

@init90 thank you for your detailed review.

Performance-wise, sprintf is known to be slower than dot concatenation or variable replacement in strings.

There is a filed issue to replace sprintf usages by dot concatenation or variable inside string, so i do not think it is necessary to use sprint here.

According to the documentation in section about exceptions, we should wrap variables into single quotes:

We should apply that rule to all instances. I think it really can make the exception messages a bit easier to read.

Addressed #38.1

@jungle, thanks for your feedback. I don't think that performance matters for exception messages.
But in general, I realize that my points about sprintf are contradictory, so I think we can implement only point 1 from #38 and leave decision about sprintf on commiters.

UPD: opps..when I was writing the message, the first point already had been implemented

I don't think that performance matters for exception messages.

@init90 IMO

• if exceptions messages were caught and they were writing into dblog constantly, under this context. I think it matters.
• If it's a White Screen of Death (WSoD), maybe performance does not matter.

Fix Drupal\Tests\dblog\Functional\DbLogResourceTest

Looks this issue should be postponed on #3123061: Create a sniff to make sure exception messages are not translated

Also policy about sprintf() is not accepted yet #3118957: Evaluate replacing all usage of sprintf by dot concatenation or variable inside string

I think it ready for the final review. Thanks!

#42 make sense, but my opinion from #41 that in general it's not critical, still with me:)

#45 IMO better to keep the issues in parallel, at least it allows having proper examples in the core and gets results quicker.

As a bug it needs test - in this case a sniffer, otherwise it will happen again and the policy for interpolation is a second blocker

One blocker is in - code sniff was added recently.

@andypost what do you mean under "policy for interpolation"?

I think. the blocker was in, coder 8.3.9 was committed

Bur policy still needs to resolve #3118957: Evaluate replacing all usage of sprintf by dot concatenation or variable inside string

Quick run of new sniff and quick re-roll for 9.1 + sniffer changes from https://git.drupalcode.org/project/coder/commit/a7c6a38

+++ b/core/includes/install.inc
@@ -281,7 +281,7 @@ function drupal_rewrite_settings($settings = [], $settings_file = NULL) {
-        list($type, $value) = $token;
+        [$type, $value] = $token;

out of scope change

Missed to add patch for #54

Fix for leftover from patch #53

Wow, blast from the past! Glad to see this issue getting addressed.

Scanned the patch visually and noticed this.

+++ b/core/modules/dblog/tests/src/Functional/DbLogResourceTest.php
@@ -88,7 +88,7 @@ public function testWatchdog() {
-    $this->assertResourceErrorResponse(404, 'Log entry with ID 9999 was not found', $response);
+    $this->assertResourceErrorResponse(404, "Log entry with ID '9999' was not found", $response);

Doesn't seem to be in scope, there is no t() function.

@quietone, I have updated the patch.

+++ b/core/modules/dblog/tests/src/Functional/DbLogResourceTest.php
@@ -88,7 +88,7 @@ public function testWatchdog() {
-    $this->assertResourceErrorResponse(404, 'Log entry with ID 9999 was not found', $response);
+    $this->assertResourceErrorResponse(404, "Log entry with ID '9999' was not found", $response);

The change is not required here.

@quintone,

Why $this->assertResourceErrorResponse(404, "Log entry with ID '9999' was not found", $response); is used instead of $this->assertResourceErrorResponse(404, 'Log entry with ID 9999 was not found', $response);?

Because we have this,

diff --git a/core/modules/dblog/src/Plugin/rest/resource/DBLogResource.php b/core/modules/dblog/src/Plugin/rest/resource/DBLogResource.php
index e373c01f3d..706e248b12 100644
--- a/core/modules/dblog/src/Plugin/rest/resource/DBLogResource.php
+++ b/core/modules/dblog/src/Plugin/rest/resource/DBLogResource.php
@@ -45,10 +45,10 @@ public function get($id = NULL) {
         return new ResourceResponse($record);
       }
 
-      throw new NotFoundHttpException(t('Log entry with ID @id was not found', ['@id' => $id]));
+      throw new NotFoundHttpException("Log entry with ID '$id' was not found");
     }
 
-    throw new BadRequestHttpException(t('No log entry ID was provided'));
+    throw new BadRequestHttpException('No log entry ID was provided');
   }
 
 }

change in the patch we have to add id '9999' in quotes instead of something like this 9999.

Moving ticket back for review. Patch #56 works.

@sja112, thanks for the explanation.

Yes, the patch in #56 is correct. I would RTBC but I don't know about changes to phpcs.xml.dist.

Coder sniff DrupalPractice.General.ExceptionT added to 8.9 in #3134731: Update coder to 8.3.9

PS: added to summary

#56 looks ready to go.

Looks coder needs another bump for false positives #3123061: Create a sniff to make sure exception messages are not translated
It could be done in follow-up

HEAD has a mix of sprintf() and just $variable inside double quotes or with dot concatenation, so I think the simplest pattern here is best for now. (Also not convinced that we should worry that much about micro-optimizing performance when exceptions are thrown; they should not be happening under normal site operation generally.)

Adding reviewer credits.

Reviewed locally with git diff --color-words and verified that the only changes are:

• Removal of t(),
• Removal of arguments arrays for t(),
• Replacing placeholders with variables, and delimiting them with single quotes for readability,
• Dot concatenation or curly brace evaluation for values that aren't simple variables, and
• Converting single quotes to double quotes to facilitate the above.

Then to verify that all of core passed with the rule enabled, I ran:
[ibnsina:maintainer | Mon 19:21:27] $ composer run phpcs -- -sp

Committed to 9.1.x. Then, since it's a beta-eligible coding standards change, cherry-picked to 9.0.x and 8.9.x (re-running the core ruleset in both cases). 9.0.x passed, so I pushed that commit; however, looks like 8.9.x has some additional instances that need to be fixed:

FILE: /Users/xjm/git/maintainer/core/includes/install.inc
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 558 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
----------------------------------------------------------------------


FILE: /Users/xjm/git/maintainer/core/includes/common.inc
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 1208 | WARNING | Exceptions should not be translated
      |         | (DrupalPractice.General.ExceptionT.ExceptionT)
----------------------------------------------------------------------


FILE: ...aintainer/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 795 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
 811 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
----------------------------------------------------------------------


FILE: ...maintainer/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 494 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
 506 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
----------------------------------------------------------------------


FILE: ...maintainer/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 705 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
 719 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
----------------------------------------------------------------------

Time: 2 mins, 53.66 secs; Memory: 164MB

Setting PTBP for a current 8.9.x version of the patch. Thanks!

@xjm,

Adding patch with the fixes for the 8.9.x branch.

Fixed test case failure error.

Thanks @sja112! Setting "Needs review" for the backport.

#66, @xjm, got it, thanks for your comment!

#69 addressed, LGTM, thanks @sja112!

>Replacing placeholders with variables, and delimiting them with single quotes for readability,

I doubt that there are many exception messages which do not follow this (haven't checked it). Is it worthy to file a new issue, or ignore it just like ignoring the micro-optimization of sprintf() and dot concatenation?

@jungle We could file a coding standards issue for what the "preferred" format is if we want, but I don't think it matters all that much, so long as we're not complicating the call stack and obscuring debugging info. :)

Reviewed the interdiff for #71. Random observations:

+++ b/core/includes/install.inc
@@ -555,10 +555,7 @@ function drupal_install_config_directories() {
+    throw new Exception("The directory '" . config_get_config_directory(CONFIG_SYNC_DIRECTORY) . "' could not be created. To proceed with the installation, either create the directory or ensure that the installer has the permissions to create it automatically. For more information, see the <a href='https://www.drupal.org/server-permissions'>online handbook</a>.");

Markup in exception messages is interesting; I think this gets sanitized as plain text? Anywho, out of scope.

Also, the database drivers really liked their translated exception messages. I guess this is because they were some of the first things in core to use proper exceptions, and so were written before we had a good grasp on what the consequences would be.

I checked the patch locally with git diff --color-words to validate that it was changing all the correct things as in #69. Finally, I ran composer run phpcs -- -sp locally again to verify that the rule is now passing everywhere for 8.9.x.

Committed and pushed to 8.9.x. Thanks!

#76, To whom may want to file a new issue. Maybe. quoting with `` is better than using single quotes.

@xjm, thank you for committing!

This change had a small syntax error; see #3150990: Updater::install() crashes on file transfer exceptions..

Adding parent as this commit added a DrupalPractice sniff to phpcs.xml.dist