dolibarr icon indicating copy to clipboard operation
dolibarr copied to clipboard

Remove deprecated on future feature...

Open atm-john opened this issue 2 years ago • 1 comments

This method was integrated for future and keep développement simple. For getRows($sql), yes it's load all in memory but this will force developpers to make good query like count to count and not only selects all lines for performances.

this was introducted in 12.0 and is use in many of our modules.

atm-john avatar Oct 21 '21 14:10 atm-john

@eldy je te relance mais je pense vraiment que cette fonction peut nous faire gagner du temps, surtout que je voudrais vraiment m'en servir sur une prochaine PR de la class ListHelper en cours de dev. Lors de nos derniers échanges, d’après mes souvenirs tu l'avais déprécié à cause de la mémoire et des mauvaises utilisations potentielles, alors que, pour moi, ça ne change rien aux requêtes actuelles car les résultats d'une requête SQL sont tous chargés en mémoire de base... Je préfère donc que les dev se rendent plus facilement compte ou anticipent plus facilement la masse de données chargée forçant les développeurs à réfléchir et optimiser leurs requêtes (ex :effectuer des SQL COUNT avant les requêtes principales pour justement optimiser les accès base de données) De plus la PHP Doc de query Msqli ne donne pas de conseil sur l'optimisation alors que sur getRows elle alerte sur les risques. Et surtout on gagne en lisibilité sur le code. Notamment ça évite aussi certaines erreurs de débutant lors l'imbrication de boucles avec des requêtes SQL (réaffectation de nom de variable ex $resql)

Avec des requêtes

Du coup ?

atm-john avatar Jul 26 '22 10:07 atm-john

The function was opened during only few days and my memory say (but i have a bad memory and i may make a confusion with something else) is that all usage we had of it was a bad usage forcing to rewrite the code using an inline loop but with a counter to stop loop when too large (for example in pages for pagination context). So the experience i have on only one version with this method as not deprecated is that we saved few lines of code but we introduced a lot of trouble that we finally solved by re-adding the line saved. May be we can introduce protection to be sure this is not used for pagination, long list, etc (in such case a list with a max in loop is required), by adding a parameter maxline that is by default to 0 but if method is called with value 0, then an error is reported, so dev is forced to enter a max number of record as a parameter (the value found in select count before). So we are sure the dev understand he must use this for limited length lists and is forced to count before. What do you think ?

eldy avatar Sep 09 '22 23:09 eldy

Oops i forgot to set the flag to say an enhancement/protection is expected in this method.

eldy avatar Aug 26 '23 16:08 eldy

It seems this PR was set with "Discussion" or "PR to fix" tag, but no answer nor correction was provided since a long time. So request is automatically closed. Please reopened and add a comment if you think this is an error.

eldy avatar Dec 28 '23 12:12 eldy