Add PHPStan to QA checks

[jrfnl]

PHPStan is a good addition to our QA toolkit and with improvements PHPStan has made over the years is now a viable tool for us to use (previously it would give way too many false positives).

This commit:

• Adds a separate job to the basics workflow in GH Actions.
  Notes:
  • I've not added PHPStan to the Composer dependencies for two reasons:
    1. It doesn't allow for installation on PHP < 7.2, which would break/block the composer install for our test runs.
    2. It would add dependencies which could conflict/cause problems for our test runs due to those defining token constants too.
  • Phive could potentially be used to still have a setup which can be used locally, but just running locally from a PHPStan PHAR file should work just fine.
  • For CI, PHPStan will be installed as a PHAR file by setup-php now.
    This does carry a risk if PHPStan would make breaking changes or if a new release adds rules for the levels being scanned as, in that case, builds could unexpectedly start failing.
    Fixing the version for the setup-php action installs to the current release 1.12.3 could prevent that, but that adds an additional maintenance burden of having to keep updating the version as PHPStan releases pretty often.
    So, for now, I've elected to run the risk of random failures. If and when those start happening, this should be re-evaluated.
• Adds a configuration file for PHPStan.
  Notes:
  • PHPStan needs to know about the dependencies (PHPCS et al), so I'm (re-)using the bootstrap file for the tests to load the PHPCS autoloader and register the standard with the PHPCS autoloader as adding an autoload directive to the composer.json file would cause problems with the PHPCS autoloader.
  • PHPStan flags type checks on properties with a documented type, while without strict_types we cannot always be sure the properties set will be of the correct type. For that reason, I've set treatPhpDocTypesAsCertain to false (which silences those notices).
• Adds the configuration file to .gitattributes and the typical overload file for the configuration file to .gitignore.

Refs:

• https://phpstan.org/
• https://phpstan.org/config-reference

[fredden]

without strict_types we cannot always be sure the properties set will be of the correct type.

Strict types protects against the wrong type being used for outbound calls; parameter types are for asserting the type (with an error for callers who use strict types, or an implicit type-cast otherwise) of inbound parameters. This is a common misconception. It's parameter types that are at play here, not strict type mode.

[jrfnl]

without strict_types we cannot always be sure the properties set will be of the correct type.

Strict types protects against the wrong type being used for outbound calls; parameter types are for asserting the type (with an error for callers who use strict types, or an implicit type-cast otherwise) of inbound parameters. This is a common misconception. It's parameter types that are at play here, not strict type mode.

In this case, I was talking about the types for public properties, not parameters/return types. As this codebase has a PHP minimum < 7.4, those cannot have type declarations.

As for strict_types, in my opinion, it generally does more harm than good for parameters and properties as the type juggling means code will act on values which should have been rejected, not juggled. (Yes, I do actually know how it works)

[fredden]

In this case, I was talking about the types for public properties, not parameters/return types.

For reference, I agree that setting treatPhpDocTypesAsCertain: false is the right thing to do for this project. When I removed that locally I only saw two complaints from PHPStan - both of which were for parameter types.

As for strict_types, in my opinion, it generally does more harm than good for parameters and properties as the type juggling means code will act on values which should have been rejected, not juggled. (Yes, I do actually know how it works)

That's exactly what declare(strict_types=1); does - reject instead of juggle.

I think there's a problem with the terminology being used here. I read strict_types as specifically the "Strict typing" language feature (ie, declare(strict_types=1); at the top of a file). From context, @jrfnl is talking about parameter / property types. These are not the same thing and should not be regarded as such.

I know that we have had a video conversation about this before, but it seems that the language is still being confused. I'm trying to clarify / correct what looks like misinformation to me.

I like strict_types and think we should adopt it in this organisation. Changing how parameter / property / return types are defined (ie, a comment versus a PHP type declaration) isn't suitable for this project (and others within this organisation) for several reasons.

[jrfnl]

As for strict_types, in my opinion, it generally does more harm than good for parameters and properties as the type juggling means code will act on values which should have been rejected, not juggled. (Yes, I do actually know how it works)

That's exactly what declare(strict_types=1); does - reject instead of juggle.

Except it only does so for calls to the functions from files also using strict_types and will still juggle if the "calling" file does not use strict_types, which is why it is a form of false security and next to useless for parameter and (public/protected) property types.
The only benefit is for return types and for calls to PHP native functions. For everything else is creates more problems than it is worth as we cannot force the ecosystem around PHPCS to adopt strict_types (i.e. all the external standards and such).

I like strict_types and think we should adopt it in this organisation. Changing how parameter / property / return types are defined (ie, a comment versus a PHP type declaration) isn't suitable for this project (and others within this organisation) for several reasons.

Sorry to disappoint you, but I think that will be a long wait as I do not support that direction.