Exclude settings from SearchDisplay exports

[colemanw]

Settings should not be translated as of civicrm/civicrm-core#33901

See civicrm/civicrm-core#35782 for a similar patch to the Api4 Explorer.

• Introduced excludeTs() method to PhpData for skipping translation wrapping on specified keys and their children.
• Updated tests to verify exclusion behavior with nested and multiple keys.

[colemanw]

@totten I don't think the test failures are caused by this PR.

[colemanw]

@totten

[totten]

I don't think the test failures are caused by this PR.

Hmm... yeah, on a quick spot-check, the failures don't exactly make sense for the PR. OTOH, contemporaneous nightly tests are running clean. (example) I guess it needs a closer look...

Settings should not be translated as of civicrm/civicrm-core#33901

Looking at this from downstream developer POV... Does it seem right to say that 33901 represents an inversion of the best-practice for 6.16+?

• Before 6.16, you MUST use E::ts() in the SavedSearch settings if you want -any- i18n support?
• After 6.16, you MUST NOT use E::ts() in the SavedSearch settings if you want -good- i18n support?

Obviously, developers can be circumstantially indifferent re:i18n and re:compatibility. The question arises for developers who are diligent about both. And in this context, it sounds like...

MGD Code	CiviCRM Version	CiviCRM Outcome (Single-lingual i18n)	CiviCRM Outcome (Multi-lingual i18n)
'text' => E::ts('Add Campaign')`	6.15	🟢 Translates OK	🟠 Erratic translation
'text' => E::ts('Add Campaign')`	6.16	🟢 Translates OK	🟠 Erratic translation
'text' => 'Add Campaign'`	6.15	🔴 No translation	🔴 No translation
'text' => 'Add Campaign'`	6.16	🟢 Translates OK	🟢 Translates OK

Is that the game?

[colemanw]

@totten developers who are diligent will know when to add ts themselves. Also, there's no way to mark an extension as compatible with 6.15 but NOT compatible with 6.16 so... I vote we merge this PR as-is and not overthink it.

[totten]

developers who are diligent will know when to add ts themselves...

I really don't think it's that obvious.

there's no way to mark an extension as compatible with 6.15 but NOT compatible with 6.16 so... I vote we merge this PR as-is and not overthink it.

It sounds like you've started thinking about a specific solution/approach/mitigation (re: details of publishing releases with compatibility flags) and started judging that. Maybe that rabbit-hole is good or bad, but right now I'm not in that rabbit-hole.

The place where I'm at: The basic facts look unusual. Did we really invert the best-practice without a bridge? I'm not saying that's good/bad/right/wrong... it's probably good in this case... but it is unusual, and I want to be sure that it really happened.

[colemanw]

@totten I'm glad you're not in that rabbit hole, but it really would have been great to raise these concerns on the original PR that has been open since last October. Better late than never I guess, but instead of having this conversation on a side channel we ought to be discussing on the original PR so the author can participate too.

[colemanw]

/test

[colemanw]

test this please

[colemanw]

Thanks @totten all fixed.

[totten]

Thanks @totten all fixed.

Woot

I'm glad you're not in that rabbit hole, but it really would have been great to raise these concerns on civicrm/civicrm-core#33901 that has been open since last October. Better late than never I guess, but instead of having this conversation on a side channel we ought to be discussing on the original PR so the author can participate too.

Perhaps -- but just to clarify, I'm not trying to litigate an alternative to 33901. Sometimes a change is necessary+acceptable. I don't really see anything that provides interop while leaving the system as clean. (Part of the benefit of 33901 is that the final arrays are actually a bit cleaner...) And in the worst-case, bad choice here doesn't leave you with a broken system -- it just means some of the strings won't be translated.

From a civix POV, I just want to communicate the migration effectively.

Added upgrade step to notify developer and (conditionally) drop all the old E::ts() clauses.