[colemanw]

Overview

Followup from #32859 this removes inline ts() from translatable strings as it's no longer required in the form markup.

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[colemanw]

@samuelsov @totten @shaneonabike this is the only case I found where the variable-injection feature of ts() was being used. IMO we don't need to support that going forward & i think this change is good enough.
But I'm also just curious to know if the resulting non-ts markup is not too weird to get parsed by the new string extractor you built. It now looks like this: <a>Client #{{theClient.id}}</a>

[shaneonabike]

Woooaaah awesome work on removing all that code!

Ohhh it stripped out all the extra classes and stuff 😦 It shouldn't be doing that actually. I will need to loop back to test that I think. I think our initial thought was to remove any type of malicious injection.. and I was using a similar thing to other parts of CiviCRM. If it comes off your head a better way to strip out malicious but keep the rest we probably should.

Ideally, we would want it to produce this I think:

<a ng-click="selected.tab = 'client_' + theClient.id">{{ts('Client #%1', {1: theClient.id})}}</a>

Thoughts @colemanw

[colemanw]

@shaneonabike Ohhh it stripped out all the extra classes and stuff 😦

Where did it do that?

[shaneonabike]

@colemanw Sorry I lead you down a bad journey 🧙 It was stripping before but long ago we removed that I think.

I checked the code again and can see that normally the code below is supposed to just extra the strings using html().

civicrm-core/ext/afform/core/Civi/Afform/StringVisitor.php

Line 62 in 9f34852

$markup = trim($pqItem->html());

        $markup = trim($pqItem->html());
        if ($this->isWorthy($markup)) {
          $translated = $callback($markup, 'content');
          if ($markup !== $translated) {
            $pqItem->html($translated);
          }

And ironically, we have a check IsWorthy that is essentially supposed to skip over any strings that have a ts(). Sooo long story short looking back on our code I'm wondering if it's not actually extracting this via another avenue like defn or attribute which don't pull the full HTML but use JSON to extract

[colemanw]

@shaneonabike my bad I think I led you down the wrong path with my shorthand comment. When I said

It now looks like this: <a>Client #{{theClient.id}}</a>

I left out the extra tag attributes on purpose for brevity, not because they're actually being removed. I was just trying to focus the conversation on the contents of the tag & not get distracted by the attributes.

[shaneonabike]

Haha man we need to be careful if we go on a hike together...

Ok so technically this was supposed to ignore anything that had a ts() in it via the IsWorthy call so it seems strange it would strip it out

[colemanw]

@shaneonabike I think we're still not on the same trail 🥾

My PR here is what's doing the stripping of ts(). I haven't checked what happens at runtime, all I've done so far here is a search/replace of ts() to remove it and leave the bare strings.

[totten]

It now looks like this: <a>Client #{{theClient.id}}</a>

I started a bit skeptical about removing ts-vars completely, but this seems a good example, and it definitely feels plausible.

So the question is: does it actually work? I could be misreading, but I think Shane's comments are drilling down on that -- e.g. perhaps it doesn't work because of the isWorthy() filter?

[totten]

FWIW, I just tried this:

• Login to demo site (http://core-33483-8q30ssa.test-1.civicrm.org:8003/)
• Enable fr_FR
• Enable oauth-client
• Enable uplang
• Open http://core-33483-8q30ssa.test-1.civicrm.org:8003/civicrm/admin/oauth?lcMessages=fr_FR#/!/?provider=testoauth.com

Unfortunately, I don't think we have relevant strings for testing. So it's a longer process to test. (Phh.)

But I did notice that the one part that was translated before ("Delete" <=> "Supprimer"; screenshot from master) is no longer translated (screenshot from PR).

[totten]

We should run civistrings on all these files before+after -- and gauge impact.

Note that there's a related PR civicrm/civistrings#18.

N.B. For strings in core, we still need packaged strings (e.g. POT => Transifex => MO) because we don't want every site-admin to manage separate strings for every core form...

[totten]

re: isWorthy() filter -- I never worked through those conditions from a systemic POV. (I think I may have tried omitting the conditions at one point, realized that they were preventing a problem, accepted it, and just... moved it into a function...)

I guess we should be explicit about the purpose (at the risk of overcommunicating...). We have two practices:

• Old Way: title="{{ts('Required')}}"
• New Way: title="Required"

That means transition time. That's either a hard switch (BC break from "Old Way") or a deprecation/bridge period (with "Old Way" and "New Way" coexisting). If I understand correctly, isWorthy() helps with the latter approach (deprecation/bridge period).

In this case, the system should run both these examples (at the same time):

<a title="New Wave">Talking Heads</a> 
<a title="{{ts('Old School')}}">The Cars</a> 

We would expect these behaviors:

• Scanning for source-strings
  • It MUST detect New Wave as a string (in both afform and civistrings)
  • It MUST NOT detect {{ts('Old School')}} as a string (even though it's a similar <a title>).
  • Detection of Old School is weirder:
    • civistrings MUST detect Old School.
    • AFAICS, afform could go either way. (Not needed - but not harmful.)
• Runtime substitution
  • afform (server-side) MUST replace New Wave with a translation
  • afform (client-side) MUST replace {{ts('Old School')}} with a translation

AFAICS, you have latitude to revise the conditions in isWorthy() so that:

1. It excludes {{ts('Old School')}} (doing the essential behavior)
2. It permits <a>Client #{{theClient.id}}</a>

[shaneonabike]

I was reflecting over the weekend on this and I think that potentially keeping the ability to do {{ts('Required %1', [1 => 'test'])} could be beneficial. Like if people have more complex ts needs, because in some languages variables need to be switched around. Chances are unlikely in most cases, but perhaps we should be doing:

• Warn if it's a regular ts no parameters and not convert it
• Allow ts with variables

[colemanw]

@shaneonabike the other consideration is that the FormBuilder UI currently crashes when the form markup includes ts() functions... so there's that.

[shaneonabike]

haha! @colemanw well if that's the case then let's just deprecate it and then if someone has a good reason to bring it back they can fix the UI and also add it back?

[samuelsov]

Ok, I suggest we change {{ts('Client #%1', {1: theClient.id})}} to Client. In that use-case, it's ok since we have the read-only Client ID field below so the information is not lost.

For the future, I think it might be a good idea to support that but it's not a big priority. One way we could solve this is to convert the syntax to something like that (transifex friendly but would require an extra process for ts):

<li role="tab" ng-repeat="theClient in theClients.result" ng-class="{active: selected.tab === 'client_' + theClient.id}"><a ng-click="selected.tab = 'client_' + theClient.id" ts-params="{ 1: theClient.id }">Client #%1</a></li>

But let's do the clean-up first and work on that edge-case next.

[shaneonabike]

I agree with @samuelsov and I think this is fairly merge ready right. We should definitely add this to make the new features work. Then we address this as an edge-case when people need it.

[mattwire]

@samuelsov @colemanw @shaneonabike needs rebase - can this be merged once rebased?

[samuelsov]

@mattwire I think we are waiting for this to be committed -> civicrm/civistrings#18