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

I think we can do this - although it is still not ideal to have translatable strings in constants in Drupal. Then you have the same problem again that translation extraction tools cannot find it in source code. I'm not sure if those extraction tools are still widely used, so I'm fine with relaxing this check.

Can you file a pull request against https://github.com/pfrenssen/coder and add a test case?

klausi → closed merge request !22

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.

Merged, thanks!

Also pushed a small follow-up to fix spacing in the test.

The main reason coder complained about constansts in t() was because potx cannot parse those.

potx is still relied by lots of people, and has no other solution solving that problem space in Drupal AFAIK.

localize.drupal.org relies heavily on potx for core and contrib modules translation, so I'd expect to have this rule still in coder.

If it's really gonna be removed, because of that impact, I'd hope it wouldn't happen in a minor release, and that's there some communication around it aside of release notes.

Thanks for checking in here! Hm, there is certainly a tradeoff here. In private projects it is fine if you use constants in t() when you don't use the potx extraction tools. When you use the same string multiple times a constant is also super helpful so that you don't have to maintain the same string in multiple places.

But maybe then private projects should simply disable this sniff if they want to use constants. I'm open to revert this change. Do we see public contrib projects often use constants where Coder should throw an error? Let me know if you have some examples!

Release policy: relaxing or hardening a sniff is perfectly fine in a minor/patch release of Coder (we are not fully following Semver regarding minor/patch releases yet for historic reasons). I would never make a major release because of tweaking a sniff.

Leaving this as active for now for more feedback.

Sadly we can't (or I don't know how) search on drupalci reports to find how often this happens.

But if I search "Only string literals should be passed to t() where possible" on d.o issues I find several issues yet from 2022 or 2023.
Most of them are from patches/code from new contributors, but also found some instances from experienced contributors (not linking because there's no need of public pointing anyone).

But I agree with reverting this. Specially since it's a warning and afaik we can ignore it with // @ignore if you really need it.

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.

@Murz: If you need

$var = 'string';
// Do whatever with t($var);

then better use

$var = t('string');
// Do whatever with $var;

This limits you to use constants in classes which I'm guessing that's what you are looking for.

Potx situation won't change. For being able to parse strings in variables on t(), you'd need to execute the code (as this strings might have operations, concatenations, etc on them) and aside of the complexity of that, that would mean a security risk on localize.drupal.org.

@penyaskito, I need something like this:

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

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

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

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

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 getPathValidationErrorMessage()
    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";
}

What do you think about this approach?

You can always use the "D7 features" approach:

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

  public __ctor() {
     t('Only numbers and characters are allowed in the string');
  }

}

class ModuleSettingsForm {

...
...
   // @ignore rule
   $form_state->setErrorByName('field', $this->t(ModuleController::PATH_VALIDATION_RULES));
...
}

You have an unused t() somewhere to ensure potx finds it, and then ignore the rest.

For new formats and parsers, better to discuss in potx queue itself.

OK, since Murz also agrees that we can revert this I think we can go ahead with the revert before the next release. As the potx parsing is much harder to fix people should make an exception in their phpcs.xml config if they would like to use constants as Murz proposed.

Revert pushed! Thanks both of you for your thoughtful comments.

Forgot credits for penyaskito, added now.

Automatically closed - issue fixed for 2 weeks with no activity.

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