Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(REF) Convert dynamic translations from ts() to _ts() #26595

Merged
merged 4 commits into from
Jun 24, 2023

Conversation

totten
Copy link
Member

@totten totten commented Jun 22, 2023

Overview

The ts() function is supposed to be used for literal strings (e.g. ts('Hello world')) rather than dynamic strings (ts($message)). This practice ensures that the scanner (civistrings) is able to identify the strings. The scanner emits warnings about nonconformant invocations.

However, there are a handful of cases where we really want dynamic strings -- and where the warning is unnecessary.

Inspired by discussion of civicrm/civicrm-buildkit#784.

Before

Ex: For the Angular integration, it needs to run ts() for all the JS strings. This loops over a statement like:

$translated = _ts($string, [
  'domain' => [$module['ext'], NULL],
]);

This provokes a warning in civistrings:

$ civistrings Civi/Angular/Manager.php
[warn] first argument of ts() call is no string. (Civi/Angular/Manager.php:316)

After

civistrings is pretty chill.

Comments

  • I fixed warnings for the dynamic ts() calls where it seemed structurally appropriate.
  • There are still some other files which have warnings for other reasons (e.g. maybe civistrings misjudges the lines, or maybe there's a weird mix of dynamic/static elements).

@civibot
Copy link

civibot bot commented Jun 22, 2023

(Standard links)

@colemanw
Copy link
Member

I initially thought the name _ts too similar to ts and too easily confused, but the underscore is a pretty standard convention, so, ok.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Jun 22, 2023
@colemanw colemanw merged commit 67f52b4 into civicrm:master Jun 24, 2023
@totten totten deleted the ts-warn branch November 7, 2023 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants