Project

General

Profile

Anomalie #4717

Erreurs nombre d'argument des filtres

Added by jluc - about 1 month ago. Updated 21 days ago.

Status:
En cours
Priority:
Normal
Assignee:
-
Category:
divers
Target version:
Start date:
04/08/2021
Due date:
% Done:

0%

Resolution:
Navigateur:

Description

Depuis PHP 7.1, une fonction appelée avec un mauvais nombre d'argument provoque une Fatal error: Uncaught ArgumentCountError: Too few arguments to function filtre_implode_dist(), 1 passed ... and exactly 2 expected

Or il n'y a aucune vérification, avant de l'appeler, du nombre d'argument avec lequel est appelée une fonction implémentant un filtre.

Du coup appeler [(#LISTE{Arbre,Bateau,Chat,Doche}|implode)] fait page blanche avec l'erreur ci dessus dans error.log

La définition est en effet

function filtre_implode_dist($a, $b) { return is_array($a) ? implode($b, $a) : $a; }

C'est assez rude pour du code SPIP où l'erreur est permise.

Du coup serait il possible de récupérer proprement ces situations d'erreur par gestion d'exception avant l'eval du code compilé ?

Sinon il faudrait s'assurer que toutes les fonctions php implémentant des filtres puissent accepter de n'avoir qu'un seul argument (la balise sur laquelle elles s'appliquent).

Ce qui donnerait ici :

function filtre_implode_dist($a, $b=", ") { return is_array($a) ? implode($b, $a) : $a; }

History

#2 Updated by b b 30 days ago

C'est amha le prix à payer de la compatibilité avec des versions de PHP récentes, je ne suis pas certain qu'on doive faire le grand écart pour que SPIP soit plus tolérant que son moteur (PHP) au risque d'avoir à prévoir plein de cas foireux dans SPIP, mais si on peut le faire sans trop d'effort et de lourdeur de code, gogogo bien sûr :)

#3 Updated by marcimat 🌻 30 days ago

Refs:

- https://www.php.net/manual/fr/migration71.incompatible.php
- https://www.php.net/manual/fr/function.implode.php
- https://www.spip.net/fr_article5672.html

Au aucun moment il n'est indiqué que le filtre |implode de SPIP puisse être appelé sans second paramètre, donc jusque là pas de problème : la personne a mal écrit son squelette.

Ceci étant dit, il semble que PHP analyse le problème même sans exécuter la fonction, et que ce Fatal pourrait être catch là https://git.spip.net/spip/spip/src/branch/master/ecrire/public/composer.php#L106 en plus, et retourner à la place une erreur_squelettes(), comme ça le fait pour le ParseError.

#4 Updated by jluc - 29 days ago

Oui c'est une erreur de squelette, mais SPIP d'ordinaire les détecte et signale proprement une erreur à l'internaute, auquel on ne demande pas de comprendre le fonctionnement de PHP et ses messages d'erreur, du moins pour les bases du codage SPIP. Ce serait bien de préserver autant que possible cette isolation de SPIP et de PHP.

Catch l'exception argumentcouterror semble possible https://www.php.net/manual/fr/class.argumentcounterror.php

Mais il y en a d'autres aussi : https://www.php.net/manual/fr/class.exception
Plus PHP devient strict et plus les contrôles préventifs de SPIP sur le code SPIP doivent être stricts aussi...

#5 Updated by jluc - 29 days ago

Une alternative à catcher les exceptions serait d'associer à chaque filtre SPIP sa déclaration d'arité minimale et maximale, et tester la conformité avant d'appeler.

(Mais faudra t il alors aussi une déclaration sur les types des arguments ? et ainsi de suite pour toutes les circonstances où PHP devient intolérant ?)

#6 Updated by cedric - 29 days ago

  • Target version changed from 3.2 to 4.0

J'ai essayé mais j'ai pas réussi à catcher l'erreur en PHP 7.3 :(

#8 Updated by cedric - 29 days ago

  • Status changed from Nouveau to En cours

#9 Updated by jluc - 28 days ago

La PR récupère l'erreur PHP, évite le crash blank et délivre le message d'erreur PHP dans la jolie box d'erreur SPIP. Le pire est évité, et donc il faut intégrer cette PR qui fait du bien.

Mais l'internaute SPIPien se retrouve avec un message d'erreur en anglais et portant sur le code compilé PHP. Ce code compilé, même le commun des devs PHP ne le connait pas, car à part XRay qui le rend accessible et les core-développeurs qui débuguent le code d'une nouvelle structure SPIP, personne ne va scruter et n'a connaissance du code php généré par le compilateur.

Pour faire mieux, il y a la piste de la déclaration des arités des filtres. $arite_des_filtres = [['implode', 2, 2], ['affdate', 1, 2]...]. Cette déclaration peut probablement être utilisée au moment de la compilation d'un appel de filtre lorsque le compilateur convertit la structure SPIP #XXX|filtre... en appel PHP.
Ça permet de détecter une erreur d'argumentcount AVANT l'appel PHP et de fournir un joli message d'erreur SPIP qui fait référence au source SPIP.

Dans ce cas, la détection d'erreur peut se faire à la compilation.

Mais il se peut que d'autres erreurs PHP doivent être gérées, avec la strictisation croissante de PHP. Il faudra donc de nouvelles déclarations permettant la détection préventive des erreurs.

Peut être sur d'autres structures de données que les filtres aussi ?

Il faut donc développer la capacité d'introspection de SPIP.

Mais parfois l'erreur n'est pas détectable au moment de la compilation. J'ai évoqué plus haut l'erreur relative au typage des arguments. S'il se confirme que le pb se pose aussi, et vu qu'on peut passer à un filtre un argument calculé dynamiquement, c'est pas lors de la compilation que ça pourra être détecté, mais lors de l'exécution du code compilé.

Il faut alors prendre chaque erreur PHP possible et voir si/comment le compilateur peut la prévenir.

Une alternative serait de "traduire" cette erreur PHP de manière à aiguiller le webmestre SPIP.

Par exemple le message d'erreur de https://www.mail-archive.com/spip@rezo.net/msg81110.html , qui fait référence au source compilé, peut être traduit, par reconnaissance (regexp), analyse du message d'erreur, et calcul du message d'erreur SPIP en « Erreur : dans votre source SPIP, il y un appel au filtre |implode qui n'a pas le bon nombre d'arguments. Ça se passe dans la boucle 'BOUCLE_contenu_article' : veuillez examiner le code et corriger. » Là en plus, ce serait bien de pouvoir associer le nom du fichier source à la boucle dont on connait le nom. Une capacité d'introspection déjà présente ? ou à développer ?

#10 Updated by jluc - 24 days ago

Le compilateur peut vérifier l'arité min et max d'une fonction PHP :

function getCountOfNotOptionalParameters(ReflectionFunction $refl) {
    $i=0;
    foreach( $refl->getParameters() as $param){
        if ($param->isOptional()) break;
        $i++;
    }
    return $i;
}
$reflection = new ReflectionFunction('filtre_implode_dist');
$min = getCountOfNotOptionalParameters($reflection);
$max = $reflection->getNumberOfParameters()."\n";
echo "il faut mini $min arguments et au maximum $max";

#11 Updated by jluc - 22 days ago

C'est plus simple directement avec la méthode getNumberOfRequiredParameters() !

J'ai donc produit une détection à la compilation des erreurs d'arité des appels de filtres : https://git.spip.net/spip/spip/commit/04b9bb552e936cd156331debd0d37d31285acce7 Ça marche bien, y compris pour les fonctions variadiques comme "printf"... mais pas pour des fonctions comme "concat" qui ne déclarent aucun argument et qui ne sont pas considérées comme variadiques, mais qui utilisent func_get_args() en interne !

Il semble que PHP ne supporte pas une erreur de nombre d'argument pour les fonctions du noyau PHP mais le supporte très bien pour celles définies par le code PHP ? Ça m'avait échappé.

#12 Updated by jluc - 22 days ago

Pour mémo :

function phraser_arite_incorrecte(string $filtre, int $nb_args) : string {
    if (!function_exists($filtre)) {    // détection des fonctions non existantes ailleurs ?
        return '';
    }
    $reflection = new ReflectionFunction($filtre);
    $min = $reflection->getNumberOfRequiredParameters();
    $max = $reflection->getNumberOfParameters();
    if (($nb_args < $min) or  ($nb_args > $max and !$reflection->isVariadic())) {
        return " appelé avec $nb_args arguments alors qu'il en faut au moins $min et au plus $max";
    }
    return '';
}

#13 Updated by jluc - 21 days ago

Voici la PR : https://git.spip.net/spip/spip/pulls/160 qui du coup teste aussi $reflection->isInternal()
et qui utilise la chaine de langue erreur existante.
Ça permet donc un message d'erreur i18n et qui fait référence au source SPIP au lieu du source PHP compilé que personne n'est sensé connaître.

Also available in: Atom PDF