backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

Remove `t()` from the `$message` variable in test assertions, or replace it with `format_string()` #6277

Open klonos opened 10 months ago

klonos commented 10 months ago

This has been on my list for a while, but never filed an issue for it before.

See https://www.drupal.org/project/drupal/issues/500866

Problem/Motivation

Status

The issue has been open since 2009. It has consensus and the approval of SimpleTest maintainers, and is already part of our documented standard: http://drupal.org/simpletest-tutorial-drupal7#t

Details

  • The $message parameter of SimpleTest assertions (e.g., DrupalTestCase::assertTrue()) is a string displayed only in the administrative UI or on the commandline following test runs, and on testbot.
  • This parameter is never translated, so using t() on it is needless overhead.
  • We have other instances in core where string function parameters should not be wrapped in t(), e.g. watchdog().
  • When the assertion message is a plain string, t() should simply be omitted.
  • When the assertion message contains placeholders for variables, format_string() should be used instead.

https://www.drupal.org/docs/7/testing/assertions (which is what we are following in Backdrop as well AFAIK) mentions that already:

Note: Whenever you pass a message into a SimpleTest assertion, do NOT translate the message using t(). (To format a string with variables, use format_string() instead.)

We should also add a regex in PHPCS to be catching these (or in CSpell if not possible PHPCS).

klonos commented 10 months ago

...we should also add a note about this wherever makes sense, like in https://docs.backdropcms.org/documentation/testing-framework for instance (but perhaps also in the docblocks of the assertion functions).