developer.jelix.org is not used any more and exists only for history. Post new tickets on the Github account.
developer.jelix.org n'est plus utilisée, et existe uniquement pour son historique. Postez les nouveaux tickets sur le compte github.

Opened 12 years ago

Closed 12 years ago

#749 closed bug (fixed)

jCoordinator: exception loop when response's constructor fails

Reported by: Surfoo Owned by: Julien
Priority: high Milestone: jelix 1.1
Component: jelix:core Version: 1.1 beta 1
Severity: blocker Keywords:
Cc: Blocked By:
Blocking: Documentation needed: no
Hosting Provider: Php version:

Description

J'ai crée un 2eme module et lors de son appel dans l'URL il y a une exception qui boucle :

Fatal error: Exception thrown without a stack frame in Unknown on line 0

En fait dans mon application, le fichier responses/myHtmlResponse.class.php utilise jLocale sans le sélecteur du module :

...
$this->addMetaDescription(jLocale::get('common.description'));
...

Et vu que dans mon 2eme module il n'existe pas la clé 'common.description' (qui est dans le premier), il devrait y avoir un message d'erreur et non pas une exception qui boucle.

Change History (24)

comment:1 Changed 12 years ago by laurentj

  • Component changed from jelix to jelix:core:jLocale
  • Owner set to Julien
  • Severity changed from normal to blocker

comment:2 Changed 12 years ago by Julien

  • Status changed from new to assigned

comment:3 Changed 12 years ago by Julien

  • Component changed from jelix:core:jLocale to jelix:core

trouvé ! patch en cours

en fait, le problème se produit en mettant

$this->addMetaDescription(jLocale::get('common.description'));

dans le constructeur de la réponse.

S'il ne trouve pas la locale, alors il balance une exception, mais l'exception cherche à obtenir un object réponse pour pouvoir s'afficher, or le construct échoue, donc pas de répons,et plantage à la con.

Ce n'est pas un bug jLocale en tout cas, c'est dans le coordinateur que ça chie.

comment:4 follow-up: Changed 12 years ago by Julien

  • review set to review?
  • Summary changed from Exception en boucle to jCoordinator: exception loop when response's constructor fails

Here's the patch. That was a hard one ton find ;)

comment:5 in reply to: ↑ 4 Changed 12 years ago by Julien

Replying to Julien:

hard one ton find

"to find" of course....

comment:6 Changed 12 years ago by bibo

Tested and tried. good patch ! Just a nit. I would advise to modify jCoordinator, line 297:

$message = 'Double error ! 1)'. $ret.';'."\n".'2)'.$message;

This makes the second and original error appear on a newline. And so is visible at once. Without, the message is lost at the end of line in firefox and visible only by an horizontal zooming.

With that, I give you an r+ but don't know if laurentj wants to also review your patch...

comment:7 Changed 12 years ago by Julien

Hello,

wouldn't the newline produce problems, in logs or somewhat else ?

I also suggest some thing like :

$message = 'Double error ! Error 1:'. $ret.' | Error 2:'.$message;

which may be easier to read, with or without a line break

comment:8 Changed 12 years ago by Julien

I also wonder if we shouldn't switch display order of messages, because in fact, it's the "$message" exception which occurred first.

Error "$ret" is only there because we can't get a response to display the first error.

What do you think about that ?

comment:9 Changed 12 years ago by bibo

Can't think how newlines would hurt log files... Finally, I would re-write $message as below to get rid of ambiguity, speaking about stacked errors more than double errors :

$message = "Stacked errors !\n\t1) ".$ret.';'."\n\t2) ".$message;

comment:10 Changed 12 years ago by Julien

Ok for me with this syntax.

Should I commit, or are we waiting for Laurent's advice ?

comment:11 follow-up: Changed 12 years ago by laurentj

  • review changed from review? to review+

Ok pour le patch, mais pourquoi le constructeur échoue ? Il ne trouve pas la classe ?

comment:12 in reply to: ↑ 11 Changed 12 years ago by Julien

Replying to laurentj:

pourquoi le constructeur échoue ? Il ne trouve pas la classe ?

non, mais Surfoo, qui a reporté le bug, a mis

$this->addMetaDescription(jLocale::get('common.description'));

dans le constructeur de myHtmlResponse. Et c'est vrai que rien n'interdit de le faire, même s'il eu peut-être été préférable de le mettre dans doAfterActions().

Bref, comme son sélecteur de chaîne localisée ne spécifie pas le module, ça utilise le module en cours, ce qui peut effectivement être une approche efficace et sympa.

Sauf si dans un module cette chaîne n'existe pas, ça envoie une exception, exception qui a besoin d'une réponse valide pour s'afficher (s'il n'y a pas d'objet réponse en cours, le code cherche à instancier la réponse par défaut pour le type de requête, soit myHtmlResponse elle-même).

Mais comme c'est au sein même du constructeur que l'exception est lancée, ben il y a une espèce de boucle d'exception (je sais pas si je suis très clair ;) )

comment:13 Changed 12 years ago by laurentj

Ah oui ok, je vois maintenant ce qui se passe :-)

Je confirme donc : ok pour ton patch :-)

comment:14 Changed 12 years ago by Julien

ok, donnes moi encore 2 minutes, je suis en train d'écrire une autre approche, un peu plus sympa.

Le principe : utiliser la réponse "de base" fournie par jelix si possible.

Par exemple, si le constructeur de myHtmlResponse (type "html") donc, lance une exception, alors j'utilise jResponseHtml pour afficher. L'avantage c'est que ça permettra d'avoir la gestion de ces exceptions dans les formats attendus (soap, xml, rss, ....)

je poste dans 2 minutes, puis on décide.

comment:15 Changed 12 years ago by Julien

voilà la variante. C'est peut-être plus sympa dans la mesure où le format de réponse est conservé si possible, via les core responses de jelix

PS : lors du commit, je vais refaire l'indentation du fichier, car il semblerai qu'il y ait des trucs bizarres, komodo me retourne des indentations de 3 espaces au lieu de 4...

comment:16 Changed 12 years ago by Julien

pour l'indentation, en fait juste un espace qui manquait sur la première ligne de la classe...

Changed 12 years ago by Julien

final one, better approach, indent fixed

comment:17 Changed 12 years ago by Julien

  • review changed from review+ to review?

comment:18 follow-up: Changed 12 years ago by laurentj

  • review changed from review? to review+

L'avantage c'est que ça permettra d'avoir la gestion de ces exceptions dans les formats attendus (soap, xml, rss, ....)

C'est déjà le cas avec ton premier patch...

L'inconvénient de ton deuxième patch, c'est qu'on récupère une réponse non personnalisée même pour les warning, notice et cie... Ça le fait pas trop...

Donc r+ mais pour ton premier patch 749-jCoordinator-exception-loop-when-response-constructor-fails.diff

comment:19 in reply to: ↑ 18 Changed 12 years ago by Julien

Replying to laurentj:

L'avantage c'est que ça permettra d'avoir la gestion de ces exceptions dans les formats attendus (soap, xml, rss, ....)

C'est déjà le cas avec ton premier patch...

non justement. avec le dernier patch (dans l'exemple quand on arrive pas à avoir la réponse de type html via la classe myHtmlResponse), il utilise une réponse texte brut.

L'inconvénient de ton deuxième patch, c'est qu'on récupère une réponse non personnalisée même pour les warning, notice et cie... Ça le fait pas trop...

non non. la réponse "de base" pour le type par défaut n'est cherchée que si la réponse personnalisée "échoue"

le schéma est en fait le suivant :

  • exception/erreur/warning/notice/...
    • si j'ai un objet réponse dans mon coordinateur, tout va bien, je l'utilise
    • sinon, je vais chercher la réponse par défaut pour le type de requête dans les réponses,
      • si le type demandé n'est pas défini, alors utilisation d'un format texte brut
      • sinon, on instancie la réponse, qu'elle soit personnalisée ou non
        • si ça se passe bien, hop affichage via cette réponse
        • si ça se passe mal (ça ne peut être le cas qu'avec une réponse personnalisée, car les core responses doivent fonctionner tout le temps), on va chercher la réponse non personnalisée correspondante (core response), et on afficher l'erreur via cette réponse.

en définitive, on ne va chercher la core response que s'il n'y a pas de réponse et que l'instanciation de la réponse personnalisée échoue.

comment:20 Changed 12 years ago by laurentj

ok. r+ for your last patch, if you inverse the test on $originalResponse and if you retrieve the list of responses by reference.

comment:21 Changed 12 years ago by Julien

  • Resolution set to fixed
  • Status changed from assigned to closed

committed in the trunk, r1194

Note: See TracTickets for help on using tickets.