Régression ? Passage-de-tableaux-en-parametres-sous-forme-de-chaine #136

Open
opened 2 months ago by g0uZ · 13 comments
g0uZ commented 2 months ago
Collaborator

Cette fonctionnalité semble cassée : https://contrib.spip.net/Doc-Saisies-complementaire#Passage-de-tableaux-en-parametres-sous-forme-de-chaine

J'ai testé sur Spip 3.2.11 et 4.0.0 avec le plugin saisies v4.0.3 :


[(#SAISIE{selection, test1, data='valeur1|texte1'})] : KO, le select est vide

[(#SAISIE{selection, test1, data=[(#VAL{'valeur1|texte1'}|saisies_valeur2tableau)]})] : OK

[(#SAISIE{selection, test2, data=[(#ARRAY{valeur1,texte1})]})] : OK

Cette fonctionnalité semble cassée : https://contrib.spip.net/Doc-Saisies-complementaire#Passage-de-tableaux-en-parametres-sous-forme-de-chaine J'ai testé sur Spip 3.2.11 et 4.0.0 avec le plugin saisies v4.0.3 : ``` [(#SAISIE{selection, test1, data='valeur1|texte1'})] : KO, le select est vide [(#SAISIE{selection, test1, data=[(#VAL{'valeur1|texte1'}|saisies_valeur2tableau)]})] : OK [(#SAISIE{selection, test2, data=[(#ARRAY{valeur1,texte1})]})] : OK ```
Collaborator

oui, effectivement, j'ai cassé cela, pour des raisons d'optimisation.

5a9d92d

il vaut mieux utiliser #GENERER_SAISIES.

oui, effectivement, j'ai cassé cela, pour des raisons d'optimisation. 5a9d92d il vaut mieux utiliser #GENERER_SAISIES.
Owner

Je ne comprends pas ta réponde @maieul ni la justification du commit

Le message de commit dit

par securité, saisies_chaine2tableau() est appelé dans les squelettes des
saisies concernée, car permet de s'assurer d'avoir systématiquement un
array. En effet, si la valeur qu'on recoit est null, alors saisies_chaine2tableau() renvoi un array qu'en même, utile pour les test sur les valeurs par défaut. On pourrait s'en passer en utilisant |in_any à la place de |in_array, mais en terme de perf on aurait une tentative de unserialize, et c'est kiff-kiff->jouons la sécurité de ne rien casser.

Alors que le commit fait le contraire en enlevant |saisies_chaine2tableau des squelettes concernés et que la feature documentée est cassée. Ça colle pas trop avec "jouons la sécurité de ne rien casser" hein

Je ne comprends pas l'argument de casser du fonctionnel pour un gain de perf qui parait tout à fait anecdotique.

Je ne comprends pas la réponse qui consiste à dire "il vaut mieux utiliser #GENERER_SAISIES"... Ou alors ça y est c'est officiel : le plugin se veut définitivement un grosse boite noire dans lequel seul l'usage des tableaux de déclaration PHP est supporté et l'utilisation de la balise #SAISIE{...} devient un usage de seconde classe ?

Enfin anecdotiquement, je pense que coder
if (substr($option, 0, 4) === 'data')
au lieu de
if (in_array($option, ['data', 'datas', 'data_rows', 'data_cols'])

est une paresse fautive qui expose à ce qu'un utilisateur retrouve son argument datatruc tout cassé sans aucune bonne raison, alors même que c'est un codage plus lent et moins efficace...

Bref, je le redis parce que je l'ai déjà dit : il faudrait faire un peu preuve de mesure et de reflexion dans les refactoring à la serpe, et la règle générale c'est qu'on ne casse une feature que si on est obligé et si on a vraiment une bonne raison pour ça.

Là je n'en vois aucune.

Je ne comprends pas ta réponde @maieul ni la justification du commit Le message de commit dit > par securité, `saisies_chaine2tableau()` est appelé dans les squelettes des saisies concernée, car permet de s'assurer d'avoir systématiquement un array. En effet, si la valeur qu'on recoit est `null`, alors `saisies_chaine2tableau()` renvoi un `array` qu'en même, utile pour les test sur les valeurs par défaut. On pourrait s'en passer en utilisant |in_any à la place de |in_array, mais en terme de perf on aurait une tentative de unserialize, et c'est kiff-kiff->jouons la sécurité de ne rien casser. Alors que le commit fait le contraire en enlevant `|saisies_chaine2tableau` des squelettes concernés et que la feature documentée est cassée. Ça colle pas trop avec "jouons la sécurité de ne rien casser" hein Je ne comprends pas l'argument de casser du fonctionnel pour un gain de perf qui parait tout à fait anecdotique. Je ne comprends pas la réponse qui consiste à dire "il vaut mieux utiliser `#GENERER_SAISIES`"... Ou alors ça y est c'est officiel : le plugin se veut définitivement un grosse boite noire dans lequel seul l'usage des tableaux de déclaration PHP est supporté et l'utilisation de la balise `#SAISIE{...}` devient un usage de seconde classe ? Enfin anecdotiquement, je pense que coder `if (substr($option, 0, 4) === 'data')` au lieu de `if (in_array($option, ['data', 'datas', 'data_rows', 'data_cols'])` est une paresse fautive qui expose à ce qu'un utilisateur retrouve son argument `datatruc` tout cassé sans aucune bonne raison, alors même que c'est un codage plus lent et moins efficace... Bref, je le redis parce que je l'ai déjà dit : il faudrait faire un peu preuve de mesure et de reflexion dans les refactoring à la serpe, et la règle générale c'est qu'on ne casse une feature que si on est obligé et si on a *vraiment* une bonne raison pour ça. Là je n'en vois aucune.
Collaborator

Pour info une recherche sur la zone ramene 2 plugins qui utilisent ce format, pour des saisies checkbox dans les formulaires de configuration de responsivenav et paravent.

À part ça j'aime beaucoup les saisies pour leur simplicité d'emploi direct... en HTML. Ne dépréciez pas cet usage !

Pour info une recherche sur la zone ramene 2 plugins qui utilisent ce format, pour des saisies checkbox dans les formulaires de configuration de responsivenav et paravent. À part ça j'aime beaucoup les saisies pour leur simplicité d'emploi direct... en HTML. Ne dépréciez pas cet usage !
Owner

Vu de ma fenêtre j'ai quand même l'impression que, outre les différences de point de vue sur les objectifs de ce plugin, on est un peu en limite de l'exercice et qu'il faudrait peut-être réfléchir à le refactorer voire à le découper.

J'ai vu passer des litanies surement bienveillantes de commits sur ce plugin les derniers mois et je ne trouve pas cela rassurant pour un plugin extrêmement utilisé. Cela manque de stabilité alors que le core devrait être finalement beaucoup plus stable vu les services qu'il offrent.

Et là où je rejoins Cedric à 100% c'est que la balise #SAISIE devrait être l'usage par défaut pour les formulaires simples et que l'usage du PHP devrait être réservé à des cas particuliers, ceux qui nécessitent des traitements complexes ou d'ajout de saisies par exemple.
A partir de là, les modifications de la balise #SAISIE devrait être très limitée et ne pas introduire de régression sauf en cas extrême.

Un plugin autant utilisé devrait faire l'objet de plus de discussions avant modification. Ce n'est pas parce qu'on a des tickets et des PR qu'on peut aussi modifier à tout va en disant tu n'avais qu'à lire les tickets et les PR.
Mais je sais que c'est difficile car on est pas tous disponibles au moment opportun.
On a rarement ce type de sujet avec les plugins du core...

Mes deux sous.

Vu de ma fenêtre j'ai quand même l'impression que, outre les différences de point de vue sur les objectifs de ce plugin, on est un peu en limite de l'exercice et qu'il faudrait peut-être réfléchir à le refactorer voire à le découper. J'ai vu passer des litanies surement bienveillantes de commits sur ce plugin les derniers mois et je ne trouve pas cela rassurant pour un plugin extrêmement utilisé. Cela manque de stabilité alors que le core devrait être finalement beaucoup plus stable vu les services qu'il offrent. Et là où je rejoins Cedric à 100% c'est que la balise #SAISIE devrait être l'usage par défaut pour les formulaires simples et que l'usage du PHP devrait être réservé à des cas particuliers, ceux qui nécessitent des traitements complexes ou d'ajout de saisies par exemple. A partir de là, les modifications de la balise #SAISIE devrait être très limitée et ne pas introduire de régression sauf en cas extrême. Un plugin autant utilisé devrait faire l'objet de plus de discussions avant modification. Ce n'est pas parce qu'on a des tickets et des PR qu'on peut aussi modifier à tout va en disant tu n'avais qu'à lire les tickets et les PR. Mais je sais que c'est difficile car on est pas tous disponibles au moment opportun. On a rarement ce type de sujet avec les plugins du core... Mes deux sous.

Et là où je rejoins Cedric à 100% c'est que la balise #SAISIE devrait être l'usage par défaut pour les formulaires simples et que l'usage du PHP devrait être réservé à des cas particuliers, ceux qui nécessitent des traitements complexes ou d'ajout de saisies par exemple.

J'ai l'avis totalement opposé (obviously :p) : ce n'est pas à la personne qui crée un formulaire de savoir s'il va être réutilisé/étendu par d'autres plus tard, en tout cas quand il s'agit des choses partagées, contrib communes (objets, config, des plugins communs). TOUT formulaire commun (qu'on l'on maintient ensemble) peut être étendu/réutilisé par d'autres plus tard, c'est cet état d'esprit qui prévaut selon moi.

Et donc non : par défaut, SI un form est décidé avec Saisies (ceux qui n'utilisent pas du tout Saisies on n'en parle pas), alors ça devrait être en priorité en saisies explicitement déclarées. Et ensuite seulement les quelques forms vraiment super perso où on est sûr que ça sera jamais à étendre/réutiliser, là on peut les faire en #SAISIE si on préfère.

L'API déclarative est la seule manière qui permet que les champs d'un formulaire soit "découverts" magiquement ensuite par d'autres plugins, et donc réutilisés (form tout-en-un intégrant les saisies d'un autre formulaire par ex), ou étendu (insertion, déplacement, modif de label, obligatoire, ou autres options, etc), de manière hyper simple, en 3 lignes code, super propre sans regex, etc. C'est notamment le plus important pour les objets déclarés (objets qui font… l'objet de multiples réutilisations génériques et automatiques ensuite par moult autres plugins, Champs Extras, Profils, etc).

Voilà pour ce qui est de l'usage "à recommander" (après chacun fait ce qu'il veut), d'après moi.

En revanche, je suis tout à fait d'accord avec ce qui précède : ya pas à casser à tout va si on a la possibiltié de garder la compat, et que ça ne change pas grand chose à la performance.

> Et là où je rejoins Cedric à 100% c'est que la balise #SAISIE devrait être l'usage par défaut pour les formulaires simples et que l'usage du PHP devrait être réservé à des cas particuliers, ceux qui nécessitent des traitements complexes ou d'ajout de saisies par exemple. J'ai l'avis totalement opposé (obviously :p) : ce n'est pas à la personne qui crée un formulaire de savoir s'il va être réutilisé/étendu par d'autres plus tard, en tout cas quand il s'agit des choses partagées, contrib communes (objets, config, des plugins communs). TOUT formulaire commun (qu'on l'on maintient ensemble) peut être étendu/réutilisé par d'autres plus tard, c'est cet état d'esprit qui prévaut selon moi. Et donc non : par défaut, SI un form est décidé avec Saisies (ceux qui n'utilisent pas du tout Saisies on n'en parle pas), alors ça devrait être en priorité en saisies explicitement déclarées. Et ensuite seulement les quelques forms vraiment super perso où on est sûr que ça sera jamais à étendre/réutiliser, là on peut les faire en #SAISIE si on préfère. L'API déclarative est la seule manière qui permet que les champs d'un formulaire soit "découverts" magiquement ensuite par d'autres plugins, et donc réutilisés (form tout-en-un intégrant les saisies d'un autre formulaire par ex), ou étendu (insertion, déplacement, modif de label, obligatoire, ou autres options, etc), de manière hyper simple, en 3 lignes code, super propre sans regex, etc. C'est notamment le plus important pour les objets déclarés (objets qui font… l'objet de multiples réutilisations génériques et automatiques ensuite par moult autres plugins, Champs Extras, Profils, etc). Voilà pour ce qui est de l'usage "à recommander" (après chacun fait ce qu'il veut), d'après moi. En revanche, je suis tout à fait d'accord avec ce qui précède : ya pas à casser à tout va si on a la possibiltié de garder la compat, et que ça ne change pas grand chose à la performance.
Collaborator

Quelques points

  1. Sur la rupture de compat, c'est bien la raison pour laquelle cela n'avait été mis que dans 4.0 et pas en 3.0, 4.0 sur lequel j'avais insisté à l'époque pour qu'on vérifie bien ensemble ce sur quoi on cassait et ce sur quoi on ne cassait pas.
  2. Ma réponse en revanche était erronnée.
    a. Même si je pense que fondamentalement, saisies devrait être utilisé vers la normalisation en tableau de saisies plutôt qu'en #SAISIE (pour profiter notamment de la decelaration simplifié de verification + des afficher_si + de la possibilité d'extensivité comme le disait rastapopoulos), il est bien évident que, vu l'historique, le support de la balise #SAISIE doit être maintenu.
    b. Lorsque j'avais supprimé le support de |, je n'avais pas en tete le passage par #GENERER_SAISIES (mea culpa), mais #ARRAY. En effet, la syntaxe avec | permet, fondementalement, de passer un tableau clé/valeur. Elle a été pensée d'abord et avant pour la declaration de saisies en constructeur (type champ extra interface/formidable), mais pas en squelette, ou nous avons précisement dans le monde SPIP le #ARRAY. Que cela ait pu être utilisé en squelette est une conséquence indirecte et non désirée (et que cela ait été documentée comme tel également).
  3. Sur les aspects perfs : @rastapopoulos avait insisté un temps pour travailler ces questions. J'étais moins même peu chaud sachant que c'était délicat, mais précisement en raison du fait que nous allions sortir une 4.0, j'avais intégré cela. Le problème majeure se situe dans le fait que, parce que la syntaxe en | est prévu d'abord pour les constructeurs de formulaire, la fonction saisies_chaine2tableau() doit faire appel à _T_ou_typo() et c'est bien cette appel systématique qui pose souci.

Cela étant, on peut tout à fait decider de revenir en arrière.

Quelques points 1. Sur la rupture de compat, c'est bien la raison pour laquelle cela n'avait été mis que dans 4.0 et pas en 3.0, 4.0 sur lequel j'avais insisté à l'époque pour qu'on vérifie bien ensemble ce sur quoi on cassait et ce sur quoi on ne cassait pas. 2. Ma réponse en revanche était erronnée. a. Même si je pense que fondamentalement, saisies devrait être utilisé vers la normalisation en tableau de saisies plutôt qu'en `#SAISIE` (pour profiter notamment de la decelaration simplifié de verification + des afficher_si + de la possibilité d'extensivité comme le disait rastapopoulos), il est bien évident que, vu l'historique, le support de la balise `#SAISIE` doit être maintenu. b. Lorsque j'avais supprimé le support de |, je n'avais pas en tete le passage par #GENERER_SAISIES (mea culpa), mais <code>#ARRAY</code>. En effet, la syntaxe avec <code>|</code> permet, fondementalement, de passer un tableau clé/valeur. Elle a été pensée d'abord et avant pour la declaration de saisies en constructeur (type champ extra interface/formidable), mais pas en squelette, ou nous avons précisement dans le monde SPIP le <code>#ARRAY</code>. Que cela ait pu être utilisé en squelette est une conséquence indirecte et non désirée (et que cela ait été documentée comme tel également). 3. Sur les aspects perfs : @rastapopoulos avait insisté un temps pour travailler ces questions. J'étais moins même peu chaud sachant que c'était délicat, mais précisement en raison du fait que nous allions sortir une 4.0, j'avais intégré cela. Le problème majeure se situe dans le fait que, parce que la syntaxe en | est prévu d'abord pour les constructeurs de formulaire, la fonction `saisies_chaine2tableau()` doit faire appel à `_T_ou_typo()` et c'est bien cette appel systématique qui pose souci. Cela étant, on peut tout à fait decider de revenir en arrière.

Alors oui attention, précision sur le point d'origine du ticket : le fait de balancer la syntaxe "texte brut décrivant tant bien que mal un tableau" quand on est dans un squelette, n'a JAMAIS été documenté. Là il s'agit d'un article WIKI, et non de la doc, et qui dit donc "ah tiens apparemment ça marche aussi comme ça". Ce qui ne signifie absolument pas que c'est prévu et supporté officiellement.

La syntaxe en texte brut est prévue uniquement parce que dans le constructeur (Extras, Formidable), quand on génère l'interface pour configurer les options, c'est la manière la plus simple de faire remplir par interface une suite de clés/valeurs.

Mais en revanche, quand on est dev et qu'on écrit soi-même tout dans le code, on DOIT utiliser un tableau, il y a tout ce qu'il faut pour ça, array() en PHP et #ARRAY en squelette, et pas du tout du texte brut.

Donc pour ce point précis : non ce n'est pas supporté, et ça devrait être enlevé du wiki, et je ne suis pas d'accord pour dire que ce point devrait être supporté à l'infini sans rien casser.

Alors oui attention, précision sur le point d'origine du ticket : le fait de balancer la syntaxe "texte brut décrivant tant bien que mal un tableau" *quand on est dans un squelette*, n'a JAMAIS été documenté. Là il s'agit d'un article WIKI, et non de la doc, et qui dit donc "ah tiens apparemment ça marche aussi comme ça". Ce qui ne signifie absolument pas que c'est prévu et supporté officiellement. La syntaxe en texte brut est prévue uniquement parce que dans le constructeur (Extras, Formidable), quand on génère l'interface pour configurer les options, c'est la manière la plus simple de faire remplir *par interface* une suite de clés/valeurs. Mais en revanche, quand on est dev et qu'on écrit soi-même tout dans le code, on DOIT utiliser un tableau, il y a tout ce qu'il faut pour ça, array() en PHP et #ARRAY en squelette, et pas du tout du texte brut. Donc pour ce point précis : non ce n'est pas supporté, et ça devrait être enlevé du wiki, et je ne suis pas d'accord pour dire que ce point devrait être supporté à l'infini sans rien casser.
Collaborator

Ps : j'ajouterai un point plus personnel/humain. On est d'accord que ce plugin joue un rôle très important, et qu'il doit donc être très surveillé. Mais quand on recoit peu d'écho à nos questionnements, ou quand les seuls echos qu'on recoit c'est "oulà cela m'a l'air compliqué", cela ne donne pas particulièrement envie. J'ajouterai que ces derniers temps, j'ai investi de l'energie pour

  • séparer du code entremelé,
  • simplifier certaines fonctions en les séparant
  • tout en gérant des cas pour lequelle le système n'était pas prévu

Donc qu'il y ait un commit à la serpe, correspondant à un objetcif global "optimiser", sur un usage secondaire est pas vraiment documenté...

Ps : j'ajouterai un point plus personnel/humain. On est d'accord que ce plugin joue un rôle très important, et qu'il doit donc être très surveillé. Mais quand on recoit peu d'écho à nos questionnements, ou quand les seuls echos qu'on recoit c'est "oulà cela m'a l'air compliqué", cela ne donne pas particulièrement envie. J'ajouterai que ces derniers temps, j'ai investi de l'energie pour - séparer du code entremelé, - simplifier certaines fonctions en les séparant - tout en gérant des cas pour lequelle le système n'était pas prévu Donc qu'il y ait un commit à la serpe, correspondant à un objetcif global "optimiser", sur un usage secondaire est pas vraiment documenté...

Point de vue "simple utilisateur" :

  • perso j'utilise environ 90% du temps #SAISIE{...} dont j'apprécie la rapidité et la simplicité d'écriture pour les formulaires
  • de ce fait #GENERER_SAISIES{...} me semble tout à fait réservé aux (rares) cas où il faut faire un truc compliqué avec prise de tête pour retrouver la syntaxe, faire le débog...
  • amha il semble important de privilégier que ce plugin reste un outil ne nécessitant pas d'être un codeur chevronné mais qui au contraire permet de rendre accessible à tous la création de formulaires
    ...ce qui implique stabilité et non-rupture de compatibilité..
Point de vue "simple utilisateur" : - perso j'utilise environ 90% du temps `#SAISIE{...}` dont j'apprécie la rapidité et la simplicité d'écriture pour les formulaires - de ce fait `#GENERER_SAISIES{...}` me semble tout à fait réservé aux (rares) cas où il faut faire un truc compliqué avec prise de tête pour retrouver la syntaxe, faire le débog... - amha il semble important de privilégier que ce plugin reste un outil ne nécessitant pas d'être un codeur chevronné mais qui au contraire permet de rendre accessible à tous la création de formulaires ...ce qui implique stabilité et non-rupture de compatibilité..
Collaborator

Par contre là ou je met un bemol @rastapopoulos c'est sur le "pas officiellement documenté", et là pour le coup "c'est notre faute" car on n'a pas de mecanisme encore dans saisies pour distinguer la generation de l'article "référence de saisies" (https://contrib.spip.net/Reference-des-saisies#option_data qui mentionne bien le pipe alors que cela devrait mentionner #ARRAY/data) de la doc en contexte dans un constructeur (qui mentionne bien le pipe).

Mais c'est un autre ticket #90.

Par contre là ou je met un bemol @rastapopoulos c'est sur le "pas officiellement documenté", et là pour le coup "c'est notre faute" car on n'a pas de mecanisme encore dans saisies pour distinguer la generation de l'article "référence de saisies" (https://contrib.spip.net/Reference-des-saisies#option_data qui mentionne bien le pipe alors que cela devrait mentionner #ARRAY/data) de la doc en contexte dans un constructeur (qui mentionne bien le pipe). Mais c'est un autre ticket #90.
Owner

J'ai l'avis totalement opposé (obviously :p) : ce n'est pas à la personne qui crée un formulaire de savoir s'il va être réutilisé/étendu par d'autres plus tard, en tout cas quand il s'agit des choses partagées, contrib communes (objets, config, des plugins communs). TOUT formulaire commun (qu'on l'on maintient ensemble) peut être étendu/réutilisé par d'autres plus tard, c'est cet état d'esprit qui prévaut selon moi.

Oui c'est pas idiot. Sauf que le principe de réalité s'applique et que 90% des formulaires ne nécessiteront jamais d'évolution externe plus facile avec la déclaration PHP j'en convient. Et même si un jour ça doit être le cas, il est assez facile de passer de #SAISIE à #GENERER_SAISIES.

Voilà pourquoi je reste sur ma position ;-).
Mais on va en rester là car on commence à dériver du ticket et la brigade de répression des dérives ticketales va nous tomber dessus.

> J'ai l'avis totalement opposé (obviously :p) : ce n'est pas à la personne qui crée un formulaire de savoir s'il va être réutilisé/étendu par d'autres plus tard, en tout cas quand il s'agit des choses partagées, contrib communes (objets, config, des plugins communs). TOUT formulaire commun (qu'on l'on maintient ensemble) peut être étendu/réutilisé par d'autres plus tard, c'est cet état d'esprit qui prévaut selon moi. Oui c'est pas idiot. Sauf que le principe de réalité s'applique et que 90% des formulaires ne nécessiteront jamais d'évolution externe plus facile avec la déclaration PHP j'en convient. Et même si un jour ça doit être le cas, il est assez facile de passer de #SAISIE à #GENERER_SAISIES. Voilà pourquoi je reste sur ma position ;-). Mais on va en rester là car on commence à dériver du ticket et la brigade de répression des dérives ticketales va nous tomber dessus.
Collaborator

@cy.altern oui tout à fait mais mon premier message était erronnée. La rupture ne concerne pas SAISIE vs GENERER_SAISIE mais le support du pipe en squelette en lieu et place du #ARRAY.

@cy.altern oui tout à fait mais mon premier message était erronnée. La rupture ne concerne pas SAISIE vs GENERER_SAISIE mais le support du pipe en squelette en lieu et place du #ARRAY.

Mais on va en rester là car on commence à dériver du ticket et la brigade de répression des dérives ticketales va nous tomber dessus.

Ouiiiiiiiiiiiiiiii ? On m'appelle ? ^^

> Mais on va en rester là car on commence à dériver du ticket et la brigade de répression des dérives ticketales va nous tomber dessus. Ouiiiiiiiiiiiiiiii ? On m'appelle ? ^^
Sign in to join this conversation.
No Milestone
No Assignees
8 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.