Editing countries.php - how should it work

Editing countries.php - how should it work

by Tim Hunt -
Number of replies: 7
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
I have been working on MDL-19764, and I think it now basically works. At least the original problem, as reported, is now fixed, which will make the Taiwanese very happy.

However, things have changed in Moodle 2.0, so there the scope to do something better there with regards to parent languages and local edits.

How should this work, ideally?


In Moodle 1.9, it works by finding countries.php from either the current language, the parent language or en_utf8, looking in both moodledata/lang and moodle/lang.

Then, it loads the list there, and applies any changes from moodle/lang/<whateverlang>_local/countries.php and moodledata/lang/<whateverlang>_local/countries.php.

As an example of that, fr_ca has parent lang fr. moodledata/lang/fr_ca_utf8/countries.php does not exist, so in Canadian French, the list you get is the one from moodledata/lang/fr_utf8/countries.php, with any amendments found in moodle/lang/fr_utf8_local/countries.php or moodledata/lang/fr_utf8_local/countries.php

However, that is not really consistent with how get_string works. For example, with get_string('CA', 'countries'), it would also look for edits in moodledata/lang/fr_ca_utf8_local/countries.php.

An alternative implementation of get_list_of_countries() do something like:
1. Get the list of known two-letter country codes from the array keys in moodle/lang/en_utf8/countries.php
2. For each of those do get_string(<country code>, 'countries')


So, anyway, I don't know which of those is better, and it is getting late, so I thought I would ask here, and then continue tomorrow.
Average of ratings: -
In reply to Tim Hunt

Re: Editing countries.php - how should it work

by David Mudrák -
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Plugins guardians Picture of Testers Picture of Translators
It's getting late here, too smile However, I would expect it checks moodledata/lang/fr_ca_utf8_local/countries.php before the parent or the parent's local modification. So the search order would be similar to what get_string() does (or at least what I expect it should do, I haven't seen the get_string()'s logic for some time):

  1. moodledata/lang/fr_ca_utf8_local/countries.php
  2. moodle/lang/fr_ca_utf8_local/countries.php
  3. moodledata/lang/fr_ca_utf8/countries.php
  4. moodle/lang/fr_ca_utf8/countries.php
  5. moodledata/lang/fr_utf8_local/countries.php
  6. moodle/lang/fr_utf8_local/countries.php
  7. moodledata/lang/fr_utf8/countries.php
  8. moodle/lang/fr_utf8/countries.php
  9. moodledata/lang/en_utf8_local/countries.php
  10. moodle/lang/en_utf8_local/countries.php
  11. moodledata/lang/en_utf8/countries.php
  12. moodle/lang/en_utf8/countries.php
I like the alternative implementation as it would tie the logic with the get_string() behaviour, whatever it is in the future. But I guess it would remove the possibility to define own language codes in local lang packs, right?

d.
In reply to David Mudrák

Re: Editing countries.php - how should it work

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Irrespective of the details, there are really three key questions about what should be possible.


1. At the moment, you could create a custom language pack, say en_ou, with parent lang en, and in en_ou define just the countries

EN => England
SC => Scotland
WA => Wales
NI => Northern Ireland

Then, in your Moodle, users would only get a choice of those four countries on the sign-up page. Is this useful functionality to keep? (With my alternate proposal, you would have to hack the en language pack to do this.)


2. At the moment, you could add

$string['XX'] = 'The Republic of Moodle';

to en_utf8_loca/countries.php to add that additional choice to the countries dropdown. Should this sort of customisation be possible without hacking en_utf8?


3. At the moment, if you are using fr_ca and you want to customise the countries list, you have to know to edit fr_utf8_local/countries.php. Is that such a big problem?


I think that the answer to those three questions (which hopefully are slightly less technical, and closer to the end user - or at least administrator) really pin down what happens technically in the code.
In reply to Tim Hunt

Re: Editing countries.php - how should it work

by David Mudrák -
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Plugins guardians Picture of Testers Picture of Translators
My opinions (based on real use cases):

  1. yes, admins want to restrict the list of offered countries.
  2. yes, admins want to extend the list of offered countries (like to add counties, districts or whatever, cantons or whatever). This is a potential issue when an user profile with the country value like UK_WLS is backed up and restored at another server. However, it is not so common to transfer users' profile data from server to server, so we should support it.
  3. it's not big problem. However, it is just not intuitive enough and the reason for it is the current implementation, not a conceptual aspect.
In reply to David Mudrák

Re: Editing countries.php - how should it work

by Nicolas Martignoni -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers Picture of Translators

+1 for the three answers of David (also based on real life cases).

In reply to Nicolas Martignoni

Re: Editing countries.php - how should it work

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
I had an alternative thought while cycling into work.

Instead of making the list implicit from the list of keys in countries.php, why not make it explicit.

Either a list

$string['allcountries'] = 'AF,AX,AL,DZ,...';

in lang/en_utf8/langconfig.php. (The idea would be that this would not be copied into other language packs, just like other language packs have parentlang, but en doesn't.)

Or an admin setting

$CFG->allcountries.


I think an admin setting is cleaner, except for upgrades when the ISO list of languages changes.


Then get_list_of_countries would explode that and call get_string a lot. That is clear simple and robust. It would be a little bit slower but not much, and it is only used in one performance critical place, admin/settings/location.php, and that should be changed to do a lazy-load anyway.
In reply to Tim Hunt

Re: Editing countries.php - how should it work

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
sad

I just implemented the $CFG->allcountrycodes solution, and 240 calls to get_string takes 0.1 seconds, which I think it too slow, even for something not performance critical.

I think we need to use the fact that we know these strings can only come from countries.php files in various places to do a more optimal implementation. However, I should be able to make it work in exactly the same way, with $CFG->allcountrycodes. We just need more efficient loading of the strings.
In reply to Tim Hunt

Re: Editing countries.php - how should it work

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
There is now a patch on MDL-19764 that seems to work. My improved implementation takes about the same amount of time as 20 get_string calls, which is plenty fast enough.

Please can someone review that patch. Thanks.