Qual: Fix some phan typing in actions_massactions.inc.php

[mdeweerd]

Qual: Fix some phan typing in actions_massactions.inc.php

Fix phan in actions_massactions.inc.php

[eldy]

$objectclass can be any Dolibarr business object here.
Is it possible to have instead a generic object class, may be CommonObject class ?

[mdeweerd]

I find often that putting CommonObject alone generates issues with the typing as some fields are missing and expected by the algorithm.

I propose to "add" CommonObject and also leave the other options that are tested for in the code. In case there is a typing issue it will indicate which classes are not compatible.

Ideally - in some future - the code would do a test using "instanceof" rather than relying on the 'element' field and the typing hints.
That implies risks of course: the "instanceof" condition could be too strict vs. what the code was actually expecting and introduce some bugs in the process.

[eldy]

I propose to "add" CommonObject and also leave the other options that are tested for in the code. In case there is a typing issue it will indicate which classes are not compatible.

You mean having this
@phan-var-force "CommonObject"|"Societe"|"Contact"|"ExpenseReport"|"Partnership"|"Holiday"|"ConferenceOrBoothAttendee"|"Facture"|"CommandeFournisseur" $objectclass ?

[mdeweerd]

Yes.

FYI, phan has more than 50 notices for this file.

For instance, we have:

htdocs\core\actions_massactions.inc.php:1288 PhanUndeclaredProperty Reference to undeclared property \CommonObject->price
htdocs\core\actions_massactions.inc.php:1289 PhanUndeclaredProperty Reference to undeclared property \CommonObject->price_min

There is no check on the object type: it is implied by the 'updateprice' action.

[eldy]

Yes, but most of them are strange. For example this one:
UndefError PhanUndeclaredProperty Reference to undeclared property \CommonObject->rowid on line 1175

The $obj is a result of fetch_object() with no declaration that it is CommonObject, so I don't understand this warning.

[mdeweerd]

That is because we have the following in the configuration for phan:

dev/tools/phan/config.php:              'obj' => '\CommonObject',     // Deprecated

We could check what the result is if this is commented in a separate PR (I guess there will be some notifications that pop up).

[mdeweerd]

I've now extended to fixing all the notices in htdocs/core/actions_massactions.inc.php.

Some observations:

• The file arguments to pdftk use '/' which is not system dependent. Maybe this works for pdftk on Windows but technically it's better that this is the path according to the system rules;
• I'ld rather have a dol_exec rather than the native exec to allow the addition of security checks. dol_exec would take an string[][] as the argument with each item being a command to execute in the format array{command,arg1,arg2,...}.
  dol_exec would take care of escaping those and executing them in order (either by separating them with ';' or executing one after the other.
  Also - not verified in detail - according to a summary for the following information, escapeshellarg was designed for bash/... not a windows shell, so that's a potential security issue: https://gist.github.com/Zenexer/40d02da5e07f151adeaeeaa11af9ab36 . And therefore a good reason to have a dol_exec taking care of escaping (or refusing execution) .

Regarding the issues:

• fk_soc and socid on common object not defined: not fixed because when that happens the type of CommonObject does not seem to be limited a a specific set of child classes;
• $pagecount possibly undefined - not fixed because the use seems invalid in case there is more than one document - it uses the pagecount of the last document;
• Issue regarding ModeleNumRefTask: not for me to decide;
• Other cases: I am looking into them. @eldy These are now fixed - your turn to "play".

[mdeweerd]

I've reduced this PR to only the changes in actions_massactions.inc.php . The rest of the PR was moved to #30747

[eldy]

Seems duplicate of line 41 to 49

[mdeweerd]

Possibly because this PR and changes still pending here are "old" and repeated in more recent PRs. I'll clean that up.

The main reason this PR is still open is for these phan notices:

Warning: actions_massactions.inc.php: PhanUndeclaredProperty: Reference to undeclared property \CommonObject->fk_soc
Warning: actions_massactions.inc.php: PhanUndeclaredProperty: Reference to undeclared property \User->email_aliases
Warning: actions_massactions.inc.php: PhanPossiblyUndeclaredGlobalVariable: Global variable $pagecount is possibly undeclared

I am tending to think that I should simply extract the code changes to a separate PR and close this one - the real technical debt is likely not going to be fixed until almost all of the pending phpdoc changes and false positives are identified.