REVERTED: Allow passing constants to t() translate function to prevent 'Only string literals should be passed to t() where possible' CS warning

Currently we have this code, that do this check, in the file coder_sniffer/Drupal/Sniffs/Semantics/FunctionTSniff.php:

        if ($tokens[$argument['start']]['code'] !== T_CONSTANT_ENCAPSED_STRING) {
            // Not a translatable string literal.
            $warning = 'Only string literals should be passed to t() where possible';
            $phpcsFile->addWarning($warning, $argument['start'], 'NotLiteralString');
            return;
        }

Any ideas on how we can add here the detection of constants?

I've found a way via allowing also T_STRING argument type, via this change:

        if (
          $tokens[$argument['start']]['code'] !== T_CONSTANT_ENCAPSED_STRING
          && $tokens[$argument['start']]['code'] !== T_STRING
        ) {

It works well, and if I'm trying to pass a string with a inline variable like "foo $bar" - the argument type is T_DOUBLE_QUOTED_STRING, so it still works well for such case.

Could anyone check if this is a suitable fix, or it can also allow some "bad things" to pass?

Here is my PR in GitHub tracker: https://github.com/pfrenssen/coder/pull/182

I've fixed all remarks by comments in the PR https://github.com/pfrenssen/coder/pull/182 - please review.

Yeah, the potx issue seems an important reason to revert this, didn't know about this. Thanks for the investigation!

But maybe there are chances to improve/fix this on potx side? If no - seems yes, we should revert this.

But for me - using constants for repeatable strings is a very convenient tool, because in many contrib modules, and even in Core I see a lot of repeatable strings, and sometimes they should be the same but differ just because of paraphrasing or some punctuation, as result we have several almost equal strings in translation queue instead of one. So, the constants approach for translation strings should minimize such cases.

@penyaskito, I need something like this:

class ModuleController {
  public const PATH_VALIDATION_RULES = "Only numbers and characters are allowed in the string";
}

class ModuleSettingsForm {
...
   $form_state->setErrorByName('field', $this->t(ModuleController::PATH_VALIDATION_RULES));
...
}

class EntityEditForm {
...
   $form_state->setErrorByName('field', $this->t(ModuleController::PATH_VALIDATION_RULES));
...
}

class ModuleViewHelpPage {
...
  $output .= $this->t("Rules for the path parameter: @rules", [
     '@rules' => $this->t(ModuleController::PATH_VALIDATION_RULES),
  ]);
...
}

So, I want to have the message text only in one single place, not the 3+ duplicates of it, where it can be typed differently like this:

Only numbers and characters are allowed in the string.
Only numbers and characters are allowed in the string
Only characters and numbers are allowed in the string
Only numbers and characters symbols are allowed in the string

in result, giving 4 almost the same strings in the translation queue.

So, the approach of defining the message text in a const variable to reuse in many places looks very convenient.

And replacing it with something like this:

class ModuleController {
  function getPathValidationRulesMessage()
    return $this->t("Only numbers and characters are allowed in the string");
  }
}

looks much worse! At least, because it will require always passing the ModuleController as a dependency to the each class where we need this string.

And about potx - maybe we can extend it via catching constant values for translating, that are described via specific annotation tags like this:

class SomeClass {
  /** 
   * @const Path validation rules description
   * @translatable
   */    
  public const PATH_VALIDATION_RULES = "Only numbers and characters are allowed in the string";
}

We already have something similar in plugin annotations:

/**
 * Provides an action that can save any entity.
 *
 * @Action(
 *   id = "entity:save_action",
 *   action_label = @Translation("Save"),
 *   deriver = "Drupal\Core\Action\Plugin\Action\Derivative\EntityChangedActionDeriver",
 * )
 */

So, this approach will not require executing the code to get strings for translation, just a simple parsing should be enough.

What do you think about this approach?

To move on I created a feature request in the potx module: #3392795: Extract translatable strings from constants with @translation tag