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

CRM_Core_I18n - Provide a better label for new/unknown locales #17021

Merged
merged 1 commit into from
Apr 8, 2020

Conversation

totten
Copy link
Member

@totten totten commented Apr 8, 2020

Overview

Mixing the current l10n data with a stable civicrm-core codebase leads to warnings/failures about the locale nl_BE (even if you don't use that locale). This patch aims to make civicrm-core more forward-compatible as l10n evolves.

Suppose you add data-files for a new language/locale in the l10n folder - and then you navigate to the screen civicrm/admin/setting/localization?reset=1. How is this new language presented?

Before

The new language appears in the admin UI as a blank item.

Screen Shot 2020-04-07 at 9 12 38 PM

Every page in the app displays a warning about the unrecognized locale, even if your system never uses that locale.

When running automated tests, the warning can lead to a large number of false-negatives.

After

The new language appears in the admin UI with a placeholder name (based on the code).

Screen Shot 2020-04-07 at 9 12 12 PM

Also, the pervasive warning goes away.

Comment

We've just had an issue where a new language (nl_BE) was added to the l10n data-set, and then all automated test-suites for all versions (incl 5.25 RC and 5.24 stable) started to throw numerous errors (about the unrecognized locale) on unrelated PRs. The issue is that l10n data is generally evergreen (so new languages can be added at anytime), but each branch/tag of civicrm-core has multiple hard-coded language lists (which locks in the list for a given release).

Mixing the current l10n with a stable civicrm-core leads to the warnings/failures. This patch tries to make the symptom less painful when mixing/matching different revisions of civicrm-core and l10n.

Overview
--------

Suppose you add new/unrecognized data files in the `l10n` folder - and then
you navigate to the screen `civicrm/admin/setting/localization?reset=1`.

Before
------

The new language appears in the admin UI as a blank item.

Every page in the app displays a warning about the unrecognized locale.

After
-----

The new language appears in the admin UI with a placeholder name (based on the code).

The warnings are not displayed.

Comment
-------

We've just had an issue where a new language was added to the `l10n`
data-set, and then all automated test-suites for all versions (incl `5.25`
RC and `5.24` stable) started to throw blocker errors on unrelated PRs.
This is because the `l10n` data is generally evergreen, but each branch/tag
of `civicrm-core` has the list hard-coded in multiple spots.

This patch tries to make the symptom less painful when mixing/matching
different revisions of `civicrm-core.git` and `l10n.git`.
@civibot
Copy link

civibot bot commented Apr 8, 2020

(Standard links)

@civibot civibot bot added the 5.25 label Apr 8, 2020
@totten
Copy link
Member Author

totten commented Apr 8, 2020

ping @demeritcowboy @seamuslee001

@demeritcowboy
Copy link
Contributor

This sounds good and it works, and then I can just make the upgrade script against master.

I briefly thought maybe it should append the word Pending or Experimental or something so people don't try to edit it but I don't think that would happen often since they'd have to figure out they'd need to add an option value. Oops I just told them.

@seamuslee001
Copy link
Contributor

I'm going to merge this and we can backport this to 5.24

@seamuslee001 seamuslee001 merged commit 9e4f1a4 into civicrm:5.25 Apr 8, 2020
@totten totten deleted the 5.25-language-labels branch April 9, 2020 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants