Le 11 juin 2012 09:48, Anthony Poiret <address@hidden> a écrit :
Sorry for french language:
Bonjour à tous,
Je réouvre rapidement cette discussion - je comprend très bien le
fond de la réflexion, un core propre permet un développement plus
modulaire et donc favorise l'enrichissement des fonctionnalités par
le biais de modules externes.
J'ai cependant quelques retours à faire à ce propos. Je viens de
travailler sur le module de composition (Auguria) que des collègues
ne travaillant plus dans l'entreprise ont développés il y a un
certain nombre de version (sans doutes entre dolibarr 2.6 et 2.9).
Contexte: une demande client pour intégrer le module en version
3.1.2.
Partie du core commentée:
htdocs/product/stock/class/mouvementstock.class.php
lignes 209 à 214 // Composition module
(this is an external module) /* Removed. This code must be provided by module on
trigger STOCK_MOVEMENT if (! $error &&
$qty < 0 &&
$conf->global->MAIN_MODULE_COMPOSITION) { $error =
$this->_createProductComposition($user, $fk_product,
$entrepot_id, $qty, $type, 0, $label); // pmp is not change for
subproduct }*/
lignes 289 à 326 /** * Cree un mouvement en
base pour toutes les compositions de produits * @param
label Label of stock movement * @return
int <0 if KO, 0 if OK */ /* This function is specific
to a module. Should be inside the trigger of module instead of
core code. function
_createProductComposition($user, $fk_product, $entrepot_id, $qty,
$type, $price=0, $label='') {
dol_syslog("MouvementStock::_createProductComposition
$user->id, $fk_product, $entrepot_id, $qty, $type, $price,
$label"); $products_compo = array();
$sql = "SELECT
fk_product_composition, qte, etat_stock"; $sql.= " FROM
".MAIN_DB_PREFIX."product_composition"; $sql.= " WHERE fk_product
= $fk_product;";
$resql =
$this->db->query($sql); if ($resql) { while ($item =
$this->db->fetch_object($resql)) { if
($item->etat_stock != 0) array_push($products_compo,$item); }
$this->db->free($resql); } else {
dol_syslog("MouvementStock::_createProductComposition echec update
".$this->error, LOG_ERR); return -1; }
// Create movement for
each subproduct foreach($products_compo as
$product) {
$this->_create($user, $product->fk_product_composition,
$entrepot_id, ($qty*$product->qte), $type, 0, $label); }
return 0; }*/
Premier point: je suis parfaitement d'accord, ce
code n'a rien à faire là. (preuve: le temps requis pour le
trouver...) Deuxième point (bémol): enfin, dans un souci de
cohérence avec le reste du core de Dolibarr. D'ailleurs, dans un souci de
cohérence avec le reste du core, je me demande si implémenter les
fonction CRUD de base à l'objet MouvementStock avant de
'casser' le fonctionnement des modules dépendant de cette classe
n'aurait pas été un effort plus cohérent.
Oui en effet, reste à trouver les contributeurs prêt à corriger le code pour l'uniformiser selon le principe CRUD (principe non généralisé partout pour cause historique le plus souvent).
C'est le fond de ma remarque. Dans ce cas de figure, on se retrouve
avec un module à remettre d'équerre... Sans appui. L'objet
MouvementStock n'a simplement pas d'attribut, ni de méthode pour
qu'on puisse l'utiliser de manière intègre. J'ai bien remarqué la
tentative (louable) de définir quelques attributs pour que la
fonction run_trigger puisse avoir les params requis par le biais du
$object... (cf l.216 à l.229:)
if ($movestock &&
! $error) { // Appel des triggers
include_once(DOL_DOCUMENT_ROOT .
"/core/class/interfaces.class.php"); $interface=new
Interfaces($this->db);
$result=$interface->run_triggers('STOCK_MOVEMENT',$this,$user,$langs,$conf); if ($result < 0) {
$error++; $this->errors=$interface->errors; } // Fin appel triggers }
Rappelons que nous sommes dans la fonction _create de la class
(l.56:)
function _create($user, $fk_product, $entrepot_id, $qty,
$type, $price=0, $label='')
En l'occurence, la fonction qui était utilisée avait été intégré
dans la classe (en commentaire au début du mail). Elle rappelle
_create autant de fois que nécessaire; elle a donc besoin de tous
les paramètres de celle-ci pour un appel correct. Manquent: $type, $price et $label.
Il serait possible de réinstancier l'objet dans le run_trigger pour
obtenir les données manquante de l'objet - s'il disposait de son id.
cf l.91:
Variable $mvid qui n'est jamais rappelée où que ce soit. Les objets
de type MouvementStock n'ont juste jamais d'id.
Synthèse: - je suis pour un code clair et propre,
découpé et structuré. -
la suppression du code 'parasite' est une chose, mais elle
implique que les moyens de greffer les fonctionnalitées de ce code
par les méthodes conventionnelles soit misent en place.
On est d'accord.
Mais les besoins étant infini, c'est au développeut du module qui a le besoin de:
- soit proposer la modif au code core (mais ce point peut avior un cout que peux ne se permettre de prendre en charge le développeur externe)
- soit de demander une évolution en précisant son besoin afin que l'équipe core l'intégre. Par exemple, en demandant l'ajout des propriétés manquantes pour le trigger (l'id peut suffir).
- si le but est de forcer
les développeur du module à reprendre le core pour pouvoir
appeller des méthodes standards, je trouve cela profondément
contre-productif. Cela fausse les chiffrages lorsque les
développement sont des réponses à des besoins clients, et je ne
pense pas avoir besoin d'exprimer les conséquence d'un tel état de
fait.
Le but est plutot de
- supprimer le code mort.
- forcer le développeur a réfléchir pour s'en sortir sans toucher au core code.
- quand il n'est pas possible de s'en sortir sans toucher, de forcer l'éditeur a communiquer sur son besoin pour que le core code soit adapté afin d'y répondre (car si un module a ce besoin, un autre l'aura aussi). C'est un peu plus long mais la politique de Dolibarr est de viser le long terme.
Notons que du code dont on ignore l'existence ou le nom du consommateur ne se distingue pas différemment du code réellement mort. Voila pourquoi, un module doit plutot utiliser les fonctions d'indépendance de modules plutot que du code en dur (ce dernier sera un jour ou l'autre supprimé).
L'équipe core n'ayant pas connaissance de tous les cas de développement autour de Dolibarr en terme de lignes de code (impossible), c'est à la charge du développeur du module externe de communiquer sur son besoin pour qu'une solution lui soit proposer de manière indépendante.
J'ai donc quelques propositions: 1 - Éventuellement,
laisser le code parasite s'il ne peut être externalisé par la
faute du core en lui-même. C'est la méthode 'sale', qui me
parait être la pire des propositions.
On est d'accord.
2 - Commenter le code
parasite, mais étudier son fonctionnement - surtout lorsque la
fonction appelé est elle-même sur la même page. C'est la plus
lourde en temps pour le 'commentateur'. 3 - Commenter
le code parasite, et envoyer un mail - sur la mailing list ou
directement au contact connu ayant développé le module.
Hélas, ce contact n'est pas toujours (rarement) connu. Pour ce qui est du suivi du code, chaque commit est concerné.
Aussi il est possible de s'abonner a la ml des commits réalisés.
La troisième possibilité me parait être un moindre mal. Peu
coûteux en temps et en effort. Peut-être qu'un projet libre
implique que le code des autres est le code de personne et de tous...
Dans la pratique, c'est hélas pas aussi simple. Pour du code mise a tort plusieurs année auparavant (et à ce moment la ce n'était pas forcément à tort), il est difficille plusieurs années après de se souvenir du pourquoi. Dans 95% des cas, c'est du vrai code mort inutile. Dans les 5% restant, il faut savoir qui à modifier la ligne, pourquoi et faire une enquete pour savoir si le pourquio est toujrous d'actualité. Bref, pas simple. Pour cette raison, chaque éditeur est tenu de valider ces modules sur chaque version de dolibarr au même titre qu'un éditeur traditionnel comme oracle ou sap doivent le faire quand il y a une nouvelle version d'OS ou d'API.
En attendant, lorsque le code en question est vendu par une entreprise, le responsable de la maintenance de ce code, c'est
cette entreprise. En ce sens, le commentaire sauvage, c'est du
silent-patching.
Et à mon sens, c'est plutôt décourageant en terme
d'investissement professionnel (désagréable de chiffrer une
matinée pour une modif qui finit par prendre 3 jours, exemple
arbitraire).
Le métier d'éditeur impose de prendre en compte un impact d'évolution de socle quand sa solution est basée sur un socle.
Si SAP utilise une API non officielle ou un fichier patché par ces soins de Windows, Microsoft n'ira jamais les prévenir de la modification de cette API non officielle, ni ne préviendra SAP de faire attention à son dev pour qu'il n'oublie pas de reporter sa modif lorsqu'il changera de Windows. Ce sont des points de charges propre au métier d'"éditeur dépendant", à prendre en compte par l'éditeur.
Seule l'utilisation des APIs ou mécanismes officiels documentés dans le wiki développeur (qui apparaissent petit à petit avec les nouvelles versions), peuvent réduire cet impact, mais ne le supprimera jamais complètement.
De: "Régis Houssin" <address@hidden> À: "Posts about Dolibarr ERP & CRM development and coding" <address@hidden> Cc: "Posts about Dolibarr ERP & CRM development and coding" <address@hidden>
Envoyé: Lundi 28 Mai 2012 23:10:22
Objet: Re: [Dolibarr-dev] Règle N°1
Sorry for french language:
Je suis d'accord pour commenter,
Par contre si des modules externes utilisent des fonctionnalités communes avec un fort potentiel d'amélioration de dolibarr, il me semble plus judicieux de les intégrer dans le cœur et pourront servir le cœur par la suite en transposant ce qui a été fait sur les modules externes.
Le 28 mai 2012 à 22:58, "Laurent Destailleur (eldy)" <address@hidden> a écrit :
The rule should be the opossite.
Every code that is not required must not exists. If some code is
here in advance for a future feature, it's the job of the developer
adding such feature to add comment to justify why useless code is
added to prevent it to be removed and to explain to quality guys
(guys that tracks dead code) why such a code and why it must be
kept.
When a code is useless it can be commented if we are not sure it is
used by dolibarr but if we are sure it is not (no reference to a
function for example), there is no reason to be kept.
And no, the code of other is not the code of other: The code of
other is the code of nobody or everybody (We are an opensource
project).
Le 25/05/2012 22:00, Cyrille de Lambert a écrit :
Bof !!!
Le 25/05/2012 21:56, Régis Houssin a écrit :
Règle N°1 : le code des autres c'est le code des autres ! ;-)
j'aimerais bien qu'on demande et/ou qu'on mette en commentaire le code
au lieu de le supprimer. Car si un code est présent c'est qu'il devait
servir.
Je me retrouve avec un module bancale et je n'arrive pas à retrouver le
pourquoi du comment...
Bon week end ;-)
Cordialement,