Contrib.social

[meta] Introduce PHPStan Strict Rules

Open on Drupal.org →
Created on 15 November 2024, 5 months ago

Problem/Motivation

This issue resolves to discuss the introduction of PHPStan Strict Rules into Drupal.

PHPStan Strict are a set of configuration chances and extra rules that encourage defensive programming. Things that could even remotely result in a bug are prevented. Think alongside: not relying on magical features of a programming language when writing code for the space shuttle. Being explicit, especially in regards to typing.

PHPStan strict can be introduced to a project simply by adding the dependency, and relying on phpstan/extension-installer to add it in.

Introducing the project will cause PHPStan to generate a lot of new messages. Rather than add all the new messages to the baseline, you can turn off all rules by default (as mentioned on strict rules Github project page):

parameters:
	strictRules:
		disallowedLooseComparison: false
		booleansInConditions: false
		uselessCast: false
		requireParentConstructorCall: false
		disallowedBacktick: false
		disallowedEmpty: false
		disallowedImplicitArrayCreation: false
		disallowedShortTernary: false
		overwriteVariablesWithLoop: false
		closureUsesThis: false
		matchingInheritedMethodNames: false
		numericOperandsInArithmeticOperators: false
		strictFunctionCalls: false
		dynamicCallOnStaticMethod: false
		switchConditionsMatchingType: false
		noVariableVariables: false
		strictArrayFilter: false
		illegalConstructorMethodCall: false

After adding this rule-disabling configuration, there will still be quite a few error messages. These messages are caused by configuration changes enforced by strict-rules.

These remaining issues are typically quite straightforward to resolve. I've already created a meta issue and a series of child issues over at 📌 Resolve issues exposed by PHPStan Strict Active . As you can see in the associated MR's, the code changes are quite obviously bugs/typos/potential logic issues. I wouldn't say we need to even consider including Strict-Rules in order to resolve those.

Proposed resolution

Introducing Strict-rules does not interfere with the ongoing (fantastic) initiative of resolving traditional PHPStan Levels. Strict rules can run in parallel.

My suggested order of operations is:

  • Resolve the non rule child issues of 📌 Resolve issues exposed by PHPStan Strict Active .
  • Discuss PHPStan strict rules, as a dependency, here. Including adding all rules as disabled, by default.
  • Enable strict rules one by one, with one issue per rule x subsystem combination. This would take months/years.
  • After all strict rules are resolved, we'd remove the strictRules parameter from phpstan.neon.dist.

Remaining tasks

Discuss above, in parallel resolve 📌 Resolve issues exposed by PHPStan Strict Active

🌱 Plan
Status

Active

Version

11.0 🔥

Component

base system

Created by

🇦🇺Australia dpi Perth, Australia

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @dpi
  • Comment 5 months ago →
    🇦🇺Australia dpi Perth, Australia

    The rules as described by the Github project page as of today:

    Require booleans in if, elseif, ternary operator, after !, and on both sides of && and ||.
    Require numeric operands or arrays in + and numeric operands in -/*///**/%.
    Require numeric operand in $var++, $var--, ++$varand --$var.
    These functions contain a $strict parameter for better type safety, it must be set to true:
    in_array (3rd parameter)
    array_search (3rd parameter)
    array_keys (3rd parameter; only if the 2nd parameter $search_value is provided)
    base64_decode (2nd parameter)
    Variables assigned in while loop condition and for loop initial assignment cannot be used after the loop.
    Variables set in foreach that's always looped thanks to non-empty arrays cannot be used after the loop.
    Types in switch condition and case value must match. PHP compares them loosely by default and that can lead to unexpected results.
    Check that statically declared methods are called statically.
    Disallow empty() - it's a very loose comparison (see manual), it's recommended to use more strict one.
    Disallow short ternary operator (?:) - implies weak comparison, it's recommended to use null coalesce operator (??) or ternary operator with strict condition.
    Disallow variable variables ($$foo, $this->$method() etc.)
    Disallow overwriting variables with foreach key and value variables
    Always true instanceof, type-checking is_* functions and strict comparisons ===/!==. These checks can be turned off by setting checkAlwaysTrueInstanceof/checkAlwaysTrueCheckTypeFunctionCall/checkAlwaysTrueStrictComparison to false.
    Correct case for referenced and called function names.
    Correct case for inherited and implemented method names.
    Contravariance for parameter types and covariance for return types in inherited methods (also known as Liskov substitution principle - LSP)
    Check LSP even for static methods
    Require calling parent constructor
    Disallow usage of backtick operator ($ls = `ls -la`)
    Closure should use $this directly instead of using $this variable indirectly

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024