#78 afficher_si sur des champs (pas extra!)

Closed
opened 1 month ago by maieul · 18 comments
maieul commented 1 month ago
Collaborator

En travaillant sur https://git.spip.net/spip-contrib-extensions/formidable/issues/58 je suis tombé sur un cas où le comportement d'afficher_si pose questions.

Attention c'est technique !

Pour résumer ce qui se passe

  • lorsqu'on utilise saisies_verifier(), on met à null (et pas à chaine vide !) toutes les saisies masquées pour cause d'afficher_si
  • ce comportement est lié au fait que afficher_si a d'abord été défini pour formidable, et donc dans formidable on n'enregistre pas les champs null alors qu'on enregistre les champs avec une chaine vide. Cela permet aussi d'éviter les erreurs sur les champs masqués qui ne répondraient pas à tel ou tel vérification programmé
  • En revanche, pour les véritables objet SPIP, c'est l'inverse (!) :
    • si pour un champ donné la valeur en _request est null, objet_modifier_champs() ne modifie pas le champ en base
    • en revanche si pour un champ donné la valeur en _request est '', on modifie en base (et donc on vide un champ).
  • Concrètement, cela veut dire que pour un objet SPIP, un champ masqué par afficher_si aura toujours sa vieille valeur en base
  • coté champ extra j'avais corrigé cela il y a quelques mois par un commit qui avait jasé (car il était un peu obscure, mais c'est aussi parce que cextras n'utilise pas vraiment l'API de saisies, enfin pas totalement !)
  • mais il me semble qu'en réalité cela devrait être géré nativement quelque soit la manière dont on déclare le champ

Proposition

  • on peut très bien rétablir à '' les champs masqués par afficher_si dans le pipeline pre_edition
  • ca casserait éventuellement le comportement historique, mais il me semble qu'il relevait d'un bug
  • et on peut annuler la case avec l'option afficher_si_avec_post

A vos avis sur la proposition.

En travaillant sur https://git.spip.net/spip-contrib-extensions/formidable/issues/58 je suis tombé sur un cas où le comportement d'afficher_si pose questions. Attention c'est technique ! ## Pour résumer ce qui se passe - lorsqu'on utilise `saisies_verifier()`, on met à `null` (et pas à chaine vide !) toutes les saisies masquées pour cause d'afficher_si - ce comportement est lié au fait que afficher_si a d'abord été défini pour formidable, et donc dans formidable on n'enregistre pas les champs `null` alors qu'on enregistre les champs avec une chaine vide. Cela permet aussi d'éviter les erreurs sur les champs masqués qui ne répondraient pas à tel ou tel vérification programmé - En revanche, pour les véritables objet SPIP, c'est l'inverse (!) : - si pour un champ donné la valeur en `_request` est `null`, `objet_modifier_champs()` ne modifie pas le champ en base - en revanche si pour un champ donné la valeur en `_request` est `''`, on modifie en base (et donc on vide un champ). - Concrètement, cela veut dire que pour un objet SPIP, un champ masqué par afficher_si aura toujours sa vieille valeur en base - coté champ extra j'avais corrigé cela il y a quelques mois par un commit qui avait jasé (car il était un peu obscure, mais c'est aussi parce que cextras n'utilise pas vraiment l'API de saisies, enfin pas totalement !) - mais il me semble qu'en réalité cela devrait être géré nativement quelque soit la manière dont on déclare le champ ## Proposition - on peut très bien rétablir à `''` les champs masqués par afficher_si dans le pipeline `pre_edition` - ca casserait éventuellement le comportement historique, mais il me semble qu'il relevait d'un bug - et on peut annuler la case avec l'option `afficher_si_avec_post` A vos avis sur la proposition.

Mais donc si on rétablit à chaine vide, dans Formidable ça va les garder à l'enregistrement ?

Mais donc si on rétablit à chaine vide, dans Formidable ça va les garder à l'enregistrement ?
maieul commented 1 month ago
Poster
Collaborator

d'où mon idée de faire cela dans le pipeline pre_edition, qui concerne uniquement objet_modifier, et non pas formidable.

d'où mon idée de faire cela dans le pipeline pre_edition, qui concerne uniquement objet_modifier, et non pas formidable.

Mais donc ça veut dire tester deux fois les champs masqués par afficher_si ? Une fois en amont et ça met à null, et une deuxième fois plus loin pour les remettre à chaine vide ?

(bon après je sais pas de tête comment c'est dans la fonction, mais une fois cherché ça pourrait être mis en static pour tel form, et donc dans le même hit si redemandé plus loin, on refait pas le boulot)

Mais donc ça veut dire tester deux fois les champs masqués par afficher_si ? Une fois en amont et ça met à null, et une deuxième fois plus loin pour les remettre à chaine vide ? (bon après je sais pas de tête comment c'est dans la fonction, mais une fois cherché ça pourrait être mis en static pour tel form, et donc dans le même hit si redemandé plus loin, on refait pas le boulot)
maieul commented 1 month ago
Poster
Collaborator

Non ce serait pas optimal (on a un effectivement un static, mais uniquement sur le parsage).

Je pensais simplement stocker en globales la listes des champs concernées, puis les récuperer ensuite et les mettres à ''.

Non ce serait pas optimal (on a un effectivement un static, mais uniquement sur le parsage). Je pensais simplement stocker en globales la listes des champs concernées, puis les récuperer ensuite et les mettres à ''.
cerdic commented 4 weeks ago
Owner

Cette histoire a un peu ni queue ni tête:

  • soit le champ est saisi dans le formulaire, il y a une valeur postée, et donc elle doit être prise en compte à l'enregistrement (que ce soit formidable ou sur un objet)
  • soit le champ est masqué, donc sa valeur n'est pas saisie, il n'y a rien de posté (en display none un input est ignoré par les navigateurs), et la valeur en base n'a aucune raison d'être modifiée.

si la feature que vous utilisez c'est "je masque et en fait ça veut dire que ça correspond à une valeur pae défaut à enregistrer en base" ça doit toujours et encore se traiter dans le HTML, par un <input type='hidden' value='' name='xxxx' /> avant la saisie potentiellement masquée.

Ainsi : soit le vrai input est masqué et c'est la valeur par défaut du hidden qui est prise en compte, soit le vrai input est pas masqué et c'est la valeur de la saisie qui est prise en compte.

Bref, il devrait jamais y avoir des bidouilles dans le php après le POST, tout doit se traiter dans le html.

Cette histoire a un peu ni queue ni tête: - soit le champ est saisi dans le formulaire, il y a une valeur postée, et donc elle doit être prise en compte à l'enregistrement (que ce soit formidable ou sur un objet) - soit le champ est masqué, donc sa valeur n'est pas saisie, il n'y a rien de posté (en display none un input est ignoré par les navigateurs), et la valeur en base n'a aucune raison d'être modifiée. *si* la feature que vous utilisez c'est "je masque et en fait ça veut dire que ça correspond à une valeur pae défaut à enregistrer en base" ça doit toujours et encore se traiter dans le HTML, par un `<input type='hidden' value='' name='xxxx' />` avant la saisie potentiellement masquée. Ainsi : soit le vrai input est masqué et c'est la valeur par défaut du hidden qui est prise en compte, soit le vrai input est pas masqué et c'est la valeur de la saisie qui est prise en compte. Bref, il devrait jamais y avoir des bidouilles dans le php après le POST, tout doit se traiter dans le html.
maieul commented 4 weeks ago
Poster
Collaborator

Ca se discute.

  1. Pour formidable, qui est un peu une exception, il faut précisement que la donné ne soit pas postée (à cause de la manière dont formidable stocket les données, précisement pas sous forme de colonne SQL mais sous forme cle|valeur dans formulaires_reponses_champ. Mais à la rigueur c'est un problème propre à Formidable.
  2. Ensuite il faut toujours faire d'une manière ou d'une autre une vérification de securité côté PHP, (que ce soit à base de set_request ou autre) car on ne peut pas se fier à ce qu'envoie le navigateur, vu que les gens peuvent très facilement modifier le contenu de leur POST. Ca été à la base de afficher_si : une double verification JS + PHP. Et en discutait l'autre jour encore sur l'IRC (car personnellement j'aimerais bien me passer de cette double verification), et il y avait consensus entre les 3 personnnes présentes sur le fait qu'on pouvait pas se passer d'une verif PHP (qu'elle qu'en soit la forme)
  3. Par ailleurs, même à supposer qu'on passe outre le point 2., resterait le problème qu'il ne faut pas verifier la valeur d'une saisie masqué par afficher_si (exemple typqiue : une saisie marquée comme obligatoire, mais conditonné : il ne faut vérifier son remplissage que si elle n'est pas masquée par afficher_si, sinon tu provoque des erreursp pour rien)
Ca se discute. 1. Pour formidable, qui est un peu une exception, il faut précisement que la donné ne soit pas postée (à cause de la manière dont formidable stocket les données, précisement pas sous forme de colonne SQL mais sous forme `cle|valeur` dans `formulaires_reponses_champ`. Mais à la rigueur c'est un problème propre à Formidable. 2. Ensuite il faut toujours faire d'une manière ou d'une autre une vérification de securité côté PHP, (que ce soit à base de set_request ou autre) car on ne peut pas se fier à ce qu'envoie le navigateur, vu que les gens peuvent très facilement modifier le contenu de leur POST. Ca été à la base de afficher_si : une double verification JS + PHP. Et en discutait l'autre jour encore sur l'IRC (car personnellement j'aimerais bien me passer de cette double verification), et il y avait consensus entre les 3 personnnes présentes sur le fait qu'on pouvait pas se passer d'une verif PHP (qu'elle qu'en soit la forme) 3. Par ailleurs, même à supposer qu'on passe outre le point 2., resterait le problème qu'il ne faut pas verifier la valeur d'une saisie masqué par afficher_si (exemple typqiue : une saisie marquée comme obligatoire, mais conditonné : il ne faut vérifier son remplissage que si elle n'est pas masquée par afficher_si, sinon tu provoque des erreursp pour rien)
marcimat commented 4 weeks ago

J'ai pas suivi l'histoire, mais sur le fait que display:none enverrait les champs, https://stackoverflow.com/a/8318442 suggère de passer les champs en disabled pour ne pas qu'ils soient postés.

J'ai pas suivi l'histoire, mais sur le fait que display:none enverrait les champs, https://stackoverflow.com/a/8318442 suggère de passer les champs en disabled pour ne pas qu'ils soient postés.

en display none un input est ignoré par les navigateurs

absolument pas (depuis des années)

et donc oui il faut un autre moyen, qui peut être disabled, mais ça n'empêche qu'on ne peut pas se fier au HTML qui peut être modifié en 2 clics (et donc casser la logique demandée par les admins ou les devs qui est que tel champ ne doit absolument pas être pris en compte si masqué par telle condition)

donc là cette argumentation ne me convainc pas, il faut bien faire des vérif après, pas du tout juste dans le HTML

> en display none un input est ignoré par les navigateurs absolument pas (depuis des années) et donc oui il faut un autre moyen, qui peut être disabled, mais ça n'empêche qu'on ne peut pas se fier au HTML qui peut être modifié en 2 clics (et donc casser la logique demandée par les admins ou les devs qui est que tel champ ne doit absolument pas être pris en compte si masqué par telle condition) donc là cette argumentation ne me convainc pas, il faut bien faire des vérif après, pas du tout juste dans le HTML

Là solution trouvée :

  1. Faire la correction de #79
  2. saisies_verifier() met les saisies masquées par afficher_si à "" (chaine vide) au lieu de null actuellement
  3. MAIS un traitement différencié détecte quand même celles qui ont une demande de normalisation, et alors ça passe quand même celle-ci (même en chaine vide donc) dans la vérif/normalisation : et sans utiliser l'erreur, ce traitement spécifique ne sert qu'à récupérer leur $normalisation
  4. Formidable dans son code perso, recherche de nouveau quelles saisies ont été masquées (c'est lui l'exception, à lui de faire plus de boulot) et remet bien à null toutes ces valeurs là (donc qu'il y ait eu normalisation ou pas, lui s'en fout, il veut null pour toutes celles masquées)
Là solution trouvée : 1) Faire la correction de #79 2) saisies_verifier() met les saisies masquées par afficher_si à "" (chaine vide) au lieu de null actuellement 3) MAIS un traitement différencié détecte quand même celles qui ont une demande de normalisation, et alors ça passe quand même celle-ci (même en chaine vide donc) dans la vérif/normalisation : et sans utiliser l'erreur, ce traitement spécifique ne sert qu'à récupérer leur $normalisation 4) Formidable dans son code perso, recherche de nouveau quelles saisies ont été masquées (c'est lui l'exception, à lui de faire plus de boulot) et remet bien à null toutes ces valeurs là (donc qu'il y ait eu normalisation ou pas, lui s'en fout, il veut null pour toutes celles masquées)
maieul commented 4 weeks ago
Poster
Collaborator

Lu et approuvé. Il y a plus qu'à.

Et donc la fonction qui permettrait d'avoir toutes les saisies masquées par afficher_si sans devoir refaire tout les calculs à chaque fois.

/**
 * Liste des saisies masquées par afficher_si dans le hit courant
 * @param str $action ('set'|'get');
 * @param str $saisie_identifiant
 * @return array|null
**/
function saisies_afficher_si_liste_masquees($action = 'get', $saisie_identifiant = '') {
	static $tableau = array();
	if ($action === 'set') {
		$tableau[] = $saisie_identifiant;
	} elseif ($action === 'get') {
		return $tableau;
	}
}
Lu et approuvé. Il y a plus qu'à. Et donc la fonction qui permettrait d'avoir toutes les saisies masquées par afficher_si sans devoir refaire tout les calculs à chaque fois. ```php /** * Liste des saisies masquées par afficher_si dans le hit courant * @param str $action ('set'|'get'); * @param str $saisie_identifiant * @return array|null **/ function saisies_afficher_si_liste_masquees($action = 'get', $saisie_identifiant = '') { static $tableau = array(); if ($action === 'set') { $tableau[] = $saisie_identifiant; } elseif ($action === 'get') { return $tableau; } } ```
maieul commented 4 weeks ago
Poster
Collaborator

hum, j'ai mis saisie_identifiant, mais vu que les saisies sont pas toujours identifiés+ le fait que le besoin concerne le retour de $POST, est-ce que ce serait pas plutôt le name qu'on devrait mettre ?

hum, j'ai mis saisie_identifiant, mais vu que les saisies sont pas toujours identifiés+ le fait que le besoin concerne le retour de $POST, est-ce que ce serait pas plutôt le name qu'on devrait mettre ?

je pensais plutôt à une fonction qui retourne les saisies complètes, tout comme la fonction actuelle retourne toutes les saisies complètes qui ne sont pas masquées, là ça serait l'inverse, toutes les saisies qui sont masquées

je pensais plutôt à une fonction qui retourne les saisies complètes, tout comme la fonction actuelle retourne toutes les saisies complètes qui ne sont *pas* masquées, là ça serait l'inverse, toutes les saisies qui sont masquées

Comme dit finalement dans #79, en fait si c'est bien fait proprement et plus finement là bas, il n'y a même pas à savoir lesquelles sont masquées. Pour l'étape 3 :

  • pour le test d'obligation, on fait passer le formulaire nettoyé des masquées comme actuellement
  • mais pour l'API Vérifier, on fait passer 100% donc y compris celles masquées qui seront devenues des chaines vides (suffit de garder la liste complète aussi)

Et du coup même pas besoin de savoir ici lesquelles sont spécifiquement masquées.

Comme dit finalement dans #79, en fait si c'est bien fait proprement et plus finement là bas, il n'y a même pas à savoir lesquelles sont masquées. Pour l'étape 3 : - pour le test d'obligation, on fait passer le formulaire nettoyé des masquées comme actuellement - mais pour l'API Vérifier, on fait passer 100% donc y compris celles masquées qui seront devenues des chaines vides (suffit de garder la liste complète aussi) Et du coup même pas besoin de savoir ici lesquelles sont spécifiquement masquées.
maieul commented 4 weeks ago
Poster
Collaborator

Et du coup même pas besoin de savoir ici lesquelles sont spécifiquement masquées.

effectivement, dans saisies_verifier() pas besoin.

je pensais plutôt à une fonction qui retourne les saisies complètes, tout comme la fonction actuelle retourne toutes les saisies complètes qui ne sont pas masquées, là ça serait l'inverse, toutes les saisies qui sont masquées

hum, est-ce bien nécessaire de stocker la saisie complète ?
a. Pour le besoin de formidable, si on a juste le name, on peut faire le set_request_null()
b. Pour les usages ailleurs... bah pour l'instant on n'en voit pas.

> Et du coup même pas besoin de savoir ici lesquelles sont spécifiquement masquées. effectivement, dans saisies_verifier() pas besoin. > je pensais plutôt à une fonction qui retourne les saisies complètes, tout comme la fonction actuelle retourne toutes les saisies complètes qui ne sont pas masquées, là ça serait l'inverse, toutes les saisies qui sont masquées hum, est-ce bien nécessaire de stocker la saisie complète ? a. Pour le besoin de formidable, si on a juste le name, on peut faire le set_request_null() b. Pour les usages ailleurs... bah pour l'instant on n'en voit pas.

Bé là il se trouve que si on fait bien 79, ya plus besoin pour saisies_verifier() mais tu vois que si on en avait eu besoin, il aurait absolument fallu avoir toutes les infos complètes, puisque pour chacune le but était de lancer la vérif décrite pour avoir la normalisation. Donc il y a bien des cas où on peut vouloir plus que le name ou l'id.

Ça coute pas plus cher de tout avoir, et au moment où on le fait on passe bien sur chacune des saisies, donc autant copier le tableau entier directement dans la liste.

Bé là il se trouve que si on fait bien 79, ya plus besoin pour saisies_verifier() mais tu vois que si on en avait eu besoin, il aurait absolument fallu avoir toutes les infos complètes, puisque pour chacune le but était de lancer la vérif décrite pour avoir la normalisation. Donc il y a bien des cas où on peut vouloir plus que le name ou l'id. Ça coute pas plus cher de tout avoir, et au moment où on le fait on passe bien sur chacune des saisies, donc autant copier le tableau entier directement dans la liste.
maieul referenced this issue from a commit 4 weeks ago
maieul commented 4 weeks ago
Poster
Collaborator

étape 1,2,3 faites. Avec en plus un peu de refactoring et de réorga de code. Reste la 4. Ca attendra demain !

étape 1,2,3 faites. Avec en plus un peu de refactoring et de réorga de code. Reste la 4. Ca attendra demain !
maieul commented 4 weeks ago
Poster
Collaborator

voilà c'est coder... avec du coup des branches dans saisies/formidable/verifier et champ_extra.

J'en ai profité pour réorganiser une partie du code de saisies, pour dispatcher en plusieurs fichiers des fonctions qui étaient dans inc/saisies.php

Testés avec champ extra sur les cas signalés+formidable.

Je sais pas si tu veux tester @rastapopoulos où si je merge/release comme cela ?

voilà c'est coder... avec du coup des branches dans saisies/formidable/verifier et champ_extra. J'en ai profité pour réorganiser une partie du code de saisies, pour dispatcher en plusieurs fichiers des fonctions qui étaient dans inc/saisies.php Testés avec champ extra sur les cas signalés+formidable. Je sais pas si tu veux tester @rastapopoulos où si je merge/release comme cela ?
maieul referenced this issue from a commit 3 weeks ago
maieul referenced this issue from a commit 3 weeks ago
maieul commented 3 weeks ago
Poster
Collaborator

c'est mergé et releasé.

c'est mergé et releasé.
maieul closed this issue 3 weeks ago
Sign in to join this conversation.
No Milestone
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.