Raising the analysis level

PHPStan offers levels 0 to 9 with version 1.10, and up to 10 with version 2. On a Dolibarr module, the climb comes down to four moves:

  1. load the core PHPDoc fixes file;
  2. set treatPhpDocTypesAsCertain: false;
  3. go up one level at a time, fixing what comes up;
  4. open the switches that are still closed.

Without the first two moves, level 4 reports about a hundred false positives, and one wrongly concludes that the module cannot be analysed.

treatPhpDocTypesAsCertain: false

parameters:
    treatPhpDocTypesAsCertain: false

This setting tells PHPStan not to take a type coming from a PHPDoc block as a certainty. Types inferred from the code itself are still checked.

It is the right setting for Dolibarr code, because the PHPDoc of the core is not reliable: a property announced as an object stays null until the fetch_*(), a method announced @return string sometimes returns false. The defensive guards a module writes around them (is_object(), empty(), ??) are therefore not redundant. On a real module, this setting alone brought the function.alreadyNarrowedType reports from 64 down to 30, and the remaining 30 were real redundancies.

It does not replace the fixes file: it lowers the trust in PHPDoc, the file fixes the ones that are wrong. Both work together.

What you meet at each level

Levels 3 and 4: most of the real work.

  • wrong @return in the module (@return string on a function that returns bool or null): fix the PHPDoc, not the code, unless the code is the one in the wrong;
  • dead code revealed: unreachable return, error counters never incremented, guards that never fire;
  • property_exists($this, 'ref') or method_exists($this, 'getNomUrl') inherited from the modulebuilder, always true on a class that declares these members.

Level 5: often clean if level 4 is.

Level 6: missing types, and nothing else. It is volume but it is mechanical: one @var per property, one @param per parameter. On a real module: 150 gaps, 97 properties and 47 parameters among them.

For the properties of a class generated by the modulebuilder, the $fields array gives the SQL type of each column:

SQL type in $fields PHPDoc type
integer int\|null
varchar, text string\|null
datetime, timestamp int\|string\|null

Nullable everywhere: the property is null as long as the object is not loaded. Do not redeclare what CommonObject already declares (ref, entity, status, date_creation, fk_user_creat, fk_user_modif, import_key): PHPStan takes the PHPDoc of the parent.

Declare $fields with a precise type, otherwise PHPStan 2 reports a covariance error from Dolibarr 21 on:

/**
 * @var array<string, array<string, mixed>> Array with all fields and their property
 */
public $fields = array(

Levels 7 to 10: nearly free once level 6 is done.

The switches to open

Many Dolibarr module configurations set customRulesetUsed: true followed by a series of switches set to false. These switches win over the level: one can display "level 9" with half the checks turned off.

Cost measured on a real module, each switch opened on its own:

Switch Errors added
checkNullables: true 0
checkExplicitMixedMissingReturn: true 0
checkPhpDocMissingReturn: true 0
reportMaybes: true 0
reportStaticMethodSignatures: true 0
reportMagicMethods: true 0
reportMaybesInMethodSignatures: true 4
reportMagicProperties: true 8
checkUnionTypes: true 1735
checkThisOnly: false 4719

Open everything except the last two. The errors this brings up are signature incompatibilities with the parent class and undocumented magic properties: real defects.

reportMagicProperties is worth it beyond the analysis: on a class that exposes its attributes through __get() and __set(), it forces you to write the @property tags of the class, hence to document its public interface.

The two to leave closed

checkUnionTypes: true reports every value read from a union or a mixed without being narrowed first. On Dolibarr code, these are mostly expects string, mixed given on dol_syslog(), $langs->trans() and the other core functions. Silencing them would mean scattering type casts, which would hide real defects, or typing the core itself.

checkThisOnly: false extends property checks to every object, not only $this. Same problem, on a wider scale.

Both are handled file by file, never in one go. Write their cost as a comment in the configuration, so that nobody opens them without knowing.

Cleaning up the ignores

Once the level is up, set:

parameters:
    reportUnmatchedIgnoredErrors: true

An ignoreErrors pattern that no longer matches anything then becomes an error, instead of hiding a real defect some day. On a real module, this setting revealed 17 patterns and 25 @phpstan-ignore-next-line comments that were no longer needed.

Check on every version before removing a pattern. A pattern can be dead on Dolibarr 22 and still needed on 18:

for v in 18 19 20 21 22 23; do
    DOLIBARR_VERSION=$v vendor/bin/phpstan analyse --no-progress 2>&1 | grep 'was not matched'
done | sort | uniq -c

Only remove the patterns reported as many times as there are versions.

For a pattern needed on some versions only, PHPStan 2 lets you turn off the check for that pattern alone:

parameters:
    ignoreErrors:
        -
            message: '#^Function llxHeader invoked with \d+ parameters?, 0 required\.$#'
            reportUnmatched: false

PHPStan 1.10 does not know reportUnmatched in an ignore entry.

Checking at every step

Run the analysis again on every supported version at each level, not only at the end, then run the tests of the module again: fixing a PHPStan error often means touching the code.

Next: Continuous integration.