spip
/
spip
29
15
Fork 19
Code Issues 392 Pull Requests 5 Projects Releases Wiki Activity
Labels Milestones
New Pull Request

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
Conversation 7 Commits 11 Files Changed 43
cerdic commented 4 months ago
Owner

Fix

  • #3311 : Renommer generer_xxxx_entite en generer_objet_xxxx
  • #962 : on peut definir une fonction qui generer une url propre pour les urls page
  • permettra de fixer #4981 en ayant plusieurs point d'entree sur les modules urls propre
    • urls_xxx_dist() pour decoder uniquement
    • urls_xxx_generer_url_objet() pour generer l'url d'un objet
    • urls_xxx_generer_url_page() pour generer l'url d'une page spip (fonction facultative)

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...)

Fix * #3311 : Renommer generer_xxxx_entite en generer_objet_xxxx * #962 : on peut definir une fonction qui generer une url propre pour les urls page * permettra de fixer #4981 en ayant plusieurs point d'entree sur les modules urls propre * urls_xxx_dist() pour decoder uniquement * urls_xxx_generer_url_objet() pour generer l'url d'un objet * urls_xxx_generer_url_page() pour generer l'url d'une page spip (fonction facultative) 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...)
cerdic added 5 commits 4 months ago
4da660017f Issue #3311 : renommer les fonctions
aea5340d7a Issue #3311 : renommer les fonctions
e4a5432e34 Issue #3311 : renommer les fonctions
2b893393e6 Une fonction charger_fonction_url() qui permet de trouver la fonction pour generer l'url d'un objet, d'une page ou de decoder une url selon le type d'url active
cerdic commented 4 months ago
Poster
Owner

Et donc une fois intégré on peut mettre au propre les modules urls de SPIP en fonction de cela

Et donc une fois intégré on peut mettre au propre les modules urls de SPIP en fonction de cela
marcimat reviewed 4 months ago
ecrire/inc/filtres.php Outdated
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
marcimat commented 4 months ago
Poster
Owner

Je suggère de mettre @deprecated 4.1 generer_TRUC_TYPE

Je suggère de mettre `@deprecated 4.1 generer_TRUC_TYPE`
ecrire/inc/filtres.php Outdated
}
/**
* @deprecated
marcimat commented 4 months ago
Poster
Owner

Idem @deprecated 4.1

Idem `@deprecated 4.1`
cerdic added 1 commit 4 months ago
cerdic added 1 commit 4 months ago
cerdic added 1 commit 4 months ago
rastapopoulos approved these changes 4 months ago
rastapopoulos left a comment

trop bien !

marcimat reviewed 4 months ago
ecrire/inc/filtres.php Outdated
* @return string
*/
function generer_info_entite($id_objet, $type_objet, $info, $etoile = '', $params = []) {
function generer_objet_info($id_objet, $type_objet, $info, $etoile = '', $params = []) {
marcimat commented 4 months ago
Poster
Owner

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

function generer_objet_info(int $id_objet, string $type_objet, string $info, string $etoile = '', array $params = []): array {
...
}


function generer_info_entite($id_objet, $type_objet, $info, $etoile = '', $params = []){
	return generer_objet_info((int) $id_objet, (string) $type_objet, (string) $info, (string) $etoile, array $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 ``` function generer_objet_info(int $id_objet, string $type_objet, string $info, string $etoile = '', array $params = []): array { ... } function generer_info_entite($id_objet, $type_objet, $info, $etoile = '', $params = []){ return generer_objet_info((int) $id_objet, (string) $type_objet, (string) $info, (string) $etoile, array $params); } ```
marcimat commented 4 months ago
Poster
Owner

Du coup, plus besoin de $id_objet = intval($id_objet) en plus à l’intérieur de la fonction.

Du coup, plus besoin de `$id_objet = intval($id_objet)` en plus à l’intérieur de la fonction.
cerdic commented 4 months ago
Poster
Owner

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

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
marcimat commented 4 months ago
Poster
Owner

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 de int là c’est erroné, sauf si ton typage permet le null (?int donc), mais encore une fois si on peut choisir, il vaut mieux int à ?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.strict

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 de `int` là c’est erroné, sauf si ton typage permet le null (`?int` donc), mais encore une fois si on peut choisir, il vaut mieux `int` à `?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.strict
cerdic added 2 commits 4 months ago
b_b commented 4 months ago
Owner

Super, 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 en urls_xxx_decoder_dist() pour plus de clarté et afin d'être homogène avec le nommage de urls_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.

Super, 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 en `urls_xxx_decoder_dist()` pour plus de clarté et afin d'être homogène avec le nommage de `urls_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 approved these changes 4 months ago
maieul commented 4 months ago

Super, 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 en urls_xxx_decoder_dist() pour plus de clarté et afin d'être homogène avec le nommage de urls_xxx_generer_url(). Mais, ça casserait peut-être trop de choses ? Ou alors ça n'est pas utile ? En attendant, je valide la PR :)

@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.

> Super, 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 en urls_xxx_decoder_dist() pour plus de clarté et afin d'être homogène avec le nommage de urls_xxx_generer_url(). Mais, ça casserait peut-être trop de choses ? Ou alors ça n'est pas utile ? En attendant, je valide la PR :) @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.
marcimat commented 4 months ago
Owner

Ç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)

Ç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)
cerdic commented 4 months ago
Poster
Owner

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 ici 2b893393e6 et utilisé partout maintenant peut fallback vers urls_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

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 ici https://git.spip.net/spip/spip/commit/2b893393e6f31c3233b1fecd71e295eaeea72149 et utilisé partout maintenant peut fallback vers `urls_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
b_b commented 4 months ago
Owner

Donc je dirai bonne idée, et je vais envoyer un truc dans ce sens

\o/

> Donc je dirai bonne idée, et je vais envoyer un truc dans ce sens \o/
cerdic added 1 commit 4 months ago
0981d56a1e La fonction de decodage des urls attendue s'appelle maintenant urls_xxxx_decoder_url[_dist]() ce qui evite toute confusion avec l'ancienne fonction urls_xxxx_dist() et est plus clair.
cerdic added 11 commits 4 months ago
ac5b08b534 Issue #3311 : renommer les fonctions
a1ab856937 Issue #3311 : renommer les fonctions
86b372af74 Issue #3311 : renommer les fonctions
6fd4680384 Une fonction charger_fonction_url() qui permet de trouver la fonction pour generer l'url d'un objet, d'une page ou de decoder une url selon le type d'url active
c8639c49d5 La fonction de decodage des urls attendue s'appelle maintenant urls_xxxx_decoder_url[_dist]() ce qui evite toute confusion avec l'ancienne fonction urls_xxxx_dist() et est plus clair.
cerdic commented 4 months ago
Poster
Owner

C'est envoyé et j'ai tout rebasé :p

C'est envoyé et j'ai tout rebasé :p
cerdic merged commit 90293ab5e8 into master 4 months ago
cerdic deleted branch issue_3311 4 months ago

Reviewers

rastapopoulos approved these changes 4 months ago
b_b approved these changes 4 months ago
The pull request has been merged as 90293ab5e8.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
rastapopoulos
b_b
Labels
Apply labels
Clear labels
accessibilité amélioration
Amélioration, nouvelle fonctionnalité APIs authentification base de données bug
Ca ne fonctionne pas code généré compilo css divers documentation doublon
Ce ticket est un doublon ergonomie espace privé filtres et balises formulaires Inscription installation invalide
Ticket invalide javascript langues LDAP plugin PostgreSQL refusé
Ignoré, c'est comme Ca... sécurité traduction
No Label accessibilité amélioration APIs authentification base de données bug code généré compilo css divers documentation doublon ergonomie espace privé filtres et balises formulaires Inscription installation invalide javascript langues LDAP plugin PostgreSQL refusé sécurité traduction
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
6 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'issue_3311'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes