Dolibarr / dolibarr

Dolibarr ERP CRM is a modern software package to manage your company or foundation's activity (contacts, suppliers, invoices, orders, stocks, agenda, accounting, ...). it's an open source Web application (written in PHP) designed for businesses of any sizes, foundations and freelancers.
https://www.dolibarr.org
GNU General Public License v3.0
5.48k stars 2.8k forks source link

NEW: Create a DB static function to check deprecated table names #31968

Open omogenot opened 5 days ago

omogenot commented 5 days ago

It looks like in many places, there is some code to change table name from old "french" names that are deprecated throughout version releases in favour of english names for internationalisation of Dolibarr. I propose to place this code as a common static function in the DoliDB class (which is generally contained in every other class). This would allow to continue to change table names without having to check every class for usage of old names and fix it, if each class calls this function before using an SQL statement.

Usage: If object has no $db property; $table_name = DoliDB::checkTableName($table_name);

if object has a $db property: $table_name = $this->db->checkTableName($table_name);

hregis commented 3 days ago

@omogenot I don't understand the purpose of this function! Can you tell me more?

hregis commented 3 days ago

This is just var name of deprecated table name? It's poor to add this function!

omogenot commented 3 days ago

@hregis

The purpose is to centralise the capture and replacement of deprecated table names so that this is not done all over the code, with the possibility to be forgotten. I agree that the function is just a single line function, but the value is in the array of table names to be changed. By the way, the fact that the function is very small, it will have nearly no impact on performances.

To illustrate this, as an example, take the extrafields.class.php file. In nearly each function you have this repeated following code:

        if ($elementtype == 'thirdparty') {
            $elementtype = 'societe';
        }
        if ($elementtype == 'contact') {
            $elementtype = 'socpeople';
        }

which would be replaced by just one call:

                $elementtype = $this->db->checkTableName($elementtype);

Now, imagine tomorrow you decide to rename the database table facture to invoice, you will have to add one more test in each function of the extrafiels.class.php make sure that this table name change will be taken into account. With the centralised function in DoliDB, it's automatically done without adding a line of code, just adding 'facture' => 'invoice' in the static array of DoliDB. And I just took one class as an example, this table name replacement scheme is scattered in many others.

Hope I have been clear enough. Do not hesitate to contact me again if need be.

Olivier.

omogenot commented 3 days ago

J'avoue que la question posée en anglais était plus claire que ce commentaire. Et justement, le but de la manoeuvre est d'éviter de modifier toutes les classes si un nom de table devait être traduit.

hregis commented 3 days ago

@omogenot yep but the static :

static $_deprecated_table_names = array(
        'thirdparty' => 'societe',
        'contact' => 'socpeople',
        'invoice' => 'facture',
        'order_supplier' => 'commande_fournisseur',
        'categorie_extrafields' => 'categories_extrafields'
    );

is static and if you add another "static" table name you could change this array ! this is not flexible

omogenot commented 3 days ago

@omogenot yep but the static :

static $_deprecated_table_names = array(
      'thirdparty' => 'societe',
      'contact' => 'socpeople',
      'invoice' => 'facture',
      'order_supplier' => 'commande_fournisseur',
      'categorie_extrafields' => 'categories_extrafields'
  );

is static and if you add another "static" table name you could change this array ! this is not flexible

I don't understand your comment. Why is this not flexible? It's static, not private.

The table is static so that you can call it in any php file, even if you do not have access (or need to access) to a $db object.

hregis commented 3 days ago

@omogenot à mon sens il ne faut plus définir des noms en dur dans le code ! à chaque modifications il faut modifier le code... faisons en sorte d'avoir un code commun qui ne dépend plus de valeurs pré établies... mais un code qui réagit en fonction de données en base ou en constantes !

hregis commented 3 days ago

@omogenot ce code est static et nécessite de le modifier si il y a un changement

static $_deprecated_table_names = array( 'thirdparty' => 'societe', 'contact' => 'socpeople', 'invoice' => 'facture', 'order_supplier' => 'commande_fournisseur', 'categorie_extrafields' => 'categories_extrafields' );

ce code est déprécié justement !

omogenot commented 3 days ago

@omogenot à mon sens il ne faut plus définir des noms en dur dans le code ! à chaque modifications il faut modifier le code... faisons en sorte d'avoir un code commun qui ne dépend plus de valeurs pré établies... mais un code qui réagit en fonction de données en base ou en constantes !

Justement, avec cette fonction centralisée, on évite de devoir changer quoi que ce soit dans le code si on change encore de nom de table.

D'accord pour ne plus utiliser de nom en dur, mais alors que proposez vous? Une solution possible serait de définir des propriétés dans la classe DoliDB pour abstraire le nom de chaque table et on y accède par $db->INVOICE_TABLE par exemple. Mais là, pour le coup, il faut reprendre tous les fichiers php, ainsi que tous les modules externes !!! C'est pire, non?

omogenot commented 3 days ago

@omogenot ce code est static et nécessite de le modifier si il y a un changement

static $_deprecated_table_names = array( 'thirdparty' => 'societe', 'contact' => 'socpeople', 'invoice' => 'facture', 'order_supplier' => 'commande_fournisseur', 'categorie_extrafields' => 'categories_extrafields' );

ce code est déprécié justement !

Je dois être idot parce que je ne comprends pas le commentaire. S'il y a un changement de nom de table, que le code soit static ou pas, il faut bien changer le nom de la table quelque part non?

Nous ne devons pas avoir la même définition du mot static. Pour moi, le fait que le tableau (et la fonction) soit de type static indique que l'on peut l'utiliser sans avoir à instancier la classe correspondante, non pas que le code ne peut pas changer.

hregis commented 3 days ago

@omogenot il y a beaucoups de codes dans dolibarr qui devrait être déplacé dans chaque module afin de rendre les modules beaucoup moins dépendant des autres modules ! un module devrait ne pas contenir des dépendances d'un autre module... un module comme une fonction linux, ne devrait fournir que ce que le module doit fournir et seulement ce qu'il doit fournir... avec des entrées et des sorties... le module ne devrait pas contenir des contraintes d'autres modules !

hregis commented 3 days ago

@omogenot j'avais cet esprit quand j'ai mis en place le répertoire "custom"... c'est l'esprit des applications Mac OS... tout au même endroit afin de na pas en mettre de partout, ce qui simplifie les mises à jour...

hregis commented 3 days ago

@omogenot à mon sens tous les modules natifs de dolibarr devraient être modifié pour que tout leur fonctionnement soit centralisé dans leur répertoire... (admin, trigger, etc...) un module doit être autonome... des entrés et des sorties... c'est ce qu'on avait fait avec le fork... les modules dialoguaient entre eux en api REST... chaque module doit avoir ses propres fonctions et seulement ses fonctions...

hregis commented 3 days ago

le but serait d'avoir un socle commun et des modules afin de pouvoir enlever un module sans que ça pénalise le fonctionnement du socle...

omogenot commented 3 days ago

@omogenot j'avais cet esprit quand j'ai mis en place le répertoire "custom"... c'est l'esprit des applications Mac OS... tout au même endroit afin de na pas en mettre de partout, ce qui simplifie les mises à jour...

C'est prêcher un converti ! Je suis un utilisateur Mac. De la même façon, je ne peux qu'applaudir à l'utilisation du répertoire custom pour mettre tous les modules externes et je me plie de bonne grâce à cette règle.

@omogenot il y a beaucoups de codes dans dolibarr qui devrait être déplacé dans chaque module afin de rendre les modules beaucoup moins dépendant des autres modules ! un module devrait ne pas contenir des dépendances d'un autre module... un module comme une fonction linux, ne devrait fournir que ce que le module doit fournir et seulement ce qu'il doit fournir... avec des entrées et des sorties... le module ne devrait pas contenir des contraintes d'autres modules !

Là par contre, je ne comprends pas bien le sens du commentaire. Le but de la programmation objet et de toutes ces classes est de centraliser et de réutiliser un maximum de code d'un object à l'autre par héritage, en évitant d'avoir sans cesse à ré-écrire les mêmes morceaux de code à chaque fois. Après, comme dans tout développement au long cours, il y a de la sédimentation de code et du nettoyage à faire...

omogenot commented 3 days ago

@omogenot à mon sens tous les modules natifs de dolibarr devraient être modifié pour que tout leur fonctionnement soit centralisé dans leur répertoire... (admin, trigger, etc...) un module doit être autonome... des entrés et des sorties... c'est ce qu'on avait fait avec le fork... les modules dialoguaient entre eux en api REST... chaque module doit avoir ses propres fonctions et seulement ses fonctions...

Il va être compliqué ce soir de tout ré-écrire, et je pense que l'on diverge du thread original. La question initiale était de savoir à quoi servait ma proposition de fonction supplémentaire, je crois l'avoir expliqué: ne pas avoir à toujours tout ré-écrire si à l'avenir on change le nom d'une table dans la base de données. Juste s'assurer dans son code de faire appel à cette simple fonction qui s'assurera que le nom de la table est toujours valide, s'il ne l'est pas le nom correct est substitué automatiquement. C'est tout.

hregis commented 3 days ago

Là par contre, je ne comprends pas bien le sens du commentaire. Le but de la programmation objet et de toutes ces classes est de centraliser et de réutiliser un maximum de code d'un object à l'autre par héritage, en évitant d'avoir sans cesse à ré-écrire les mêmes morceaux de code à chaque fois. Après, comme dans tout développement au long cours, il y a de la sédimentation de code et du nettoyage à faire...

oui c'est ce que je dit ! un module est un objet, une classe... un module fait ce qu'on lui demande... actuellement nous avons par exemple un module "facture" qui contient du code "if module commande", "if module propal", "if module machin", etc etc... jusqu'à combien de module il va falloir faire des if ???

tu vois ce que je veux dire ?

hregis commented 3 days ago

@omogenot yep on ne va pas tout réécrire ce soir... mais il va falloir y penser sérieusement ! :-)

hregis commented 3 days ago

bonne soirée

omogenot commented 3 days ago

Là par contre, je ne comprends pas bien le sens du commentaire. Le but de la programmation objet et de toutes ces classes est de centraliser et de réutiliser un maximum de code d'un object à l'autre par héritage, en évitant d'avoir sans cesse à ré-écrire les mêmes morceaux de code à chaque fois. Après, comme dans tout développement au long cours, il y a de la sédimentation de code et du nettoyage à faire...

oui c'est ce que je dit ! un module est un objet, une classe... un module fait ce qu'on lui demande... actuellement nous avons par exemple un module "facture" qui contient du code "if module commande", "if module propal", "if module machin", etc etc... jusqu'à combien de module il va falloir faire des if ???

tu vois ce que je veux dire ?

Je vois, néanmoins il est vrai qu'un utilisateur peut activer les modules de son choix et que cela a un impact sur le déroulement général. Une facture est liée à une commande, elle même liée à une propal, etc... Mais si, comme moi, je n'active pas le module propal, il faut bien que le module facture continue à fonctionner... Pas si simple...

hregis commented 3 days ago

@omogenot il faut juste revoir l'architecture du code ! si linux fonctionnait comme dolibarr on serait obligé d'ajouter tous les packages pour qu'il fonctionne ! Un package linux fait une fonction tout en autonomie et il le fait bien, il peut utiliser d'autres packages mais il n'en est pas dépendant !