Réécriture sur les fonctions generer_xxxx_entite() et mise au carré des points d'entrée des fonctions url #5000
Merged
cerdic
merged 11 commits from issue_3311
into master
4 months ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'issue_3311'
Deleting a branch is permanent. It CANNOT be undone. Continue?
Fix
A priori aucune rupture de compatibilité : toutes les fonctions nommées selon l'ancienne convention continuent d'être appelées, et les appels vers les anciennes fonctions du core sont redirigés vers le nouveau nommage.
Seul risque : si un utilisateur a defini des fonctions qui correspondent au nouveau nommage du core (erreur fatale) ou aux nouvelles conventions de surcharge et qui du coup ne feront pas ce qui est attendu
(en particulier sur
generer_url_page()
qui peut potentiellement exister avec une signature différente...)Et donc une fois intégré on peut mettre au propre les modules urls de SPIP en fonction de cela
if ($generer = charger_fonction("generer_${info}_${type_objet}", '', true)) {
// Si la fonction generer_TYPE_TRUC existe, on l'utilise pour formater $info_generee
if ($generer = charger_fonction("generer_${type_objet}_${info}", '', true)
// @deprecated generer_TRUC_TYPE
Je suggère de mettre
@deprecated 4.1 generer_TRUC_TYPE
}
/**
* @deprecated
Idem
@deprecated 4.1
trop bien !
* @return string
*/
function generer_info_entite($id_objet, $type_objet, $info, $etoile = '', $params = []) {
function generer_objet_info($id_objet, $type_objet, $info, $etoile = '', $params = []) {
En fait il faudrait en profiter pour typer les signatures et retours de ces nouvelles fonctions.
C’est bien le moment de le faire vu que cela va nécessiter de modifier le code ailleurs.
Éventuellement de forcer du typage sur les appels depuis les fonctions précédentes déprécies.
Quelque chose donc comme
Du coup, plus besoin de
$id_objet = intval($id_objet)
en plus à l’intérieur de la fonction.oui mais comme c'est une fonction utilisée dans les squelettes aussi, pas sur que ce soit très safe : id_objet a autant de chance d'être un int qu'un string... mais on doit pouvoir typer tout le reste par contre
PHP fait un cast automatique. Et caster un string en int se ferait donc sans problème. C’est au contraire même pratique (pas besoin du intval derrière)
Par contre appeler avec
null
, à la place deint
là c’est erroné, sauf si ton typage permet le null (?int
donc), mais encore une fois si on peut choisir, il vaut mieuxint
à?int
.Il y a une subtilité si un fichier déclare
declare(strict_types=1);
en tête de fichier (ce qui n’est pas le cas chez nous par ailleurs) : l’exécution du code à l’intérieur des fonctions déclarées dans ce fichier, lorsqu’il appelle des fonctions aux arguments typés, doit appeler avec exactement le type attendu (pas de cast automatique) ; un mauvais type il lèvera une erreur (même s’il aurait pu caster la valeur donc). https://www.php.net/manual/fr/language.types.declarations.php#language.types.declarations.strictSuper, je me demande si ce refactoring pourrait être l'occasion de revenir sur le fait que
urls_xxx_dist()
qui sert à décoder soit renommées enurls_xxx_decoder_dist()
pour plus de clarté et afin d'être homogène avec le nommage deurls_xxx_generer_url()
. Mais, ça casserait peut-être trop de choses ? Ou alors ça n'est pas utile ? En attendant, je valide la PR :)PS : j'ai un jeu d'url perso dans https://github.com/geodiversite/geodiversite/blob/master/urls/geodiv.php si vous voulez je peux tester ça en 4.1, ça permettrait de valider que les fonctions de compat fonctionnent bien.
@b_b je ne peux pas me prononcer sur le ca casserait des choses, mais je pense que ce serait utile. J'avoue que les fonctions d'urls ont tjr été obscures pour moi, et du coup avoir quelque choses de plus claires serait une aide.
Ça serait une excellente idée que de séparer en 2 fonctions distinctes si ça n’a pas été fait. Et si c’est faisable également (avec la retrocompat)
La PR sépare déjà en deux (ou du moins permet de séparer) la fonction "decoder" de la fonction "generer", mais pour la fonction "decoder" j'ai gardé le nommage antérieur
urls_xxxx_dist()
Cela dit, à la reflexion, je pense qu'on pourrait en effet renommer la fonction "decoder" en
urls_xxxx_decoder_dist()
car le helper introduit ici2b893393e6
et utilisé partout maintenant peut fallback versurls_xxxx_dist()
si la fonction decoder n'existe pas (cas des anciens jeux d'URL utilisé sur un SPIP 4.1).Et même mieux, cela permet de garder la compat d'un module d'URL avec les anciens SPIP : il suffit de garder une fonction
urls_xxxx_dist()
non typée qui renvoie vers le generer ou le decoder selon si le premier argument est un int ou pas (comme c'était fait dans les fonctions d'urls jusqu'ici).Donc je dirai bonne idée, et je vais envoyer un truc dans ce sens
\o/
C'est envoyé et j'ai tout rebasé :p
Reviewers
90293ab5e8
.