Bug de comportement de la fonction `objet_modifier_champs()` en 4.1+ : les valeurs null sont ecrasées en base #5228

Closed
opened 2 months ago by cerdic · 8 comments
cerdic commented 2 months ago
Owner

La fonction objet_modifier_champs() a changé de comportement en 4.1+ quant aux valeurs nulles envoyées dans le tableau $c
https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/modifier.php#L99

Les entrées ayant une valeur nulle était ignorée jusqu'à SPIP 4.0.
En SPIP 4.1+, la valeur nulle est conservée dans le tableau associatif, et le passage dans corriger_caracteres() la transforme en chaine vide, qui est envoyée en base et écrase donc la valeur existante.

Le bug vient d'un refactoring.
On avait avant https://git.spip.net/spip/spip/src/branch/4.0/ecrire/inc/modifier.php#L144
les lignes

	// N'accepter que les champs qui existent
	// TODO: ici aussi on peut valider les contenus
	// en fonction du type
	$champs = [];
	foreach ($desc['field'] as $champ => $ignore) {
		if (isset($c[$champ])) {
			$champs[$champ] = $c[$champ];
		}
	}

qui ont été refactorées en
https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/modifier.php#L143

$champs = array_intersect_key($c, $desc['field']);

Le premier s'appuyant sur un isset() qui renvoie false sur les valeurs nulles, le second utilisant la fonction array_intersect_key() qui se contente de filtrer les valeurs de $c qui n'ont pas une entrée dans $desc['field']

Le bug n'est en général pas visible sur les formulaires d'édition des objets, car la fonction collecter_request() utilisée en amont pour collecter les variables postée ignore les valeurs nulles.

Mais avec les crayons, la fonction crayons_recupere_post() n'ignore pas les valeurs nulles et les récupère dans le tableau associatif
https://git.spip.net/spip-contrib-extensions/crayons/src/branch/master/action/crayons_store.php#L61

elles finissent par arriver dans objet_modifier_champs() qui écrase alors les valeurs en base

La fonction `objet_modifier_champs()` a changé de comportement en 4.1+ quant aux valeurs nulles envoyées dans le tableau `$c` https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/modifier.php#L99 Les entrées ayant une valeur nulle était ignorée jusqu'à SPIP 4.0. En SPIP 4.1+, la valeur nulle est conservée dans le tableau associatif, et le passage dans `corriger_caracteres()` la transforme en chaine vide, qui est envoyée en base et écrase donc la valeur existante. Le bug vient d'un refactoring. On avait avant https://git.spip.net/spip/spip/src/branch/4.0/ecrire/inc/modifier.php#L144 les lignes ``` // N'accepter que les champs qui existent // TODO: ici aussi on peut valider les contenus // en fonction du type $champs = []; foreach ($desc['field'] as $champ => $ignore) { if (isset($c[$champ])) { $champs[$champ] = $c[$champ]; } } ``` qui ont été refactorées en https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/modifier.php#L143 ``` $champs = array_intersect_key($c, $desc['field']); ``` Le premier s'appuyant sur un `isset()` qui renvoie false sur les valeurs nulles, le second utilisant la fonction `array_intersect_key()` qui se contente de filtrer les valeurs de `$c` qui n'ont pas une entrée dans `$desc['field']` Le bug n'est en général pas visible sur les formulaires d'édition des objets, car la fonction `collecter_request()` utilisée en amont pour collecter les variables postée ignore les valeurs nulles. Mais avec les crayons, la fonction `crayons_recupere_post()` n'ignore pas les valeurs nulles et les récupère dans le tableau associatif https://git.spip.net/spip-contrib-extensions/crayons/src/branch/master/action/crayons_store.php#L61 elles finissent par arriver dans `objet_modifier_champs()` qui écrase alors les valeurs en base
Poster
Owner

Pour info le bug vient de
#4916

Pour info le bug vient de https://git.spip.net/spip/spip/issues/4916
Owner

Hum, c’est subtile.
Tu veux dire qu’il faut compléter par :

$champs = array_filter($champs, 'is_null');

?

Hum, c’est subtile. Tu veux dire qu’il faut compléter par : ```php $champs = array_filter($champs, 'is_null'); ``` ?
Owner

Mais en êmme temps si tu fais cela ça veut dire que jamais on ne pourrait écrire null dans une colonne de mysql en passant par cette fonction ?

Mais en êmme temps si tu fais cela ça veut dire que jamais on ne pourrait écrire `null` dans une colonne de mysql en passant par cette fonction ?

Aaarg c'est pas très subtil, c'est un gros bug pour tous ceux qui utilisent l'API objet justement en mode API, par ex depuis des scripts, des actions persos, ou depuis des API ouvertes en JSON, ou dans des formulaires tout-en-un qui s'occupent de plusieurs objets, etc, plein de cas où parfois il peut y avoir des champs null non filtrés en amont (ne passant pas par collecter). 😐

Aaarg c'est pas très subtil, c'est un gros bug pour tous ceux qui utilisent l'API objet justement en mode API, par ex depuis des scripts, des actions persos, ou depuis des API ouvertes en JSON, ou dans des formulaires tout-en-un qui s'occupent de plusieurs objets, etc, plein de cas où parfois il peut y avoir des champs null non filtrés en amont (ne passant pas par collecter). 😐
Owner

J’ai pas tout à fait compris le cas d’usage Rasta, mais on passe à cette fonction (objet_modifier_champs) les entrées qu’on souhaite appliquer.

MYSQL accepte pour ses colonnes potentiellement des valeurs NULL.

J’en conclue que si on filtre tous les nulls ici, il est impossible d’envoyen NULL à mysql par cette fonction, qui semblerait pourtant tout indiqué.

Du coup je me demande si c’est pas à l’appelant de filtrer car il sait qu’il ne veut pas de null.

Ou alors tu supposes que SPIP considère que tous les champs / colonnes de ses objets sont en NOT NULL et que donc il faut tout le temps filtrer.

Ça me parait particulier comme convention, mais soit, c’est ce qu’il y avait avant.

J’ai pas tout à fait compris le cas d’usage Rasta, mais on passe à cette fonction (objet_modifier_champs) les entrées qu’on souhaite appliquer. MYSQL accepte pour ses colonnes potentiellement des valeurs NULL. J’en conclue que si on filtre tous les nulls ici, il est impossible d’envoyen NULL à mysql par cette fonction, qui semblerait pourtant tout indiqué. Du coup je me demande si c’est pas à l’appelant de filtrer car il sait qu’il ne veut pas de null. Ou alors tu supposes que SPIP considère que tous les champs / colonnes de ses objets sont en `NOT NULL` et que donc il faut tout le temps filtrer. Ça me parait particulier comme convention, mais soit, c’est ce qu’il y avait avant.
Owner

Il y a aussi un tableau d’option en paramètre, il pourrait y avoir une option 'filtrer_null' => true, par défaut (ou tout nommage compréhensible à trouver)

Il y a aussi un tableau d’option en paramètre, il pourrait y avoir une option `'filtrer_null' => true,` par défaut (ou tout nommage compréhensible à trouver)

Bé oui je parle bien de l'usage actuel, qui part bien du principe que tous les champs de SPIP (et des plugins) sont en not null, avec chaine vide "" par défaut pour la plupart. Et donc ce changement casse l'utilisation de nombreux trucs dans la nature.

Je suis d'accord que dans l'absolu null existe bien en SQL, donc oui faudrait sûrement le permettre pour être propre. Mais sans casser aussi directement tout ce qui est dans la nature. 🙂

Une option pour ne pas filtrer me semble une bonne idée.

Bé oui je parle bien de l'usage actuel, qui part bien du principe que tous les champs de SPIP (et des plugins) sont en not null, avec chaine vide "" par défaut pour la plupart. Et donc ce changement casse l'utilisation de nombreux trucs dans la nature. Je suis d'accord que dans l'absolu null existe bien en SQL, donc oui faudrait sûrement le permettre pour être propre. Mais sans casser aussi directement tout ce qui est dans la nature. 🙂 Une option pour ne pas filtrer me semble une bonne idée.
Poster
Owner

alors oui si vous voulez gérer les NULL pourquoi pas, mais là, en l'état, si tu envoie une valeur NULL sur une clé, elle est forcée en chaine vide par la ligne

$champs = array_map('corriger_caracteres', $champs);

donc en l'état ce n'est pas possible d'envoyer des null dans les tables objets via cette fonction.

Je propose de fixer le bug déjà (retour au fonctionnel précédent), et de traiter le cas des null comme une évolution possible, mais amha il y a des tests à faire un peu partout...

alors oui si vous voulez gérer les NULL pourquoi pas, mais là, en l'état, si tu envoie une valeur NULL sur une clé, elle est forcée en chaine vide par la ligne ``` $champs = array_map('corriger_caracteres', $champs); ``` donc en l'état ce n'est pas possible d'envoyer des null dans les tables objets via cette fonction. Je propose de fixer le bug déjà (retour au fonctionnel précédent), et de traiter le cas des null comme une évolution possible, mais amha il y a des tests à faire un peu partout...
marcimat closed this issue 2 months ago
Sign in to join this conversation.
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.