Compat PHP 8.1 #4968

Closed
opened 1 month ago by marcimat · 10 comments
marcimat commented 1 month ago
Owner

Il y a quelques soucis à l’utilisation de PHP 8.1 pour le moment.

Le plus embêtant est du à un changement lors d’erreurs de mysql : https://php.watch/versions/8.1/mysqli-error-mode : une exception est levée maintenant. Et comme notre code ne la catch pas, ça finit mal :)

Il y a également de très nombreux deprecated, essentiellement sur les éléments suivants :

  • les fonctions attendant des chaines de caractères en entrée (strlen, substr, preg_replace, etc.) ne tolèrent plus la valeur null. https://php.watch/versions/8.1/internal-func-non-nullable-null-deprecation . C’est plutôt bien, mais dans notre code ce n’est pas toujours évident de savoir si l’arrivée de null est du à une erreur ou si c’est tout à fait normal et dans ce cas là le cast avec (string) $var.

  • les méthodes étendues dans une classe héritée doivent avoir les typages des retours qui soient cohérents avec la méthode parente : et effectivement sur les classes internes à PHP des (promesses de) typages ont été ajoutés (notamment sur les itérateurs) : https://php.watch/versions/8.1/internal-method-return-types . Il faut compléter également chez nous ces typages.
    Cependant dans le cas du type mixed (introduit en PHP 8.0) il est préférable, si le peut (en connaissance de notre code) de réduire la portée du type à la place (string, int...). Sinon, tant que l’on reste compat avec php 7.4, un attribut #[\ReturnTypeWillChange] qui cache le deprecated avec conscience est à ajouter.

Il y a quelques soucis à l’utilisation de PHP 8.1 pour le moment. Le plus embêtant est du à un changement lors d’erreurs de mysql : https://php.watch/versions/8.1/mysqli-error-mode : une exception est levée maintenant. Et comme notre code ne la catch pas, ça finit mal :) Il y a également de très nombreux deprecated, essentiellement sur les éléments suivants : - les fonctions attendant des chaines de caractères en entrée (strlen, substr, preg_replace, etc.) ne tolèrent plus la valeur `null`. https://php.watch/versions/8.1/internal-func-non-nullable-null-deprecation . C’est plutôt bien, mais dans notre code ce n’est pas toujours évident de savoir si l’arrivée de `null` est du à une erreur ou si c’est tout à fait normal et dans ce cas là le cast avec `(string) $var`. - les méthodes étendues dans une classe héritée doivent avoir les typages des retours qui soient cohérents avec la méthode parente : et effectivement sur les classes internes à PHP des (promesses de) typages ont été ajoutés (notamment sur les itérateurs) : https://php.watch/versions/8.1/internal-method-return-types . Il faut compléter également chez nous ces typages. Cependant dans le cas du type `mixed` (introduit en PHP 8.0) il est préférable, si le peut (en connaissance de notre code) de réduire la portée du type à la place (`string`, `int`...). Sinon, tant que l’on reste compat avec php 7.4, un attribut `#[\ReturnTypeWillChange]` qui cache le deprecated avec conscience est à ajouter.
Poster
Owner

Au vu de https://discuter.spip.net/t/installation-echoue-mysqli-sql-exception-table-spip-git-spip-mots-articles-doesnt-exist/157131/5 ou pourra peut être reporter
b90d031e59 dans SPIP 4.0 … ça éviterait le principal problème, même si la 4.0 n’est pas prévue pour php 8.1 ?

Au vu de https://discuter.spip.net/t/installation-echoue-mysqli-sql-exception-table-spip-git-spip-mots-articles-doesnt-exist/157131/5 ou pourra peut être reporter b90d031e591ac dans SPIP 4.0 … ça éviterait le principal problème, même si la 4.0 n’est pas prévue pour php 8.1 ?
Poster
Owner

J’ai mis des typages sur certains arguments de fonctions, mais ça semblerait dire que c’est une mauvaise idée pour ne «pas casser» depuis les versions précédentes de SPIP.

Effectivement avec un typage sur les arguments, si une fonction est appelée incorrectement (notamment en utilisant null au lieu du type attentu tel que string), une erreur fatale est générée.

Des remarques / avis là dessus ?

https://discuter.spip.net/t/un-calendrier-des-futures-sorties/157212

J’ai mis des typages sur certains arguments de fonctions, mais ça semblerait dire que c’est une mauvaise idée pour ne «pas casser» depuis les versions précédentes de SPIP. Effectivement avec un typage sur les arguments, si une fonction est appelée incorrectement (notamment en utilisant `null` au lieu du type attentu tel que `string`), une erreur fatale est générée. Des remarques / avis là dessus ? https://discuter.spip.net/t/un-calendrier-des-futures-sorties/157212

J'ai du mal à me faire une idée des cas réels et surtout de la fréquence où ça peut arriver, des cas où ça pèterait.

À priori pour un changement de Z moyen, mais pour un changement de Y là ça me parait correct : ça casse pas au sens ya des choses qui ont été enlevées (telle fonction d'API enlevée etc), mais plutôt au sens au ça montre plus visibles des bugs qui ne devraient pas être là et ça pousse les gens à les corriger. Donc c'est plutôt bien pour du Y non ?

(Après on peut aussi dire que c'est de la visibilité de bugs et que donc même pour du Z ça irait… mais bon faut pas être trop méchant)

J'ai du mal à me faire une idée des cas réels et surtout de la fréquence où ça peut arriver, des cas où ça pèterait. À priori pour un changement de Z moyen, mais pour un changement de Y là ça me parait correct : ça casse pas au sens ya des choses qui ont été enlevées (telle fonction d'API enlevée etc), mais plutôt au sens au ça montre plus visibles des bugs qui ne devraient pas être là et ça pousse les gens à les corriger. Donc c'est plutôt bien pour du Y non ? (Après on peut aussi dire que c'est de la visibilité de *bugs* et que donc même pour du Z ça irait… mais bon faut pas être trop méchant)
b_b commented 2 weeks ago
Owner

Sinon, tant que l’on reste compat avec php 7.4, un attribut #[\ReturnTypeWillChange] qui cache le deprecated avec conscience est à ajouter.

La 4.0 est compat PHP 7.3 / 8.0, donc la 4.1 devrait être compat 7.4 / 8.1, on est bons pour ça je pense.

J’ai mis des typages sur certains arguments de fonctions, mais ça semblerait dire que c’est une mauvaise idée pour ne «pas casser» depuis les versions précédentes de SPIP. Effectivement avec un typage sur les arguments, si une fonction est appelée incorrectement (notamment en utilisant null au lieu du type attentu tel que string), une erreur fatale est générée.

Comme le disait @rastapopoulos :

À priori pour un changement de Z moyen, mais pour un changement de Y là ça me parait correct

On est sur une version Y+1, donc ça ne me choque pas, mais c'est tout de même une fatale qui est levée. Mais bon, on se prend plus souvent des fatale en PHP 8 qu'en PHP 7 :p

Tant qu'on fourni aux gens des outils de scan (à la phpcs) pour repérer ce qui cloche dans le code de leurs plugins, ça devrait aller je pense.

En résumé, gogogo ?!?

> Sinon, tant que l’on reste compat avec php 7.4, un attribut #[\ReturnTypeWillChange] qui cache le deprecated avec conscience est à ajouter. La 4.0 est compat PHP 7.3 / 8.0, donc la 4.1 devrait être compat 7.4 / 8.1, on est bons pour ça je pense. > J’ai mis des typages sur certains arguments de fonctions, mais ça semblerait dire que c’est une mauvaise idée pour ne «pas casser» depuis les versions précédentes de SPIP. Effectivement avec un typage sur les arguments, si une fonction est appelée incorrectement (notamment en utilisant null au lieu du type attentu tel que string), une erreur fatale est générée. Comme le disait @rastapopoulos : > À priori pour un changement de Z moyen, mais pour un changement de Y là ça me parait correct On est sur une version Y+1, donc ça ne me choque pas, mais c'est tout de même une fatale qui est levée. Mais bon, on se prend plus souvent des fatale en PHP 8 qu'en PHP 7 :p Tant qu'on fourni aux gens des outils de scan (à la phpcs) pour repérer ce qui cloche dans le code de leurs plugins, ça devrait aller je pense. En résumé, gogogo ?!?
Poster
Owner

Bah pourquoi pas alors.

Il faudra penser à reporter 348e4229 dans la 4.0 (ça fatale en php 8.0 aussi sinon)

Bah pourquoi pas alors. Il faudra penser à reporter 348e4229 dans la 4.0 (ça fatale en php 8.0 aussi sinon)
b_b added this to the 4.1 milestone 1 week ago
b_b added the
amélioration
label 1 week ago
JLuc commented 1 week ago

Il me semble que typer les arguments permet de détecter les problèmes plus tôt, ce qui est plutôt bien. C'est aussi l'occasion de mieux spécifier les fonctions (et ce qu'elles font quand et sur quoi et ce qu'elles ne font pas), du moins si le fix ne consiste pas à cacher la poussière sous le tapis. Et ce peut être, au moins dans certains cas, l'occasion de compléter ou raffraichir la documentation, ce qui est carrément bien, et même si ce n'est pas le cas, un typage est en soi déjà un élément de documentation à jour.
En plus, depuis toujours SPIP est proche de PHP et c'est une de ses forces. PHP étant de plus en plus strict, j'ai l'impression que SPIP doit suivre cette évolution, pour en bénéficier lui même et aussi pour garder sa proximité avec PHP.

Il me semble que typer les arguments permet de détecter les problèmes plus tôt, ce qui est plutôt bien. C'est aussi l'occasion de mieux spécifier les fonctions (et ce qu'elles font quand et sur quoi et ce qu'elles ne font pas), du moins si le fix ne consiste pas à cacher la poussière sous le tapis. Et ce peut être, au moins dans certains cas, l'occasion de compléter ou raffraichir la documentation, ce qui est carrément bien, et même si ce n'est pas le cas, un typage est en soi déjà un élément de documentation à jour. En plus, depuis toujours SPIP est proche de PHP et c'est une de ses forces. PHP étant de plus en plus strict, j'ai l'impression que SPIP doit suivre cette évolution, pour en bénéficier lui même et aussi pour garder sa proximité avec PHP.
Poster
Owner

Bon, bah lets go, on verra à l’usage :)

Bon, bah lets go, on verra à l’usage :)
Poster
Owner

#4989 est intégré.

Je vais faire une autre PR pour des simplifications de code avec Raptor.

#4989 est intégré. Je vais faire une autre PR pour des simplifications de code avec Raptor.
marcimat closed this issue 1 week ago
b_b commented 1 week ago
Owner
Pour référence https://github.com/rectorphp/rector
b_b commented 2 days ago
Owner

Bah pourquoi pas alors.

Il faudra penser à reporter 348e4229 dans la 4.0 (ça fatale en php 8.0 aussi sinon)

C'est fait avec 1cc1c4cc50 :)

> Bah pourquoi pas alors. > > Il faudra penser à reporter 348e4229 dans la 4.0 (ça fatale en php 8.0 aussi sinon) > C'est fait avec 1cc1c4cc50 :)
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.