Create a sniff to make sure exception messages are not translated

PR: https://github.com/pfrenssen/coder/pull/98

This is tricky because there could be use cases where it makes sense to use t() in exceptions. For example HttpException where we give a user facing reason. Ideally that message should be translated. Maybe Drupal core already does that when printing the exception and translation is done twice?

Example in Drupal core: ImageStyleDownloadController with the user facing exception message "Image generation in progress. Try again shortly."

On the other hand I don't see where Drupal core's default HTML exception subscriber would even make use of the exception message. I only see JSON API and Serialization module use the exception message and printing that in a HTTP response. Those are API modules, so probably does not count as user facing text?

I ran this sniff on Drupal 8.8 core with ./vendor/bin/phpcs -p --standard=coder_sniffer/DrupalPractice --sniffs=DrupalPractice.General.ExceptionT /home/klausi/workspace/drupal-8/core/modules and checked usages. It seems most warnings are correct where t() really should not be used. I was also checking for false positives because the sniff implementation is very broad, but so far it turned up only correct warnings.

https://www.drupal.org/docs/develop/coding-standards/php-exceptions says "Only messages from the install and update system are currently translated as they are user facing.", so it is good that this sniff is in the DrupalPractice standard which is allowed to have minor false positives.

Given that I think we can go ahead with this.

Thanks, committed!

This is too broadly implemented, it throws false positives if a translation method is called anywhere inside the exception invocation.

We want this to happen on cases like this:

throw new \Exception($this->t("Something"));

But since exceptions are classes it is possible that they have entirely legitimate custom methods that are intended to be translated. For example in our project we have exceptions that are intended to stop a build pipeline if a certain condition is not met, and our exception class has a custom method setUserMessage() that passes a render array to show to the end user. This message is intended to be translated. This throws a false positive:

      throw (new PipelineStepExecutionLogicException('Graph data could not be inserted', 0, $e))
        ->setUserMessage([
            '#markup' => $this->t('Could not store triples in triple store. Reason: @message', [
              '@message' => $e->getMessage(),
            ]),
          ]);

Good catch, we should only check for translation within the exception itself. Any methods which might be defined on the exception should be excluded from the rule. I've created a PR to properly scope the rule to only check for translations within the exception: https://github.com/pfrenssen/coder/pull/106

Great job! I checked in my project and no more false positives are reported. Thanks for the quick fix!

It means related core issue should wait for next release with the fix

Thanks a lot, merged!