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

(dev/core#3979) Localization - Assign a locale for UF during negotiation #24996

Merged
merged 3 commits into from
Nov 23, 2022

Conversation

totten
Copy link
Member

@totten totten commented Nov 18, 2022

Overview

For: https://lab.civicrm.org/dev/core/-/issues/3979
Related: https://lab.civicrm.org/dev/translation/-/issues/78

Before

If you make an APIv4 call with setLanguage('en_NZ'), it negotiates an active locale for various layers (Civi's tsLocale and dbLocale, the currency-formatter, and so on). However, it neglects to choose a locale for the UF.

Consequently, it calls CRM_Utils_System::setUFLocale(NULL) which produces PHP warnings in some environments (eg PHP 8.1 and D9(?)).

After

If you make an APIv4 call with setLanguage('en_NZ'), it negotiates an active locale. It now chooses the UF locale as well.

Consequently, it calls CRM_Utils_System::setUFLocale('en_NZ') which does not produce those PHP warnings.

Comments

The implementation is a bit wrong, but I suspect it's not consequential. Want to figure out testing before we geeking out on that.

The test will probably fail because the test sites don't have localization data for UFs. @demeritcowboy, is there some magic drush incantation to get the localization data for a couple example locales? (Some Civi tests use de_DE and fr_FR, so de and fr might be handy...)

Note: On current D7, this doesn't actually reproduce the message because `bootstrap.inc` redirects it

```
function _drupal_bootstrap_configuration() {
  // Set the Drupal custom error handler.
  set_error_handler('_drupal_error_handler');
  set_exception_handler('_drupal_exception_handler');
```

But if you comment out `set_error_handler()`, then it does. And I presume it would on D8+...
@civibot
Copy link

civibot bot commented Nov 18, 2022

(Standard links)

@civibot civibot bot added the 5.56 label Nov 18, 2022
@demeritcowboy
Copy link
Contributor

I don't know about drush but for drupal 7 https://ftp.drupal.org/files/translations/7.x/drupal/drupal-7.x.de.po will give you a "recent" version of the DE one.

@totten
Copy link
Member Author

totten commented Nov 22, 2022

Looks like that test passes now.

Aside: As someone who hadn't used drupal.org's translations before, this felt like a bit of rabbit hole. Here were some observations/comments along the way:

  • All Drupal variants (7/8/9/BD) distribute strings for the main application+modules as *.po files, and they also support web-based editing of translations. Serving both goals requires an "import" mechanism.
  • The workflow for adding a language is roughly:
    1. Add the language to a list of supported languages (eg drush lanugage-add fr).
    2. Download some *.po files (e.g. curl https://ftp.drupal.org/files/translations/7.x/drupal/drupal-7.x.de.po, as Dave noted).
    3. Import each *.po file to the Drupal DB (eg drush language-import-translations fr /path/to/some/*.po).
  • The l10n_update module sort of automates the download+import, but it's aimed at long-lived production sites and web-based admins. You can't re-use previous downloads across different builds (which is handy for ephemeral test sites in a matrix and for offline dev). For civibuild, I wound up using curl (with a local file-cache) and drush-language.
  • There are small bits of dephell (e.g. drush-language plugin for D7 doesn't seem to work on BD or D8; e.g. l10n_update moved around between versions).
  • After updating the D7 builds, I took a quick stab at BD/D8+ and got them to download the matching PO files, but I didn't work through the dephell for using drush-language.
  • New D7 test builds will include some localization data. They will use the same locales as civicrm-core (per CIVICRM_LOCALES) or it will just do de_DE (required for this test).
  • If you have an existing D7/BD/D8+ build and want to add locales in a way that matches civibuild, then use a command like one of these:
    civibuild run dmaster --eval 'drupal7_po_download de_DE,fr_FR,nl_NL drupal-7.x && drupal7_po_import'
    civibuild run dmaster --eval 'drupal7_po_download ${CIVICRM_LOCALES:-de_DE} drupal-7.x && drupal7_po_import'
    civibuild run bcmaster --eval 'backdrop_po_download ${CIVICRM_LOCALES:-de_DE} drupal-7.x && backdrop_po_import'
    civibuild run drupal9-clean --eval 'drupal8_po_download ${CIVICRM_LOCALES:-de_DE} drupal-9.4.x && drupal8_po_import'
    

@demeritcowboy
Copy link
Contributor

Thanks! Of course I don't seem to have recorded where this came up which would have been helpful. I'll put merge ready and check my notes.

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Nov 22, 2022
@totten
Copy link
Member Author

totten commented Nov 22, 2022

I'm not sure if there's a common scenario, and you probably only had a warning. (At least, that's what tdev/core#3979 emphasized.)

Hypothetically, this probably fixes a bug in the following scenario:

  1. Setup a site with an admin-user who speaks English
  2. Enable "Partial Locales" (to communicate in a wide range of locales)
  3. Enable "Message Admin"
  4. Install a Drupal module that defines some custom Civi tokens (e.g. {contact.say_hello}). As part of the token computation, it should use Drupal's localization APIs (e.g.t() as in t('Hello')).
  5. In "Message Templates", pick a system workflow message and translate it. Be sure to include the custom token. Use the "Preview" dialog to see how it appears when rendered in that language.
    • Without this patch, {contact.say_hello} would always show "Hello" (because that is the admin-user's locale -- and we haven't asked Drupal to change locale).
    • With this patch, t('Hello') would be translated to the match the message.

@demeritcowboy
Copy link
Contributor

The situation was I was trying to debug why cdntaxreceipts wasn't translating. The rest of that story is a bit irrelevant here but there's a part where it calls setLocale. It turned out this null business wasn't the issue, but I obviously felt it worth noting, and it must have made me wonder why it wasn't coming up in unit tests which call setLocale plenty.

Where it might come up is the way drupal does that language url redirection on its side, but I don't use that anywhere. I.e. maybe it would end up reverting to english in some spots. And maybe a question for another day is why does it even try to set the cms lang - possibly as you've noted for cms modules implementing civi things? Except also as noted it doesn't even try for non-drupal.

So anyway this seems to work fine.

@demeritcowboy demeritcowboy merged commit fa9e307 into civicrm:5.56 Nov 23, 2022
@totten totten deleted the 5.56-locale-uf-test branch November 23, 2022 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.56 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