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

Refactor localization for not changing UF locale for each string #85

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

jensschuppe
Copy link
Contributor

@jensschuppe jensschuppe commented Jun 6, 2024

This should drastically improve performance for localized remote event forms with many fields/many field options.

Testing should involve a requested locale different from the current default locale (i.e. CiviCRM running in English, requested locale being another language).

This makes use of some code copied from Core's ts() function to make it work with locales other than the current default one.

Since localization might get very slow, this should be cherry-picked for the 1.2.x branch as well.

systopia-reference: 25219

Copy link
Contributor

@TychoSchottelius TychoSchottelius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I can't help with the review, but I have carried out a test with the an environmental registration form and can confirm that the form now loads much faster.

@jensschuppe
Copy link
Contributor Author

Unfortunately, I can't help with the review, but I have carried out a test with the an environmental registration form and can confirm that the form now loads much faster.

Exactly what we needed, thanks for that!

Let's wait for a code review though, as this copies some code from Core's ts() function.

@jensschuppe
Copy link
Contributor Author

Noting that this approach might cause side effects when using native GetText, as in this case instantiating a new \CRM_Core_I18n object with a locale differing from the globally defined one causes \CRM_Core_I18n::setNativeGettextLocale() be called, which calls

// CRM-11833 Avoid LC_ALL because of LC_NUMERIC and potential DB error.
setlocale(LC_TIME, $locale);
setlocale(LC_MESSAGES, $locale);
setlocale(LC_CTYPE, $locale);

and does not reset afterwards, so within the same request the latest processed locale gets stuck for concurrent translations.

With the PhpGetText this is capsuled in the \CRM_Core_I18n object and should do no harm.

@jensschuppe
Copy link
Contributor Author

Also, when using a customTranslateFunction setting, this will always translate to the configured locale instead of the requested one, as those functions do not take a specific parameter for the target locale.

@jensschuppe
Copy link
Contributor Author

Actually, calling \CRM_Core_I18n::setLocale() would still be the correct thing to do, but only once for each CiviRemote Event request instead of for each string …

@jensschuppe jensschuppe added enhancement New feature or request status:needs review Code needs review and testing labels Jun 14, 2024
@jensschuppe jensschuppe added this to the 1.2 milestone Jun 14, 2024
@jensschuppe jensschuppe merged commit bf56913 into master Jun 14, 2024
@jensschuppe jensschuppe deleted the localization branch June 14, 2024 12:45
CRM/Remoteevent/Localisation.php Show resolved Hide resolved
CRM/Remoteevent/Localisation.php Show resolved Hide resolved
CRM/Remoteevent/Localisation.php Show resolved Hide resolved
CRM/Remoteevent/Localisation.php Show resolved Hide resolved
@jensschuppe jensschuppe added status:fixed The issue has been resolved (usually by committing/merging code) and removed status:needs review Code needs review and testing labels Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request status:fixed The issue has been resolved (usually by committing/merging code)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants