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#3095 Permit setting of format_locale, prefer if set #22885

Merged
merged 4 commits into from
Mar 5, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 3, 2022

Overview

Many sites in Australia, Canada and NZ use 'en_US' as the site language - as there is no practical difference and if the language files are not present then there is no actual option. However, in CiviCRM 5.47 this is used for displaying currency too - and it is non optimal

Before

No option to change language (in some cases)
image

And for my 'Canadian' site....

image

After

Can set format locale & once set....

image

image

Technical Details

This is a minimal patch to address https://lab.civicrm.org/dev/core/-/issues/3095
in time for 5.47. With this set it is possible to change the format locale to
English, Canada or English, Australian (erm & sorta NZ!)
and the currency will only be displayed before dollar amounts NOT of that
currency.

This should be enough to mitigate that regression feeling but missing are

  1. fixing the admin form to hide irrelevant settings if format_locale is set
  2. the psuedoconstant is cludgey - existing stuff doesn't seem to work so
    I added a function - also - if we ARE going to use this option group we
    should .... add NZ to it

Comments

This is a minimal patch to address https://lab.civicrm.org/dev/core/-/issues/3095
in time for 5.47. With this set it is possible to change the format locale to
English, Canada or English, Australian (but not NZ!)
and the currency will only be displayed before dollar amounts NOT of that
currency.

This should be enough to mitigate that regression feeling but missing are
1) fixing the admin form to hide irrelevant settings if format_locale is set
2) the psuedoconstant is cludgey - existing stuff doesn't seem to work so
I added a function - also - if we ARE going to use this option group we
should .... add NZ to it
@civibot
Copy link

civibot bot commented Mar 3, 2022

(Standard links)

@civibot civibot bot added the 5.47 label Mar 3, 2022
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 @colemanw if we can get this into 5.47 I think it's non-regressive - the fiddly parts can follow

CRM/Core/I18n.php Outdated Show resolved Hide resolved
// Hacking in for now since the is probably the most important use-case for
// money formatting in an English speaking non-US locale based on any reasonable
// metric.
$return['en_NZ'] = ts('English - New Zealand');
Copy link
Contributor

Choose a reason for hiding this comment

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

The other ones all have the country in brackets, BUT:

On the larger r-tech, it does feel a little wrong hacking NZ in to the list. It seems like it can just be added as a real option value but without needing translations. It won't appear in the site language dropdown, because there's a built-in check that translation files exist, but it would still be available to the format_locale etc code when you retrieve the list of options values.

What I mean is just remove the hack at line 259, and then add it as a real option value (for testing purposes I just did insert into civicrm_option_value values (null, 84, 'English (New Zealand)', 'en', 'en_NZ', null, 0,0,45, null, 0,0,1,null,null,null,null,null);). That seems to work to get the desired functionality. Of course, for real it would go in the xml file.

totten added 2 commits March 4, 2022 15:46
Overview: Fix display of new setting in admin form.

Before: Field incorrectly displays as multi-value selection.

After: Field displays as single-value selection. The null value is specifically allowed.
@demeritcowboy
Copy link
Contributor

Adding merge on pass from chat conversation https://chat.civicrm.org/civicrm/pl/c8otu63kgbnsmn76uths1jtdjo

@seamuslee001 seamuslee001 merged commit 682ce85 into civicrm:5.47 Mar 5, 2022
@seamuslee001 seamuslee001 deleted the lang branch March 5, 2022 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants