Suite au bug reporté récemment par @rom.jou concernant TypeError: round(): Argument #1 ($num) must be of type int|float, string given (lien vers le topic original), j’ai analysé le correctif apporté dans le commit 442d472 (lien GitHub) et je trouve l’approche très discutable.
Note : Ce correctif date de plus de 11 mois et je ne connais pas la raison initiale de ces modifications, mais l’approche me semble problématique d’un point de vue technique.
Le vrai problème :
L’erreur vient du fait que $_value peut être une chaîne de caractères au moment de l’appel à round($_value, ...) ligne 1036. En PHP 8+, round() est plus strict sur le typage.
Analyse du code problématique :
Dans la version master, le flux est :
$_value = floatval(str_replace(',', '.', $_value)); (conversion uniquement si numeric ET remplacement virgule/point)
Plus tard : $_value = round($_value, $this->getConfiguration('historizeRound'));
Le problème : si $_value n’était pas numérique initialement, floatval() sur une chaîne non-numérique retourne 0, mais ce floatval() n’est appliqué que dans le cas où $this->getSubType() == 'numeric' ET en début de section.
Le « correctif » actuel :
Ajoute des return 0; précoces
Supprime les conditions maxValueReplace/minValueReplace (perte de fonctionnalité !)
Masque le symptôme au lieu de traiter la cause
Proposition de vraie correction :
case 'numeric':
// S'assurer que $_value est numérique avant round()
$_value = floatval($_value);
if ($this->getConfiguration('historizeRound') !== '' && is_numeric($this->getConfiguration('historizeRound')) && $this->getConfiguration('historizeRound') >= 0) {
$_value = round($_value, $this->getConfiguration('historizeRound'));
}
if ($_value > $this->getConfiguration('maxValue', $_value) && $this->getConfiguration('maxValueReplace') == 1) {
$_value = $this->getConfiguration('maxValue', $_value);
}
if ($_value < $this->getConfiguration('minValue', $_value) && $this->getConfiguration('minValueReplace') == 1) {
$_value = $this->getConfiguration('minValue', $_value);
}
return floatval($_value);
Cette approche :
Corrige vraiment le bug de typage
Préserve la logique métier existante (conditions maxValueReplace/minValueReplace)
Reste lisible et maintenable
Le correctif actuel me semble être du « quick & dirty » qui crée plus de problèmes qu’il n’en résout. Qu’en pensez-vous ?
Le correctif apporté par Loic (béta) est la simplement pour une commande qui n’a pas de valeur, lors de la création d’une commande d’un virtuel par exemple.
Il est tout a fait discutable que le return 0 (pour un numérique) est précoce, un simple $_value = 0 permettrait de continuer le check et de pouvoir passer les fonctionnalité maxValueReplace / minValueReplace mais est-ce que c’est l’effet voulu ? c’est une autre info discutable…
La je comprend pas :
Oui, mais le round est appliqué pour les même condition, c.a.d : seulement si getSubType est numeric, donc pas d’erreur normalement.
En faite la seule erreur que je vois et qui peut-être provoqué, c’est si l’user utilise du texte dans calculValueOffset que evaluateExpression ne sait pas traiter (Ex : #value# mm), c’est donc une erreur de configuration de l’user.( qui je le conçois peut-être traité par le core )
Merci pour tes éclaircissements, tu as mis le doigt sur le vrai problème !
Tu as raison sur mes erreurs : le floatval() et le round() sont bien dans le même contexte (numeric), mon analyse du flux était incorrecte.
Le vrai problème comme tu le dis est que calculValueOffset avec une formule mal construite type #value# * 3 mm, retourne "6 mm" au lieu de 6 :
PHP 7.4 : round("6 mm", 2) → prenait 6 et ignorait le reste (permissif)
PHP 8+ : round("6 mm", 2) → TypeError (strict)
Je suis d’accord avec toi sur le correctif de Loïc, le return 0 précoce empêche maxValueReplace/minValueReplace de fonctionner. Un simple $_value = 0 aurait été plus cohérent.
Si evaluateExpression() retourne du non-numérique sur une commande numeric, c’est effectivement une erreur de configuration utilisateur qui devrait être remontée à l’interface plutôt que masquée :
case 'numeric':
if (!is_numeric($_value)) {
throw new Exception('L’expression doit retourner une valeur numérique uniquement : résultat actuel '. $_value);
}
Plutôt que de masquer avec des return 0 précoces, ça aiderait l’utilisateur à corriger sa formule défaillante. Qu’en penses-tu ?
Alors la bravo pour votre analyse !! ( je dis ça parce que je n’ai rien compris )
Pour moi on a un paramètre de la fonction, $_value, qui semble être un string, puisqu’on y applique plein de fonctions de traitement de string… jusqu’à ce round, qui devrait recevoir du numérique, ça ne peut pas marcher… (?)
Il vaudrait mieux créer une variable de retour $_result et ne pas écraser $_value à chaque ligne, je comprendrais mieux
C’est pour cela que je disais que c’était une autre chose discutable.
formatValue peut-être appelé par 2 fonctions différentes (3 avec le checkAndUpdateCmd) :
execCmd
Sur une cmd « info » pas d’impacte car pas de formatValue (return getCache())
Sur une cmd « action » pas d’impacte car pas de calculValueOffset par contre appel de event si il y a un updateCmdId
event
Dans ce cas il faudrait que formatValue ne renvoie pas une exception (comme tu le suggère) mais une valeur (comme actuellement en beta)
Voici mon avis sur ce qu’il faudrait modifier pour en avertir l’utilisateur :
après les catch j’ajouterais simplement ceci :
if ($this->getSubType() == 'numeric' && !is_numeric($_value)) {
log::add('cmd', 'error', __('La formule de calcul doit retourner une valeur numérique uniquement : ', __FILE__) . $this->getHumanName() . ' => ' . $_value); // avertissement user
$_value = floatval(str_replace(',', '.', $_value)); // force une valeur
}
Il vaudrait mieux créer une variable de retour $_result et ne pas écraser $_value à chaque ligne
@pifou Je comprends ta suggestion d’utiliser une variable $_result séparée. Cependant, étant donné la complexité du code existant et les risques de régression, je préfère pour cette PR me limiter à corriger le problème initial sans refactoriser cette partie. On pourrait envisager cette amélioration dans une PR dédiée si nécessaire.
)