Incohérence du type de l'argument contexte pour fonctions urls_decoder_url et urls_page_dist #4981

Open
opened 2 weeks ago by JLuc · 8 comments
JLuc commented 2 weeks ago

La fonction urls_decoder_url reçoit un argument $contexte qui est un tableau associatif. Elle peut aussi être appelée sans $contexte, par exemple par form_hidden qui fait $p = urls_decoder_url($action, ''). Dans ce cas, le $contexte prend sa valeur par défaut : [] un tableau vide.

Cette fonction calcule une fonction $renommer. Par défaut, en l'absence de 'type', cette fonction c'est urls_page_dist, qu'elle soit calculée par generer_url_entite('', '', '', '', true); (voir ici) ou via $renommer = charger_fonction('page', 'urls');.

Puis cette fonction est appellée : $a = $renommer($url, $fond, $contexte);.

Or l'argument $contexte est un tableau, alors que la fonction urls_page_dist appelée attend une chaine en argument.

Ça semble indiquer un problème, qui se confirme par la suite.
C'est en voulant implémenter des urls raccourcies sur un objet que je tombe là dessus (/123 est l'url publique visible et présente la page de l'objet 123), mais le problème semble plus général.

Là, si le premier argument ($url) est un entier, alors c'est _generer_url_page qui prend immédiatement la main, avec la même attente de chaine concernant son 3eme argument, $args, qui reçoit le tableau $contexte, et qui finit par le concatèner aux autres chaînes reçues, générant un warning en php7.3.

PS1 : c'est assez compliqué vu la variété des cas prévus, les surcharges possibles en pas mal d'endroits, des fonctions réparties dans 4 ou 5 fichiers, la compatibilité avec des schémas d'urls historiques...
PS2 : du coup ces fonctions mériteraient une meilleures documentation

[La fonction `urls_decoder_url`](https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/urls.php#L59) reçoit un argument `$contexte` qui est un tableau associatif. Elle peut aussi être appelée sans `$contexte`, par exemple [par `form_hidden`](https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/filtres.php#L2725) qui fait `$p = urls_decoder_url($action, '')`. Dans ce cas, le `$contexte` prend sa valeur par défaut : `[]` un tableau vide. Cette fonction calcule une fonction $renommer. Par défaut, en l'absence de 'type', cette fonction c'est `urls_page_dist`, qu'elle soit calculée par `generer_url_entite('', '', '', '', true);` ([voir ici](https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/utils.php#L1807)) ou via [ `$renommer = charger_fonction('page', 'urls');`](https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/urls.php#L113). Puis cette fonction est appellée : `$a = $renommer($url, $fond, $contexte);`. Or l'argument `$contexte` est un tableau, alors que [la fonction `urls_page_dist` ](https://git.spip.net/spip/spip/src/branch/master/ecrire/urls/page.php#L53) appelée attend une chaine en argument. Ça semble indiquer un problème, qui se confirme par la suite. C'est en voulant implémenter des urls raccourcies sur un objet que je tombe là dessus (/123 est l'url publique visible et présente la page de l'objet 123), mais le problème semble plus général. Là, si le premier argument ($url) est un entier, alors c'est [`_generer_url_page`](https://git.spip.net/spip/spip/src/branch/master/ecrire/urls/page.php#L31) qui prend immédiatement la main, avec la même attente de chaine concernant son 3eme argument, `$args`, qui reçoit le tableau `$contexte`, et qui finit par le concatèner aux autres chaînes reçues, générant un warning en php7.3. PS1 : c'est assez compliqué vu la variété des cas prévus, les surcharges possibles en pas mal d'endroits, des fonctions réparties dans 4 ou 5 fichiers, la compatibilité avec des schémas d'urls historiques... PS2 : du coup ces fonctions mériteraient une meilleures documentation
b_b commented 2 weeks ago
Owner

J'ai pas tout compris (ou presque rien), mais je pense que ça vaudrait le coup de donner l'erreur complète générée :)

PS0 : c'est en voulant implémenter des urls raccourcies sur un objet que je tombe là dessus. Genre /123 est l'url publique canonique durable et amène à la page de l'objet 123.

Ça risque de clasher avec ce que fait déjà SPIP par défaut /123 renvoie vers l'article 123, et de mémoire ça se gère directement depuis le htaccess sans se prendre la tête ;)

J'ai pas tout compris (ou presque rien), mais je pense que ça vaudrait le coup de donner l'erreur complète générée :) > PS0 : c'est en voulant implémenter des urls raccourcies sur un objet que je tombe là dessus. Genre /123 est l'url publique canonique durable et amène à la page de l'objet 123. Ça risque de clasher avec ce que fait déjà SPIP par défaut /123 renvoie vers l'article 123, et de mémoire ça se gère directement depuis le htaccess sans se prendre la tête ;)
b_b added the
bug
label 2 weeks ago
b_b added this to the 4.1 milestone 2 weeks ago

Je sais que j'ai pas mal galéré dans le plugin "Urls par numero" mais que j'y était tant bien que mal arrivé

Je sais que j'ai pas mal galéré dans le plugin "Urls par numero" mais que j'y était tant bien que mal arrivé
Poster

@b_b oui /123 redirige vers la bonne page dans le spip de base mais c'est une R301 et donc l'url n'est pas préservée. Dans ce que j'ai fait, ce n'est pas une R301 et ça reste l'url visible et j'ai défini un nouveau type d'url 'courtes' pour pouvoir définir la fonction urls_generer_url_monobjetcourt_dist qui produit l'url /123.
(et inversement le htaccess redirige une éventuelle url page classique vers /123 en R301, mais sauf erreur ou traînage de vieilles urls sur le net il n'y a pas de raison d'y avoir de telles urlpages classiques)

@maieul ah je savais pas que ça existait. Je vais regarder. Merci.

Actuellement dans mon petit cas perso, je crois avoir remédié à ce problème en recopiant aussi les fonctions _generer_url_page et urls_page_dist renommée url_courtes_dist dans le fichier urls/courtes.php et en y ajoutant 2 lignes au tout début de url_courtes_dist :

if ($i and !$entite) {
	$entite = 'monobjetcourt';
}

Du coup _generer_url_page appelle generer_url_monobjetcourt et renvoie son résultat, le code n'arrive pas à la concaténation de chaines et de tableau foireuse à la fin.

Reste donc concernant spip un problème de flux de l'argument $contexte, que le typage des arguments des fonctions concernées signalerait surement comme une erreur en amont (ou bien un outil d'analyse du code si ces outils pouvaient pas sans plus d'information analyser les flux de fonctions variables et les surcharges _dist auxquelles spip a recours).

@b_b oui /123 redirige vers la bonne page dans le spip de base mais c'est une R301 et donc l'url n'est pas préservée. Dans ce que j'ai fait, ce n'est pas une R301 et ça reste l'url visible et j'ai défini un nouveau type d'url 'courtes' pour pouvoir définir la fonction `urls_generer_url_monobjetcourt_dist` qui produit l'url /123. (et inversement le htaccess redirige une éventuelle url page classique vers /123 en R301, mais sauf erreur ou traînage de vieilles urls sur le net il n'y a pas de raison d'y avoir de telles urlpages classiques) @maieul ah je savais pas que ça existait. Je vais regarder. Merci. Actuellement dans mon petit cas perso, je crois avoir remédié à ce problème en recopiant aussi les fonctions `_generer_url_page` et `urls_page_dist` renommée `url_courtes_dist` dans le fichier `urls/courtes.php` et en y ajoutant 2 lignes au tout début de `url_courtes_dist` : ``` if ($i and !$entite) { $entite = 'monobjetcourt'; } ``` Du coup `_generer_url_page` appelle `generer_url_monobjetcourt` et renvoie son résultat, le code n'arrive pas à la concaténation de chaines et de tableau foireuse à la fin. Reste donc concernant spip un problème de flux de l'argument $contexte, que le typage des arguments des fonctions concernées signalerait surement comme une erreur en amont (ou bien un outil d'analyse du code si ces outils pouvaient pas sans plus d'information analyser les flux de fonctions variables et les surcharges `_dist` auxquelles spip a recours).

Est-ce que tu pourrais au moins renommer le titre, avec un verbe, une action ? Là je n'ai toujours pas compris s'il y a un bug, quel est le problème ou le manque. Même si c'est "Documenter l'utilisation de… " ou je sais pas, mais là ça veut vraiment rien dire le ticket.

Est-ce que tu pourrais au moins renommer le titre, avec un verbe, une action ? Là je n'ai toujours pas compris s'il y a un bug, quel est le problème ou le manque. Même si c'est "Documenter l'utilisation de… " ou je sais pas, mais là ça veut vraiment rien dire le ticket.
JLuc changed title from contextes pour urls_decoder_url et urls_page_dist to Incohérence du type de l'argument contexte pour fonctions urls_decoder_url et urls_page_dist 2 weeks ago
Poster

Ok J'ai mis incohérence de type de l'argument contexte car c'est le symptome que relève une analyse du code, mais c'est lié au mésusage des fonctions concernées.

Certaines de ces fonctions ont une documentation, mais pas toutes et ça ne couvre pas l'ensemble du process. Évidemment ça n'aide pas.

Heureusement, ça ne porte à conséquence que dans des cas particuliers. Les problèmes doivent être dans du code mort en général, mais pas dans ces cas particuliers.

Ok J'ai mis **incohérence de type de l'argument contexte** car c'est le symptome que relève une analyse du code, mais c'est lié au **mésusage des fonctions concernées**. Certaines de ces fonctions ont une documentation, mais pas toutes et ça ne couvre pas l'ensemble du process. Évidemment ça n'aide pas. Heureusement, ça ne porte à conséquence que dans des cas particuliers. Les problèmes doivent être dans du code mort en général, mais pas dans ces cas particuliers.
Owner

Alors ce qui est mal compris c'est que il n'est pas prévu/possible d'avoir des urls propres numériques : les fonctions urls_truc() considèrent que si le premier argument est numérique c'est qu'on veut générer l'url de l'entité correspondante, pas qu'on veut la décoder.

En ce qui concerne le décodage, il est bien prévu que $args soit bien tableau ou une chaine, cf
https://git.spip.net/spip/spip/src/branch/master/ecrire/urls/page.php#L64

On pourrait sans doute améliorer le test
https://git.spip.net/spip/spip/src/branch/master/ecrire/urls/page.php#L54
en ajoutant un and !is_array($args) qui permettrait pour le coup de décoder des urls numériques, mais je pense que le mieux serait de refondre l'API en séparant la fonction de décodage d'une URL de la fonction de génération d'une URL

(quitte à supporter en fallback une fonction point d'entrée unique pour compat avec l'ancienne API)

Alors ce qui est mal compris c'est que il n'est pas prévu/possible d'avoir des urls propres numériques : les fonctions urls_truc() considèrent que si le premier argument est numérique c'est qu'on veut générer l'url de l'entité correspondante, pas qu'on veut la décoder. En ce qui concerne le décodage, il est bien prévu que `$args` soit bien tableau ou une chaine, cf https://git.spip.net/spip/spip/src/branch/master/ecrire/urls/page.php#L64 On pourrait sans doute améliorer le test https://git.spip.net/spip/spip/src/branch/master/ecrire/urls/page.php#L54 en ajoutant un ` and !is_array($args)` qui permettrait pour le coup de décoder des urls numériques, mais je pense que le mieux serait de refondre l'API en séparant la fonction de décodage d'une URL de la fonction de génération d'une URL (quitte à supporter en fallback une fonction point d'entrée unique pour compat avec l'ancienne API)

Oui je crois qu'on ne sortira pas de ce bazar tant qu'on gardera le fourre-tout de fonctions qui font une chose et son contraire dans la même fonction (j'ai jamais compris pourquoi ce choix d'architecture).

(Quitte à refondre, autant passer à un vrai routeur d'URL extensible comme sf/routing :p )

Oui je crois qu'on ne sortira pas de ce bazar tant qu'on gardera le fourre-tout de fonctions qui font une chose et son contraire dans la même fonction (j'ai jamais compris pourquoi ce choix d'architecture). (Quitte à refondre, autant passer à un vrai routeur d'URL extensible comme sf/routing :p )
JLuc commented 1 week ago
Poster

Tentative de synthèse :

  • le cas des urls /123 n'est pas prévu mais une petite modif permettrait qu'il soit géré
  • certaines fonctions d'url servent à gérer à la fois à une chose et son contraire (et le warning rencontré dans le cas d'une url /123 viendrait peut être d'une erreur d'orientation vers l'une des choses ou son contraire)
  • c'est complexe
  • utiliser sf/routing serait une alternative
Tentative de synthèse : - le cas des urls `/123` n'est pas prévu mais une petite modif permettrait qu'il soit géré - certaines fonctions d'url servent à gérer à la fois à une chose et son contraire (et le warning rencontré dans le cas d'une url /123 viendrait peut être d'une erreur d'orientation vers l'une des choses ou son contraire) - c'est complexe - utiliser [sf/routing](https://symfony.com/doc/current/routing.html) serait une alternative
Sign in to join this conversation.
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.