Fix: deprecations in repair.php & Qual: Improve ... phpunit/setup_conf.sh

[mdeweerd]

Fix: deprecations in repair.php

repair.php was making calls to explode(string, null) (reported as deprecations by php).

Qual: Improve dev/setup/phpunit/setup_conf.sh

• Use variables for credentials instead of repeating hardcoded values;
• Add variables to configuration parameters;
• Add prefix to tables;
• Execute repair.php.

[mdeweerd]

@eldy
FYI, after the setup of the database some tmp tables exist (shown further below).
I think that some tables were present due to premature interruption of the setup script, but the ...accouting... table is something I had on my produciton server as well.

One seems to be an issue with deleting in an old upgrade script (see below), the others are not so obvious.
Maybe the repair script should cleanup such tables as well?

upgrade800900.log:2024-09-01 21:42:19 DEBUG     25440         sql=CREATE TABLE tmp_llx21_accouting_account AS SELECT MIN(rowid) as MINID, MAX(rowid) as MAXID, account_number, entity, fk_pcg_version, count(*) AS NB FROM llx21_accounting_account group BY account_number, entity, fk_pcg_version HAVING count(*) >= 2 order by account_number, entity, fk_pcg_version;<br>
upgrade800900.log:2024-09-01 21:42:19 DEBUG     25440         sql=DELETE from llx21_accounting_account where rowid in (select minid from tmp_llx21_accouting_account where minid NOT IN (SELECT fk_code_ventilation from llx21_facturedet) AND minid NOT IN (SELECT fk_code_ventilation from llx21_facture_fourn_det) AND minid NOT IN (SELECT fk_code_ventilation from llx21_expensereport_det));<br>

[eldy]

Yes, the repair can delete some tables but rule is only if table is no more used since several version (we try to keep temporary table content several version after migration to allow to fix of data if there was a bug in migration).

[edupulpillo]

Trying to help to improve the repair tool, another issue with accounting tables:
trying to update the charset and collation using
https://www.mydomain.com/install/repair.php?force_utf8mb4_on_tables=confirmed
we always get an error in the tables accounting_account and accounting_system because exist a foreign key.
So i guess the foreign key should be deleted, then apply the update of the charset and collation, and create again the foreign keys.
This problem also provocates that the update script of Dolibarr fails from 19.0.2 to 19.0.3 be interrupted.
can you please update the repair tool?

[eldy]

Trying to help to improve the repair tool, another issue with accounting tables: trying to update the charset and collation using https://www.mydomain.com/install/repair.php?force_utf8mb4_on_tables=confirmed we always get an error in the tables accounting_account and accounting_system because exist a foreign key. So i guess the foreign key should be deleted, then apply the update of the charset and collation, and create again the foreign keys. This problem also provocates that the update script of Dolibarr fails from 19.0.2 to 19.0.3 be interrupted. can you please update the repair tool?

Thanks for warning. Fixed in v20 branch.

[mdeweerd]

Yes, the repair can delete some tables but rule is only if table is no more used since several version (we try to keep temporary table content several version after migration to allow to fix of data if there was a bug in migration).

That's a good approach, but the DELETE is using accounting and the table is accouting... for one of the cases.
I did not find the other cases with a simple lookup, but I observe that: the naming is not uniform (tmp_user, not tmp_llx_user (or tmp_<prefix>user ). And speaking of prefix, it would be more logical to use <prefix>_tmp_user or <prefix>_users_tmp than tmp_....
Not having any prefix in tmp_user can result in name clashes (probably rare, but possible).
Also not sure why the prefix I configured was not used for the tmp tables where a prefix was added (expected:llx21).

[eldy]

All tables:

• tmp_name of table without prefix are temporary table. Used temporarly during migration or repair. Not used by application code (so out of scope of the prefixed tables that are production tables). Tables can be removed with to effect. Yes having a prefix for such tables with a name like tmp_llx_name would have be better choice to avoid collision when using 2 instances in same database. Not that collision will have not effect because temporary table are dropped each time we need it, except we may lose the data of the table in case of error detected later and the second instance was also migrated.
• the table llx_accounting_bookkeeping_tmp is not a temporary table. It is a production table required by application. A better name would have been llx_accounting_bookkeeping_draft

[mdeweerd]

• the table llx_accounting_bookkeeping_tmp is not a temporary table. It is a production table required by application. A better name would have been llx_accounting_bookkeeping_draft

Here the table is tmp_<prefix>accouting_account (missing n).

[eldy]

• the table llx_accounting_bookkeeping_tmp is not a temporary table. It is a production table required by application. A better name would have been llx_accounting_bookkeeping_draft

Here the table is tmp_<prefix>accouting_account (missing n).

Beurk.
I fixed the n, and i added the clean of this table in v21 migration. It was created for v9 migration so we can say we let this temporary data during an enougth long time.

[edupulpillo]

Thanks for warning. Fixed in v20 branch.

My pleasure.
Kindly can you please fix it also in 19.0.x branch to fix it also in next 19.0.4 version?

Note: our tables have prefix. The real names of our tables are llxhh_accounting_account and llxhh_accounting_system . The tool https://www.mydomain.com/install/repair.php?force_utf8mb4_on_tables=confirmed always answer KO in the both tables, and also the both tables can not be repaired, and the fields are not updated with the utf8mb4, since a foreign key exist. Then the upgrade script fails.

So maybe a good approach recommended by @sonikf here #30686 (comment) is:
ALTER TABLE llx_accounting_account DROP INDEX idx_accounting_account_fk_pcg_version;
ALTER TABLE llx_accounting_account ADD INDEX idx_accounting_account_fk_pcg_version (fk_pcg_version);
to be able to recreate the index with the new collation and charset, but undfortunately we can not drop the index, and is returned an error #1553 - Cannot drop index 'idx_accounting_account_fk_pcg_version': needed in a foreign key constraint

Approach1: in case the data in the both tables are production data needed to keep:
Step1: drop indexes (How to do it?)
Step2: update collation in all the fields of the both files.
Step3: add indexes

Aproach2: In case is no problem to loose the data of the accounting.
Disable the double accounting module, then delete the both tables, and activate the double accounting module again to create from scratch the both tables. Please can you confirm me if this approach is safe ?