fix(BazarFields): change canEdit signature and create formatValuesBeforeSaveIfEditable methods

[J9rem]

BREAKING_CHANGE custom fields must change canEdit signature

je crée cette PR pour résoudre un souci d'accès aux données par les champs lors de la création et de l'usage du droit d'écriture %. En effet celui-ci n'est pas bien pris en compte car la page n'existant pas encore, il n'est pas possible de vérifier que la personne connectée en est propriétaire et il faut forcer le mode creation à la main.
La détection en place jusque là fonctionne pour renderIfpermitted mais pas pour formatBeforeSave.

je propose cette PR pour permettre le passage du paramètre isCreation au bon endroit.

Il y a un BREAKNG_CHANGE concernant la signature de la méthode BazarField::canEdit.
J'attends donc que nous travaillions sur doryphore 4.3 pour mettre en place la fusion.

en attendant, est-ce qu'un de vous @mrflos ou @acheype aurait un retour à me faire sur les points d'attention que je dois avoir avant de fusionner ? Merci d'avance.

[VeveQNV]

Selon moi le test CanEdit n'a pas sa place dans une fonction qui est censé formater l'entrée avant de la sauver. Si l'entrée peut etre sauvée c'est qu'elle a été validé en amont comme pouvant etre sauvée. Il faudrait en amont éliminer les valeurs du formulaire qui ne peuvent pas etre éditée.

Il serait judicieux d'avoir la meme approche pour renderifpermitted meme si je comprend que par commodité on puisse le conserver. Cette fonction ne devrait etre appelée que par une couche du dessus qui appelle le template si il peut etre affiché plutot qu'un template qui l'appele dans tous les cas et qui ne l'affiche que si c'est permis.

[VeveQNV]

Je ne suis pas sur que ce soit lié uniquement à l'utilisation de % puisque la fonction canedit se base sur la présence ou non de l'entrée en pamamètres. Il faudrait creuser également de la fonction check.

public function canEdit($entry)
{
$writeAcl = empty($this->writeAccess) ? '' : $this->writeAccess;
$isCreation = !$entry;
return empty($writeAcl) || $this->getService(AclService::class)->check($writeAcl, null, true, $isCreation ? '' : $entry['id_fiche'], $isCreation ? 'creation' : 'edit');
}

[J9rem]

@VeveQNV j'ai l'impression que tu n'as pas vu ni testé ma proposition de correctif car normalement ça corrige bien le souci dont tu parles d'après mes tests, est-ce que tu pourrais regarder le nouveau code que je propose qui est différent de celui que tu as cité pour que tu me dises si ça prend bien en compte le cas dont tu parles.

Le plus simple serait d'attendre notre prochain RDV pour l'usage de Github pour que tu puisses plus facilement tester les propositions que je fais.

[VeveQNV]

Ok. Désolé. J'ai regardé : ca fait sens. On en reparle plus tard.

[VeveQNV]

Salut,

j'ai re-regardé.

Sauf erreur, le check des acls avant la sauvegarde pour les fields semble effectivement réalisé en amont dans la fonction update de EntryController qui utilise assignRestrictedFields pour le faire.

C'est cohérent selon moi.

Ainsi selon moi le check CanEdit dans FormatValuesBeforeSave est inutile. Il n'a pas de sens non plus et est une source d'erreur dans les développements futurs.

Le sens de FormatValuesBeforeSave est de formater les données saisies afin de les rendre conforme. Il s'agit d'une fonction désirée par le noyau et indépedante des droits d'écriture.

Dans mon cas, on a empeché le noyau de faire son travail et l'entrée a tout de meme été sauvée sous un mauvais format. Selon moi c'est un choix structurel au niveau du code qui génère des difficultés.

J'ai supprimé CanEdit de mon code et ca marche. Pour le moment, je ne vois rien qui contredit mon raisonnement.

Concernant renderIfPermitted, je ne pense pas qu'il faille y voir une similitude. L'enregistrement des données ou l'affichage à l'écran semble 2 concepts similaires c'est vrai, on met des données quelquepart.

Cependant la différence ici est la cohabitation avec VueJS, Twig et toute la moulinette qui se charge d'une bonne partie du travail.

Ceci étant dit : la logique devrait etre identique : les templates d'affichage ne devraient etre appelés QUE SI la variable peut etre affichée plutot qu'appelé le template et que celui ci décide si il peut l'afficher ou non.

Ce n'est pas son role selon moi. Ce travail doit etre réalisé en amont.

[J9rem]

fusion de cette branche d'arrière boutique pour stabiliser les écritures pour les champs en droits d'écriture %.
On pourra discuter de l'intérêt ou non de canEdit dans une issue dédiée.
En effet, je trouve que discuter de l'intérêt de canEdit ici s'éloigne de la finalité même de la présente Pull-request. Celle-ci a pour objectif de corriger un bug, ce que ça fait et non de changer le comportement de canEdit

[VeveQNV]

Je comprends. Ceci étant dit supprimer le test CanEdit semble corriger le bug. De mon coté, l'analyse du code semble montrer que ce test est inutile puisque le test des ACLs est réalisé en amont, ce qui est logique. Il me semble important de considérer que l'architecture des algorithmes participe à leur solidité. Si certaines choses sont "mal concues", cela a des effets de bords, et ce bug en est un exemple. Selon moi ce test est "inutile et dangeureux". Désolé pour les termes de ce message. J'espère que vous êtes ouvert à la critique. On en reparlera plus tard, si vous le voulez bien. A bientot, Yves

[J9rem]

@VeveQNV je suis ouvert à la critique, c'est juste que tu la fais à un mauvais endroit pour que nous puissions en discuter facilement. Est-ce que nous pourrons voir ceci à l'oral ce jeudi lors de la visio (en fin de visio) ?

concernant ta remarque, le test des ACLS n'est fait en amont que pour les données qui correspondent au champ par ces lignes:

• yeswiki/tools/bazar/services/EntryManager.php

  Line 581 in 5c7fe74

  public function update($tag, $data, $semantic = false, $replace = false)

• yeswiki/tools/bazar/services/EntryManager.php

  Line 600 in 5c7fe74

  $data = $this->assignRestrictedFields($data, $previousData, $form);

• yeswiki/tools/bazar/services/EntryManager.php

  Line 641 in 5c7fe74

  protected function assignRestrictedFields(array $data, array $previousData, array $form)

si le champ en question utilise d'autres données dans le formulaire, celles-ci ne seront pas nettoyées par EntryManager->assignRestrictedFields
Actuellement, avant de rajouter une nouvelle méthode, il n'y a que la méthode BazarField->formatValuesBeforeSave qui permet de définir ce qu'il faut nettoyer dans la liste des données qui viennent du formulaire et qui n'auraient pas été nettoyées par EntryManager->assignRestrictedFields.

Pour le faire, il faut donc vérifier à nouveau dans BazarField->formatValuesBeforeSave pour les champs concernés si le champ peut être édité, et si ça n'est pas le cas, alors il faut provoquer le nettoyage correcte des valeurs en trop.

Donc pour le moment, la méthode EST nécessaire
Effectivement, supprimé le canEdit qui est à l'intérieur de BazarField->formatValuesBeforeSave pour les champs concernés résout le bug car c'est bien à cet endroit que le bug à lieu MAIS ça changerait alors le comportement des champs concernés.

La présente pull request avait pour objectif de corriger un bug, ce que ça fait, sans changer le comportement.

Toutefois, pour imaginer une autre façon de faire, je trouve que affronter nos avis ici ne facilite pas le respect entre nous car le non verbal n'est pas présent et le faire dans une issue dédiée éviterait de polluer la présente PR.

Je propose donc de patienter quand nous aurons le temps de faire une concertation entre développeurs pour déterminer comment déplacer ce test.
J'ai ouvert une nouvelle issue #963 où je propose la migration de la discussion écrite en attendant la concertation orale.