FIX 17.0 - collisions in cache for dol_getIdFromCode

[atm-florianm]

FIX cache collisions

The cache mechanism of this function is sometimes broken because it doesn't store all query parameters, resulting in collisions. In practice, this can mean that the wrong value will be returned.

In practice, there are not many cases where this bug will manifest itself, but it will occur in scripts that process similar data across multiple entities.

Minimal case for reproducing:

<?php

include '/path/to/dolibarr/htdocs/main.inc.php';

$nameByCode = dol_getIdFromCode($db, '12', 'c_departements', 'code_departement', 'nom', 0);
$nameById = dol_getIdFromCode($db, 12, 'c_departements', 'rowid', 'nom', 0);

$cache_codes = []; // reset cache to get the correct value
$correctNameById = dol_getIdFromCode($db, 12, 'c_departements', 'rowid', 'nom', 0);

echo "Expected: byCode=$nameByCode, byId=$correctNameById\n";
echo "Actual:   byCode=$nameByCode, byId=$nameById\n";

$idPrelevementEntity1 = dol_getIdFromCode($db, 'PRE', 'c_paiement', 'code', 'id', 0, ' AND entity = 1');
$idPrelevementEntity2 = dol_getIdFromCode($db, 'PRE', 'c_paiement', 'code', 'id', 0, ' AND entity = 2');

$cache_codes = []; // reset cache to get the correct value
$correctIdPrelevementEntity2 = dol_getIdFromCode($db, 'PRE', 'c_paiement', 'code', 'id', 0, ' AND entity = 2');

echo "Expected: entity1=$idPrelevementEntity1, entity2=$correctIdPrelevementEntity2\n";
echo "Actual:   entity1=$idPrelevementEntity1, entity2=$idPrelevementEntity2\n";

Here is the output I get:

Expected: byCode=Aveyron, byId=Alpes-Maritimes
Actual:   byCode=Aveyron, byId=Aveyron
Expected: entity1=3, entity2=105
Actual:   entity1=3, entity2=3

Analysis

This occurs because the functions enables the user to use 6 querying parameters, but only 3 are part of the cache key, meaning that if you change any of the 3 remaining parameters, you will get results from the cache even though they don't match all your criteria.

My proposed solution is to use all 6 parameters as keys. I am aware that this could be wasteful in some cases (because the same result will be stored twice if, for instance, you use two totally equivalent but different filters in $filter. However, this will at least return the expected result.

In addition, an optional parameter allows the programmers to skip the cache entirely. This parameter is new, but it won't break backward compatibility since it has a default value and up to the current develop branch, there are no other new parameters.

[atm-florianm]

Note: I do have the pre-commit hook enabled, so I don't understand why it fails here (especially as it fails on the very whitespace changes that my local pre-commit hook forced me to make). I'll check that my pre-commit hook is up to date.

[hregis]

@atm-florianm just replace the word "Overide" by "Override"

[hregis]

@atm-florianm otherwise you will get scolded by @mdeweerd 😄

[atm-florianm]

@hregis Thanks! The typo was already there, though. But I guess my old pre-commit changed whitespace on the lines where it appeared.

[eldy]

Why using static instead of global ?

[atm-florianm]

It avoids overcrowding the global namespace and ensures the variable stays private (can't be read or written from the outside, which can be good or bad depending on the situation). I saw that in v21, this variable is replaced with $conf->cache, which I assume is used by other functions, so in this regard, a global variable makes more sense.
Feel free to revert this line.

[mdeweerd]

@atm-florianm otherwise you will get scolded by @mdeweerd 😄

'scolded' is a big word, codespell does the job ! I explain why, and @hregis you are clearly progressing.

@atm-florianm The php-lint issues you are having depend as far as I remember on the local php version you have installed.
This step simply executes 'php -l ' on the concerned php files.
Apparently the issue already exists in 17.0.

php -l htdocs/includes/maximebf/debugbar/src/DebugBar/DataCollector/PDO/TraceablePDO.php

Lists several notices and one fatal error for me with PHP 8.2.12 .

FYI, this is my local.sh script called by pre-commit (I still use cygwin, but you can get ideas from it).

#!/bin/bash
#exit 0
MYDIR=$(dirname "$0")
if [ -r "$MYDIR/dev/tools/updatelicense.php" ] ; then
        git diff --name-only HEAD ':!*/includes/*' ':!htdocs/modulebuilder/template/*' | sort -u | xargs -r "$MYDIR/dev/tools/updatelicense.php"
elif  [ -r "$MYDIR/dev/tools/fixcopyrightheader.php" ] ; then
        git diff --name-only HEAD ':!*/includes/*' ':!htdocs/modulebuilder/template/*' | sort -u | xargs -r "$MYDIR/dev/tools/fixcopyrightheader.php"
fi
paths=()
for f in "$MYDIR/flow/"* "$MYDIR/flow/".* ; do
        if [ "$(basename "$f")" = ".pre-commit-config.yaml" ] ; then continue ; fi
        if [ "$(basename "$f")" = ".codespellrc" ] ; then continue ; fi
        if [ ! -r "${MYDIR}/$(basename "$f")" ] ; then
                echo cp "$f" "${MYDIR}/$(basename "$f")"
                cp "$f" "${MYDIR}/$(basename "$f")"
        fi
done

for f in $(git diff --name-only HEAD ':*.php' ':!*/includes/*' | sort -u) ; do
        echo "Fix '$f'"
        paths+=( "$(cygpath -w "$f")" )
        phpcbf "$(cygpath -w "$f")"
        php-cs-fixer fix "$(cygpath -w "$f")"
done

for f in $(git diff --name-only HEAD ':*.sql' ':!*/includes/*') ; do
        sqlfluff fix --rules CP01 --exclude-rules CP02  "$f"
done

#git diff --name-only HEAD ':*.php' ':!*/includes/*' | xargs -r

if [ "${#paths}" != 0 ] ; then
        phpcbf "${paths[@]}"
fi

[mdeweerd]

Using the develop version for the violating file will fix pre-commit, if acceptable for 17.0:

git checkout develop htdocs/includes/maximebf/debugbar/src/DebugBar/DataCollector/PDO/TraceablePDO.php

[atm-florianm]

Using the develop version for the violating file will fix pre-commit

I've opened a separate PR: #32651 to avoid mixing changes that addresses the initial issue with code changes that only address checks from the GitHub workflow.