[SPIP Zone] [Spip-zone-commit] r107572 - _plugins_/polyhierarchie/branches/v2.0

cerdic · Novembre 15, 2017, 8:34

Ben moi je suis pas du tout d'accord avec ce revert parce que :

1/ sans polyhierarchie le critere branche fonctionne sur la table des evenements, il parait logique que {branche} étende son fonctionnement de la même façon sur une boucle EVENEMENTS que sur une boucle article

2/ je fais un commit qui explique et donne le cas d'usage que l'on améliore

3/ tu revert en disant "ça casse tout" mais en donnant aucun exemple précis reproductible que l'on pourrait essayer de résoudre — alors que je n'ai constaté aucun rupture de compatibilité

4/ quand en plus tu justifie ça par
> Quand on demande "les événements liés en polyhier aux rubriques de la branche 30" on veut bien les événements qui y sont liés, pas les événements des articles de cette branche ce qui n'a rien à voir.

je rappelle que
A/ le fonctionnement nominal et supporté du plugin agenda ce sont des événements liés aux ARTICLES, qu'il n'est jamais nulle part prévu de lier les événements aux RUBRIQUES
B/ que le fonctionnement nominal et supporté du plugin polyhierarchie c'est d'étendre le rattachement aux rubriques multiples des objets qui sont DEJA prévu pour être rattachés aux rubriques

Je comprends donc que tu dévie l'usage de ces 2 plugins pour faire des choses qui ne sont pas prévues et pour assurer ce fonctionnement dérogatoire tu supprimes un comportement nominal !

En l'occurence le fonctionnement attendu est normal du critère {branche} c'est que SI un objet n'a pas de champs id_rubrique on va chercher ce champs par jointure pour trouver l'objet attaché qui est dans la branche. Et en cas de polyhierarchie on regarde aussi les liens de cet objet attaché aux autres rubriques éventuelles.

Donc c'est ton cas d'usage dérogatoire qui est à réécrire pour fonctionner dans ce cas, soit en modifiant tes squelettes, soit en modifiant le critere branche chez toi pour déroger à cette spec.

Mais l'argument "j'ai des squelettes qui fonctionnaient parce qu'ils s'appuyaient sur un bug du plugin donc du coup il faut pas réparer le bug du plugin" ne vaut pas.

Je propose donc de revert ce revert.

--
Cédric

spip-zone-commit@rezo.net a écrit :

Author: rastapopoulos@spip.org
Date: 2017-11-15 01:47:31 +0100 (Wed, 15 Nov 2017)
New Revision: 107572

Modified:
_plugins_/polyhierarchie/branches/v2.0/paquet.xml
_plugins_/polyhierarchie/branches/v2.0/polyhier_fonctions.php
Log:
Backport de [107571] : Revert d'une partie de [96181] : non on ne doit pas aller chercher les liens indirects sur une autre table que celle demandé. Cette modif casse complètement de nombreux squelettes qui marchaient bien avant (on vient seulement de le voir et comprendre pourquoi).

Le principe des tables de liens avec objet/id_objet c'est de pouvoir les utiliser sur les objets que l'on veut. Donc ensuite quand on demande les liens de rubrique pour les objets "patates", on VEUT les liens avec "patates", pas avec un autre objet calculé et remplacé en cachette comme ça. Quand on demande "les événements liés en polyhier aux rubriques de la branche 30" on veut bien les événements qui y sont liés, pas les événements des articles de cette branche ce qui n'a rien à voir.

Si des gens ont besoin d'une fonctionnalité différente en cherchant les articles parents liés, c'est autre chose, et il faudrait un autre critère ou bien un paramètre explicite supplémentaire à ce critère. Mais le fonctionnement normal, cohérent, toujours sur l'objet demandé, doit rester le même.

Details: Connexion · GitLab

_______________________________________________
Spip-zone-commit@rezo.net - http://listes.rezo.net/mailman/listinfo/spip-zone-commit

cerdic · Novembre 15, 2017, 9:02

Et je note que le péché originel vient de ce commit

qui en toute discretion et sans aucune discussion a changé complètement la philosophie et l'esprit du plugin.

* Au départ le plugin permet d'étendre à plusieurs parents rubriques la hiérarchie des ARTICLES et RUBRIQUES
* Puis un trunk est mis en place pour étendre ça à tous les objets qui sont attachés aux rubriques => OK, ça a du sens

* Puis au détour d'un commit on dit "ah ben en fait avec ce plugin on peut attacher n'importe quel objet à une rubrique même si c'était pas prévu" => et là je dis NON

Si tu as besoin de rattacher un objet aux rubriques ça doit être déclaré explicitement.

On a donc un hack du plugin qui a été introduit et utilisé qui n'a plus rien à voir avec la fonctionnalité initiale du plugin, qui n'a pas été discuté, qui n'est pas documenté.

Tant que ça impacte pas le fonctionnement normal du plugin, pourquoi pas, on peut tolérer ce hack.

Mais à partir du moment où il existe il est certain qu'à un moment il va rentrer en conflit avec une fonctionnalité standard, et on y est.

Autant dire que c'est la merde.

--
Cédric

Cédric Morin a écrit :

Ben moi je suis pas du tout d'accord avec ce revert parce que :

1/ sans polyhierarchie le critere branche fonctionne sur la table des
evenements, il parait logique que {branche} étende son fonctionnement de
la même façon sur une boucle EVENEMENTS que sur une boucle article

2/ je fais un commit qui explique et donne le cas d'usage que l'on améliore

3/ tu revert en disant "ça casse tout" mais en donnant aucun exemple
précis reproductible que l'on pourrait essayer de résoudre — alors que
je n'ai constaté aucun rupture de compatibilité

4/ quand en plus tu justifie ça par
> Quand on demande "les événements liés en polyhier aux rubriques de la
branche 30" on veut bien les événements qui y sont liés, pas les
événements des articles de cette branche ce qui n'a rien à voir.

je rappelle que
A/ le fonctionnement nominal et supporté du plugin agenda ce sont des
événements liés aux ARTICLES, qu'il n'est jamais nulle part prévu de
lier les événements aux RUBRIQUES
B/ que le fonctionnement nominal et supporté du plugin polyhierarchie
c'est d'étendre le rattachement aux rubriques multiples des objets qui
sont DEJA prévu pour être rattachés aux rubriques

Je comprends donc que tu dévie l'usage de ces 2 plugins pour faire des
choses qui ne sont pas prévues et pour assurer ce fonctionnement
dérogatoire tu supprimes un comportement nominal !

En l'occurence le fonctionnement attendu est normal du critère {branche}
c'est que SI un objet n'a pas de champs id_rubrique on va chercher ce
champs par jointure pour trouver l'objet attaché qui est dans la
branche. Et en cas de polyhierarchie on regarde aussi les liens de cet
objet attaché aux autres rubriques éventuelles.

Donc c'est ton cas d'usage dérogatoire qui est à réécrire pour
fonctionner dans ce cas, soit en modifiant tes squelettes, soit en
modifiant le critere branche chez toi pour déroger à cette spec.

Mais l'argument "j'ai des squelettes qui fonctionnaient parce qu'ils
s'appuyaient sur un bug du plugin donc du coup il faut pas réparer le
bug du plugin" ne vaut pas.

Je propose donc de revert ce revert.

rastapopoulos · Novembre 15, 2017, 11:28

Le 15/11/2017 à 10:02, Cédric Morin a écrit :

* Au départ le plugin permet d'étendre à plusieurs parents rubriques la
hiérarchie des ARTICLES et RUBRIQUES
* Puis un trunk est mis en place pour étendre ça à tous les objets qui
sont attachés aux rubriques => OK, ça a du sens

* Puis au détour d'un commit on dit "ah ben en fait avec ce plugin on
peut attacher n'importe quel objet à une rubrique même si c'était pas
prévu" => et là je dis NON

Si tu as besoin de rattacher un objet aux rubriques ça doit être déclaré
explicitement.

Alors bien évidemment je ne suis pas du tout d'accord avec ton
argumentation.

Les objets ne sont *pas obligatoirement* rattachés aux rubriques
justement, c'est tout l'intérêt d'avoir des liens multiples dans une
table de liens !

Ce commit ne fait absolument rien de nouveau et il n'aurait pas existé
que ça n'aurait rien changé à l'affaire. En effet, le fait d'avoir une
table générique existe déjà des années et des années, et cela fait plus
de 5 ans que l'on a des projets où Polyhiérarchie est utilisé
massivement pour catégoriser des contenus de manière à la fois multiple
et arborescente *sans à aucun moment que ces contenus n'aient de
rangement principal dans les rubriques*. Ça ne date absolument pas de
2016 : depuis que polyhiérarchie existe quasiment, on l'utilise pour
classer des objets de manière bien plus complète qu'avec les mots clés.

Le fait d'avoir besoin de classer de manière multiple (comme les mots)
ET arborescentes (comme les rubriques) n'a strictement *aucun relation
directe* avec le fait que les objets aient un lien "physique" immédiat
avec id_rubrique ! On utilise ce classement depuis au moins 2011 pour de
nombreux projets, sur des fiches patrimoine, sur des œuvres d'art, sur
des événements… Ce n'est pas un mystère, le fait de l'utiliser sur
absolument n'importe quels objets a été évoqué moult fois, et c'est pour
moi l'intérêt principal de ce plugin.

Après déjà plusieurs années d'utilisation de cette super table
générique, et quand même il y a déjà plus de 4 ans, j'avais ajouté un
sous-plugin pour faciliter encore son utilisation sur tout objet qu'on
désire. Cet ajout avait été prévu pour être directement dans le plugin
Polyhiérachie, car sa table est générique, et on considère que le fait
qu'il ne fournisse par défaut d'interface que pour deux objets précis
est une scorie du passé, datant du début, mais que depuis on sait faire
mieux (et on sait même faire encore mieux avec editer_liens si la table
était au norme).

Cela a été ajouté dans un plugin séparé uniquement pour être sûr de ne
rien gêner au départ, et aussi parce que le formulaire générique fourni
(par polyhiérarchie en plus, c'était déjà prévu et fourni, juste jamais
affiché !) n'est pas super génial. Si l'interface était mieux, notamment
en utilisant l'API editer_liens, alors cette fonctionnalité devrait être
réinsérée dans le plugin directement.

Le fait donc d'avoir un nouveau contenu quelconque, et de le classer de
manière complexe comme on veut, est utilisé sur de nombreux projets,
tous en ligne en prod, avec ensuite des formulaires de filtrage qui
permette de filtrer par branche ou autre. Cela fonctionne tout à fait
normalement depuis des années, pas du tout depuis 2016. Et y compris
donc sur des objets connus et classiques comme les événements.

Exemple concret : un gros agenda où tous les événements sont dans un
même article, tout simplement car chacun des événements n'a pas de
classement "principal", cela n'a aucun sens (et même s'ils avaient un
classement principal cela ne change rien : il n'y a jamais un article
par événement, cela n'a aucun sens non plus). Ensuite chaque événement
est classé suivant des catégories, et ensuite suivant des rubriques
éditoriales du site (là où il y a les articles). Sur la page de l'agenda
complet, un formulaire de filtrage permet de… filtrer les événements
suivant les secteurs du site etc. Donc
(EVENEMENTS)
{branche_indirecte?#ENV{secteurs}}
{branche_indirecte?#ENV{categories}} etc.

--
RastaPopoulos

cerdic · Novembre 15, 2017, 1:57

RastaPopoulos a écrit :

Ça ne date absolument pas de
2016 : depuis que polyhiérarchie existe quasiment, on l'utilise pour
classer des objets de manière bien plus complète qu'avec les mots clés.

define "ON" ? Rastapopoulos chez LDD ?

Pour ma part, en tant que développeur de ce plugin, c'est un usage que je découvre et que personne n'a documenté nulle part.
Et qui *de mon point de vue* est un hack du plugin et sort du périmètre initial, dont, je le répète, le but est d'*étendre* le rattachement à plusieurs rubriques possibles des objets qui sont normalement rattachés à une seule rubrique.

Je comprends très bien que tu veuilles faire des trucs pas prévu, que tu aies ton usage etc.
Mais de là à dire "c'est comme ça qu'il doit marcher" il y a un pas.

En l'occurrence, en ce qui me concerne, ce n'est pas la spec et en ce qui concerne les EVENEMENTS je le dis encore d'autant plus sereinement que de ce côté là non plus il n'a jamais été prévu qu'ils soient attachés à des rubriques directement (là encore ce n'est absolument pas prévu comme ça ni documenté).

Donc j'insiste, la spec nominale pour une boucle
(EVENEMENTS){branche #X}
c'est d'aller chercher le id_rubrique via la table spip_articles et de prendre les événements qui sont attachés à un article de la branche #X

Cela fonctionne de cette façon nativement dans le core, sans plugin polyhierarchie, et cela doit fonctionner de la même façon avec le plugin polyhierarchie.

Qu'il y ait d'autres usages, certes, mais que cela empêche d'avoir un comportement par défaut normal et identique de cette écriture dans le cas avec/sans polyhierarchie ça me choque et je m'y oppose.

Une façon de s'en sortir est de prévoir une déclaration qui dit "ces objets là sont susceptibles d'être rattachés à des rubriques par polyhierarchie même si ils n'ont pas de champ id_rubrique" et que le critère compile dans ce cas un OR pour prendre à la fois les EVENEMENTS attachés à un article de la branche et les EVENEMENTS qui sont eux-meme dans la branche.

--
Cédric

rastapopoulos · Novembre 15, 2017, 3:18

Le 15/11/2017 à 14:57, Cédric Morin a écrit :

Une façon de s'en sortir est de prévoir une déclaration qui dit "ces
objets là sont susceptibles d'être rattachés à des rubriques par
polyhierarchie même si ils n'ont pas de champ id_rubrique" et que le
critère compile dans ce cas un OR pour prendre à la fois les EVENEMENTS
attachés à un article de la branche et les EVENEMENTS qui sont eux-meme
dans la branche.

--
RastaPopoulos

cerdic · Novembre 15, 2017, 8:22

Top, une belle sortie par le haut
Merci !

--
Cédric

RastaPopoulos a écrit :

Le 15/11/2017 à 14:57, Cédric Morin a écrit :

Une façon de s'en sortir est de prévoir une déclaration qui dit "ces
objets là sont susceptibles d'être rattachés à des rubriques par
polyhierarchie même si ils n'ont pas de champ id_rubrique" et que le
critère compile dans ce cas un OR pour prendre à la fois les EVENEMENTS
attachés à un article de la branche et les EVENEMENTS qui sont eux-meme
dans la branche.

Connexion · GitLab
Connexion · GitLab

David_Prevot · Mars 22, 2018, 3:47

Le 15/11/2017 à 05:18, RastaPopoulos a écrit :

Connexion · GitLab
Connexion · GitLab

On me signale seulement maintenant que cette modification a cassé le
multirubrique pour les sites utilisant le squelette Sarka-SPIP 3.2.36.

Amicalement

David

rastapopoulos · Mars 24, 2018, 10:34

Le 22/03/2018 à 04:47, David Prévot a écrit :

Le 15/11/2017 à 05:18, RastaPopoulos a écrit :

Connexion · GitLab
Connexion · GitLab

On me signale seulement maintenant que cette modification a cassé le
multirubrique pour les sites utilisant le squelette Sarka-SPIP 3.2.36.

Est-ce que tu peux détailler ? Parce que ce commit fait un OR donc teste
les deux, donc aucune suppression de fonctionnalité.

--
RastaPopoulos