Major break into 20.0 : revert to fetchAll signature

[rycks]

There is a major code break in dolibarr 20.0 with a incredible function signature change ... fetchAll switch from array to string argument for $filter which cause major breaks into all dolibarr ecosystem

That PR is a work in progress to open exchanges about that problem and don't waist my time if you refuse that PR but please consider it.

The original change was done without PR and with no review.

This PR is a solution which cause no break down with all other tools.

More details on french forum if needed: https://www.dolibarr.fr/forum/t/question-ouverte-sur-le-dev-du-coeur-de-dolibarr/46868

[eldy]

When there is a compatibility change into the code, there is often
1-some code added to manage and keep the compatibility to avoid to introduce bugsbug or
2-a mention in changelog when compatibility may be broken.

Or in a better world, we can have the two.
But to be able to implement the 1 if it was forgotten, can you describe how to reproduce the bug ?

[rycks]

eldy,
it seems you made a big commit without respecting your own rules (contributing.md) or maybe i didn't find the PR linked to that commit ? c544efe

then now i have to justify "my" PR wich is (i think) a better solution because it does not break ascending AND descending compatibility with $filter array type of argument and different versions of dolibarr and ALL the external module ecosystem

your initial problem was about customsql syntax who could be a security hole is that true ?

ok but the problem is not the array, the problem is what is stored into the array, then we could depreciate the old 'customsql' part of array and use new ('uss' for example) par of the same array

and really, please focus me on the initial PR where i could make comments or explain me how we could work in same situation but please don't apply different rules between you and other.

[eldy]

eldy,
it seems you made a big commit without respecting your own rules (contributing.md) or maybe i didn't find the PR linked to that commit ? c544efe

then now i have to justify "my" PR wich is (i think) a better solution because it does not break ascending AND descending compatibility with $filter array type of argument and different versions of dolibarr and ALL the external module ecosystem

your initial problem was about customsql syntax who could be a security hole is that true ?

ok but the problem is not the array, the problem is what is stored into the array, then we could depreciate the old 'customsql' part of array and use new ('uss' for example) par of the same array

and really, please focus me on the initial PR where i could make comments or explain me how we could work in same situation but please don't apply different rules between you and other.

Sorry, my question was:
"Can you describe the process to reproduce the bug, soni can validate the PR"
Your answer was:
You do not apply the rule in contribing.

What is the relation between this question to justify this PR ?

It seems important to note one point : the contributing is the rule that explain how a developer can suggest one or several commits to ask another developer who has the role of Merger if its change can be approved. Here you mention that a rule is not followed because the Merger didn't ask the Merger (so to itself) if he is ok with a change he did himself to make the code valid for integration.
I can't imagine you do not make difference between a Pull Requester and the Code approver.
So i am not sure it is really a serious question ? Or an attempt to change the topic of the PR, in the hope of disturbing the attention and having the PR validated without giving the expected information ?

So, even of this point has no relation with this PR and should not appear shared in this ticket, I have just made a completion into the CONTRIBUTING.MD file to add a note to make the difference clearer.

For this PR, my understanding is that there is no way to reproduce any bug or regression in v20, so I close it.

[rycks]

waw ! that's magic.

you have two roles : merger and developper.

when you act as a developper did you have to respect contributing rules ? apparently no

all others developpers MUST follow that contributing rules but not you ?

then because you (as developper) did not follow the rules (you don't make a PR) i can't comment your commit then we don't have any chance to purpose other (maybe better) solutions than yours ???? that's magic ... there is no solution for that sort of situation ? please don't stay in a hole and be a little open minded ! do i have to make a new pr with more consensual title ? is that the problem ?

[eldy]

waw ! that's magic.

you have two roles : merger and developper.

when you act as a developper did you have to respect contributing rules ? apparently no

all others developpers MUST follow that contributing rules but not you ?

then because you (as developper) did not follow the rules (you don't make a PR) i can't comment your commit then we don't have any chance to purpose other (maybe better) solutions than yours ???? that's magic ... there is no solution for that sort of situation ? please don't stay in a hole and be a little open minded ! do i have to make a new pr with more consensual title ? is that the problem ?

Yes, I can also make code myself in addition to the role of Code Approval. This is not forbidden for the Code Approver to be also a Developer. This is not a secret, so i may have two roles. And the contributing.md file has been clarified to mention that even if this is implicit since a longtime: the code approver does not need to request the approval of its code change to himself. Just the common sense. May be this will change in a future (for example if duplicate approvals is put in place, this will be necessary), but currently this is the rule since the first line of code of the projet (so this is not Magic, just a fact). It was already the case with my predecessor, the previous Code Approver (Rodolphe Quiedeville) and author of Dolibarr, like it was for the Backup Code Approver Juanjo, there is no change about this. Note that this may change in future (but I do not see the relation between this point and the code of this PR).

Also you mention "i can't comment your commit". Why ? Everybody should be able to post comments on commit (in fact, most comments are done on commits, they are even done on a given line of a file of a commit). I suggest you to contact github support if this is not true for you.

And Yes also, once the code approver has validated commits, the code is definitely merged and, if there is need to change the validated code, different commits must be done onthe base of the validate code. And like any other change, it can be done by me or by any developer, meaning that anybody can suggest a different solution (even if the last post try to say the opposite in a crusade of disinformation started on the forum ). But having everybody able to suggest different solution does not mean that the suggestion will be accepted !
If the suggestion is not valid, it just won't be.

Answer in relation with this PR:
I am really sorry but i won't integrate some code to fix a major break that does not exists. Disinformation or denigration will not change me the way I do the job of Control Quality and will not force me to accept contributions that should not, even if contribution need a lot of time to be done:

• "If there is no bug to fix, then no code to fix this non existing bug will be merged"
• "If the PR is a PR to change the code quality (so not a bug fix), that both introduce technical debt and increases the risk of breaking something already stable, and has no valid strong justification, I will not validate the PR too (and for me a suggestion of code saying there is compatibility break because we changed the default variable in phpdoc, when the code analysis + tests specific to this case shows there is no compatibility break, is not a valid strong justification)".

Sorry, Code Quality of Dolibarr takes priority on my image.

The parameter $filter that is now a USF string must remains a string by default (this is the new and final way to pass the USF so no technical debt is introduced by having it defined as default). If you really want a second compatiblity layer by being able to pass the USF in the old array, this is possible (useless but possible), but it must be inside the old way of caling fetchAll insidethe $filter that is an array.