localization system should work with langage code without country

Salut,

une idée : http://jelix.org/forums/read.php?5,1580#msg-1584

et bientôt un patch sur cette idée.

Julien

Voilà le patch.

Finalement, je n'ai pas fait de fusion au moment de la compilation comme je l'évoquais sur le forum.

Détails du patch

jSelectorLoc

j'ai modifié cette classe pour que avoir 1 fichier de cache par locale. Avant, le fichier cache fr_CA écrasait le fichier fr_FR par exemple.

J'ai simplement rajouté la locale dans le nom du fichier cache.

jBundle

j'ai modifié la méthode de compilation, je pense qu'elle devrait aller un peu plus vite (je ne suis pas sûr, mais bon).

Le fichier source n'est plus ouvert en lecture durant toute la compilation.

Le test d'existence du fichier se fait tout au début de la méthode, et lance l'exception qui se trouvait tout à la fin.

Vers la fin de la méthode, j'ai aussi optimisé un elseif(condition){}else{code} en elseif(!condition){code}.

jLocale

J'ai fait pas mal de modifs ici, principalement pour découper les actions en méthodes + petites. C'était plus clair pour effectuer le processus suivant :

la locale demandée est fr_CA

• si c'est disponible, alors tout va bien
• sinon on va chercher dans la locale "générique" française : fr_FR
  • si c'est disponible, alors tout va bien
  • sinon, on va chercher dans la locale par défaut comme avant, sauf si bien entendu fr_FR est déja celle par défaut (on lance alors une exception).
    • si c'est disponible, alors tout va bien
    • sinon on lance une exception

En résumé, j'ai rajouté la recherche dans le locale générique avant de chercher dans la locale par défaut comme c'était le cas avant.

On peut donc surcharger uniquement quelques éléments en fr_CA, le reste sera lu dans fr_FR.

Je crois que le patch résoud l'ensemble de problèmes du ticket.

@+ Julien

Laurent : Amélioration et fix en cours, ne pas faire de review tout de suite

Voici un patch + complet et robuste.

le principe reste le même

• fr_CA surcharge uniquement ce qui est nécessaire sur fr_FR
• le fichier n'a pas besoin d'exister dans fr_CA, le système le cherchera dans fr_FR
• si on demande une chaine en fr_CA et qu'elle n'existe pas (échec des 2 étapes ci-dessus), alors on cherche dans la langue par défaut (par exemple de_AT)
  • si de_AT est ok, alors pas de problème
  • sinon ça va chercher dans de_DE
    • si pour X raisons çà ne marche pas avec de_DE, alors exception.

Pour les messages d'erreurs jelix localisés (jException principalement), je demande de les mettre en dernier recours en en_EN (variable de config internalLocale). On pourrait aussi choisir fr_FR, mais il faut que ce soit une locale générique existante de Jelix, pour toujours arriver avec un message d'exception localisé comme il faut. Normalement le développeur ne doit pas toucher à cette variable de config.

Voilà, ça fonctionne plutot très bien.

NB : le patch est très gros, car apparement les fichiers originaux avaient une fin de ligne <CRLF>, et moi j'ai enregistré avec juste <LF>

@+ Julien

le patch est très gros, car apparement les fichiers originaux avaient une fin de ligne <CRLF>, et moi j'ai enregistré avec juste <LF>

Je préfère un patch avec les CRLF, parce que là il m'est difficile de voir ce que tu as modifié exactement. De plus, si les fichiers ont CRLF, je préfère que tout les fichiers utilisent les même conventions.

Raison supplémentaire : comme ton patch n'a pas été fait avec la toute dernière version des fichiers, je n'arrive pas à l'appliquer (comme dans le patch, la portion à remplacer est tout le fichier, comme ce dernier a été modifié depuis, il ne trouve pas la portion à remplacer..).

Y a toutefois un problème : il y a en effet des fichiers en CRLF et d'autres en LF. Va falloir que je corrige ça :-(

J'ai corrigé tout les fichiers sur le trunk. Ils sont tous en LF. Je vais essayer d'appliquer ton patch.

The same patch updated for the latest trunk

premiers tests unitaires pour le patch

À priori, il y a un problème avec le patch proposé : de simples tests unitaires font planter php chez moi (cf le patch joint pour les tests unitaires), et un des test unitaires existant ne passe plus (celui qui test si on a bien une exception quand le fichier de locale demandé n'existe pas). Il faut donc soit corriger le code pour que le test passe, soit modifier le test, bien que je sois pour la première solution : on doit avoir une exception qui indique quand le fichier de locale n'est pas trouvé.

De plus, quelques remarques sur le code :

     static function get ($key, $args=null, $locale=null, $charset=null) {
         global $gJConfig;
+
+        $selector = self::_getSelector($key, $locale, $charset);
+        $bundle = self::_getBundle($selector);
+        $string = $bundle->get($selector->messageKey, $selector->charset);

et

+    static public function getStringFromDefaultLocale($key){
+        global $gJConfig;
+        $selector = self::_getSelector($key, $gJConfig->locale, $gJConfig->charset);
+        $bundle = self::_getBundle($selector);
+
+        $string = $bundle->get($selector->messageKey, $selector->charset);

Ce sont les deux seuls endroits où tu utilises _getSelector, _getBundle, et $bundle->get : tu peux rassembler _getSelector et _getBundle en une seule méthode, en y ajoutant le $bundle-get.

+    static public function getGenericLocaleForLocale($locale){
+        global $gJConfig;
+        if ($locale === null)  $locale = $gJConfig->locale;
+        return substr($locale,0,2).'_'.strtoupper(substr($locale,0,2));
+    }
+
+    static public function isGenericLocale($locale){
+        global $gJConfig;
+        if ($locale === null)  $locale = $gJConfig->locale;
+        return $locale === substr($locale,0,2).'_'.strtoupper(substr($locale,0,2));
+    }

Ces deux méthodes font pratiquement la même chose. Or à chaque appel de isGenericLocale, tu appels ensuite getGenericLocaleForLocale. Tu fais donc deux fois le même traitement à chaque fois. Il vaut mieux donc faire un getGenericLocaleForLocale, faire ensuite un simple test d'égalité plutôt que d'appeler isGenericLocale (qui n'a donc pas lieu d'exister).

Ensuite, plutôt que faire deux fois un substr, c'est certainement mieux de mettre le résultat dans une variable.

Enfin, j'ai des doutes sur la recursivité

Vu les modifications assez importantes que cela apporte dans le fonctionnement de jLocale, et vu que je veux sortir la RC1 en début de semaine, donc sans avoir le temps d'apporter les corrections nécessaires et avoir une période de tests suffisants, je préfère reporter ce patch pour la 1.1. À moins que Dimanche soir, un patch nikel est fourni avec une tonne de tests unitaires :-) (il faudrait en faire sur jSelectorLoc, compléter ceux sur jBundle et ceux sur jLocale)

oups, j'ai pas fini une phrase : j'ai des doutes sur le bon fonctionnement de la recursivité lors de la recherche d'un fichier de locale ou d'une locale précise. Il y a eu des problèmes (genre boucle infinie ou problème bizarres) par le passé là dessus, et il faut donc des tests complets pour vérifier tout ça.

Salut Laurent,

comme tu l'as vu, il y avait des fichiers CRLF et LF dans Jelix. Je m'étais donc dis que suite à cette remarque, tu ferais la conversion CRLF > LF pour les fichiers concernés ;)

Pour ton test unitaire qui part en boucle infinie (sans doute), l'explication est qu'effectivement je ne lance plus d'exception quand le fichier locale n'est pas trouvé, sauf en dernier recours.

Le problème, c'était que l'exception était elle-même localisée par jLocale, donc ça se mordait la queue, et hop, boucle infine, out of memory, etc...

Lancer une exception c'est bien, mais est-ce utile si on cherche une localisation fr_CA et que fr_FR existe ? Je ne suis pas certain que le programme doit s'arreter pour autant. Et j'avoue que jouer sur les codes d'exception pour les traiter au cas par cas ne m'inspirait pas.

Dans mon patch, j'ai préféré essayer d'obtenir le meilleur fichier locale, et balancer l'exception uniquement en dernier recours.

La récursivité fonctionne d'après les tests que j'ai pu mener, mais je comprends que tu préfères reléguer à + tard.

Pour le patch complet avec les tests, il faudra donc attendre, surtout que je décolle pour Tokyo samedi, pour 15jours ;)

D'ailleurs j'avais pour idée d'encore améliorer le patch, pour que les overloads des locales puissent également être partiels (je surchage juste 2 chaines, les autres sont recherchées dans le fichier original).

Je verrai quand même demain ou mardi si j'ai un peu de temps.

@+ Julien

Salut Laurent,

j'ai vu que tu as mis des tests unitaires.

Est-ce que je dois quand même te faire un patch à partir de la dernière version des fichiers (qui seront donc en LF) ?

Si ça peut t'aider à voir les modifs + simplement.

Sinon, est-ce que d'une façon ou d'une autre, tu es prêt à intégrer le patch dans Jelix 1.0 ?

Car je crois que sans ce patch, on aura le problème suivant :

si je dis que la locale par défaut c'est de_DE, alors les messages d'erreurs localisés de Jelix ne pourront être affichés, et il y a même un risque de tomber dans une boucle infinie :

L'exception de jSelectorLoc est localisée avec jLocale, et fait donc appel à un jSelectorLoc. Donc récursivité à ce niveau là.

Si le jSelectorLoc lance une exception (car le fichier n'est pas trouvé) même pour la langue par défaut (ici de_DE), est-ce que ça va pas se mordre la queue ?

Je me demande si ça ne peut pas se produire.

Je vais tester dans la journée, à partir du trunk. Si y a le problème, je ré-écris mon patch sur le trunk, pour que tu puisse éventuellement l'intégrer en 1.0 (car si le problème que j'évoque se confirme, ce sera plus un bugfix qu'une évoluotion).

@+ Julien

Salut Laurent,

bon, c'est bien ce que je craignais :

• créé une appli "test" avec la RC1 (rev 712)
• appelle index.php : [error 130] Module "test" inconnu ou désactivé < OK, tout va bien
• change la locale dans le fichier de config pour "de_DE" (le gars veut un site allemand par défaut)
• appelle index.php : out of memory, boucle infinie

donc ça bug :(

je refais donc mon patch à partir de la rev 712 (comme ça on aura le vrai diff, cf prob CRLF), je le re-teste et je te le soumets.

NB : j'augmente la sévérité et tout le toutim, histoire de traiter ça encore avant mon départ chez les sushi samedi (avant vendredi soir donc).

Non, le ticket concerne une évolution, donc on le laisse comme tel. Si il y a bug, on crée un ticket spécifique à ce bug -> ticket #387.

Je n'incluerais donc pas ton patch conçernant cette evolution pour la 1.0. C'est trop délicat cette histoire de locale (ex: le bug trouvé). Je pense qu'il est beaucoup plus important de corriger juste le bug que tu as trouvé.

Conçernant ton patch initial, tu n'as apparement pas vu que je l'avais refait à partir du trunk **après** avoir corrigé les CRLF. cf les pieces jointes et mes commentaires plus haut.

Pour les tests unitaires, je n'ai pas intégré dans le trunk, j'ai juste intégré les fichiers de locales à utiliser dans les tests unitaires. Donc dans le fichier ticket_95.unit_tests.patch, il y a juste le diff sur core.jlocale.html.php à prendre en compte.

Replying to laurentj:

Non, le ticket concerne une évolution, donc on le laisse comme tel. Si il y a bug, on crée un ticket spécifique à ce bug -> ticket #387.

ok ;)

Je n'incluerais donc pas ton patch conçernant cette evolution pour la 1.0. C'est trop délicat cette histoire de locale (ex: le bug trouvé). Je pense qu'il est beaucoup plus important de corriger juste le bug que tu as trouvé.

Yup je comprends. Comme le patch global permettait de corriger le bug que j'ai confirmé ce matin, je me disais qu'on aurait pou tout mettre dedans éventuellement.

Je vais proposer le patch qui fixe juste le bug du ticket #387, ce sera une partie de mon patch.

Conçernant ton patch initial, tu n'as apparement pas vu que je l'avais refait à partir du trunk **après** avoir corrigé les CRLF. cf les pieces jointes et mes commentaires plus haut. Pour les tests unitaires, je n'ai pas intégré dans le trunk, j'ai juste intégré les fichiers de locales à utiliser dans les tests unitaires. Donc dans le fichier ticket_95.unit_tests.patch, il y a juste le diff sur core.jlocale.html.php à prendre en compte.

Euh, je n'avais pas regardé dans les détails, et ça m'avait échappé que tu avais refais le patch à partir du trunk.

De toute façon comme on va découper le patch à présent...

RDV sur le ticket 387 pour le bugfix, puis on reviendra sur ce ticket pour les améliorations fonctionnelles.

Ça marche, merci :-)

Il y a des remontées de problèmes de boucle infinie dans le fichier properties n'existe pas dans la locale. Ça devient urgent donc de corriger :-) (peut-être pour la 1.0.3 ? on verra)

Hello,

je vais revoir pour ré-écrire le patch posté à l'époque, qui fonctionnait, mais qui avait modifié trop profondement le code pour être intégré juste avant la release 1.0. D'ailleurs les tests unitaires ne passaient plus, car je ne pouvais plus envoyer d'exception localisée.

Comme on a plus ou moins réglé ça dans le ticket #387, je vais pouvoir a priori ré-ecrire le patch sans bazarder les test unitaires.

Pour des raisons de comodité, je vais sans doute fixer le ticket #442 en même temps, pour ne pas avoir une ecriture de code en 2 fois (car il s'agit bien du même problème, que ce soit sur la langue ou le charset).

ok super :-)

Hello, j'ai fait des tests et j'arrive à un résultat satisfaisant pour ce ticket :

si on spécifie juste "fr" comme locale, alors c'est "fr_FR" qui est utilisé (ça c'était déja inclus dans le code actuel

si on spécifie "fr_CA" et que le fichier n'existe pas, alors ça cherche dans "fr_FR". Si ça trouve, on a donc la localisation en fr_FR. Si ça ne trouve pas, on a une exception du type : (200)The given locale key "appdev~test.machaine" is invalid (for charset UTF-8, lang de_AT) car je n'ai ni la locale de_AT, ni la de_DE.

Grâce au patch de Laurent sur le #387, on n'a plus de risque de boucle infinie.

Oui, cette modification fera normalement échouer le test unitaire qui vérifie qu'une exception est bien levée si la locale n'existe pas.

Il faut peut-être envisager que l'erreur "fatale" ne se produit que si on a pas de locale "générique" (fr_FR, de_DE, ...) et émettre un simple warning/notice à l'attention du développeur pour les locales non existantes du type fr_CA, en_US, ... ? Et encore, il faudrait envisager (j'y reviendrais dans un autre ticket pour une amélioration) que le développeur ne crééra pas la locale fr_CA intentionnellement et en toute connaissance de cause, c'est à dire qu'il ne le fera que lorsqu'une chaine change par rapport à la locale générique fr_FR.

L'exemple parlant : les messages internes de jelix sont localisés en en_EN et en_US. Avec une telle amélioration, on pourrait tout simplement zapper le coté en_US, sauf quand en_US != en_EN.

Je sais pas si je suis bien clair, mais en gros la question c'est : est-ce qu'il n'est pas uniquement nécessaire de générer une exception quand c'est une locale GENERIQUE qui n'est pas trouvée ?

Je ferai le patch ce soir.

Voici le patch fonctionnel.

Je regarde encore du coté du test unitaire qui est peut-être à modifier.

erreur dans le chemin de base du diff

Les tests unitaires de testapp passent au niveau de jLocale.

+        $generic_locale = substr($this->locale,0,2).'_'.strtoupper(substr($this->locale,0,2));

Plutôt que de faire 2 substr, peut-être mettre le résultat dans une variable temporaire

+            if (is_readable ($path)){
+				$this->_where = 'modules/';
+				$this->_path = $path;
+				$this->_cacheSuffix = '.'.$locale.'.'.$this->charset.'.php';
+				return;
+			}
         }
-        $this->_where = 'modules/';
+

Attention, l'indentation ici est fait de tabulation, non d'espaces

+        // in the specific lang, so we retrieve it in en_EN language
+        if($this->toString() == 'jelix~errors.selector.invalid.target')
+            $l = 'en_EN';
+        else
+            $l=null;
+        throw new jExceptionSelector('jelix~errors.selector.invalid.target', array($this->toString(), "locale"),1,$l);

Cette portion de code a changé (ticket #442), il va donc falloir la mettre à jour.

Fait un checkin du ticket #442 dans le trunk et la branche avant de refaire ton patch. Ton patch doit inclure aussi les modifications dans les tests unitaires : des tests doivent être décommenté (dans les methodes testWithNoAskedLocale*). D'ailleurs il y en a un qui ne passent pas.

On va y arriver :-)

Ticket #442 is now fixed, so this ticket is not blocking anymore

don't remove blocking value (for history), unless it is an error :-)

Hello,

I had to remove blocking to be able to close #442 (it was not possible to close the ticket).

It was quite late, so I may have missed something about that ?

oh, ok... sorry. You didn't miss something. This is just a bug of Trac (in fact, the plugin which add blocking feature)

enfin le bon patch ? ;)

Hello,

version corrigée du patch.

Tests unitaires : j'en ai juste activé 1 car ce patch n'est pas censé les passer tous.

Le patch permet de régler le problème suivant : si le fichier n'existe pas pour fr_CA, alors il va chercher dans fr_FR.

Pour permettre de prendre la clé de locale fr_FR si elle n'existe pas dans le fichier fr_CA (fichier qui existe), alors c'est plutôt du coté du ticket #489. Là on pourra activer le 2eme test unitaire de la méthode testWithNoAskedLocale().

Pour qu'il y ait un fallback vers une "last-chance locale" (en_EN dans les tests unitaires), alors ce sera encore un autre patch, mais j'avoue que je m'interroge sur l'utilité d'un tel système au final (même si je l'ai suggéré au départ). A mon avis, si on demande du fr_CA et que ça n'existe ni en fr_CA ni en fr_FR, alors il vaut mieux sortir une exception comme actuellement (il y a de grandes chances que la locale n'existe pas non plus en en_EN dans l'appli).

Je pense donc que les tests du type

// no test3.properties file for fr_CA and fr_FR, so we should have the en_EN one
$GLOBALS['gJConfig']->locale = 'en_EN';
$this->assertEqual('this is an en_EN sentence test3',jLocale::get('tests3.first.locale', null, 'fr_CA'));
$this->assertEqual('this is an en_EN sentence test3',jLocale::get('tests3.first.locale', null, 'fr_FR'));
$GLOBALS['gJConfig']->locale = 'fr_FR';

ne seront jamais activés, car ce ne sera pas une fonctionnalité qu'on intègrera (ou du moins pas tout de suite, car cela ferai a priori échouer d'autres tests, notamment ceux qui testent si une exception survient quand on demande une locale qui n'existe pas, cf. commentaire 12 de ce ticket).

It's ok

ok, commit demain soir, là trop KO ;)