Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PHPStan at level 6 #637

Merged
merged 29 commits into from
Jan 31, 2022
Merged

Add PHPStan at level 6 #637

merged 29 commits into from
Jan 31, 2022

Conversation

imjoehaines
Copy link
Contributor

Goal

PHPStan is a static analyser for PHP that can catch a bunch of potential bugs in code

This PR is based on #633, but runs at level 6 instead of max — PHPStan has 9 error levels (currently) that go from loosest (1) - strictest (9)

For the most part this is only changes to comments, but there are a couple of code changes too:

  • methods that can return null need an explicit return null statement — a bare return or no return at all are both errors
  • passing an int to ini_set and passing null to preg_match are flagged as errors, so these have been fixed

Additionally, some errors have been ignored:

  • some of our Guzzle compatibility code errors because we check for methods that don't exist on new versions. We'll need to run PHPStan against multiple versions of Guzzle to fully check these (this will be done in a future PR)
  • some code exists for PHP 5 compatibility, but PHPStan doesn't run on PHP <7.1 so this errors as well. When we drop PHP 5 support we can also remove this compatibility code, which will solve these errors
  • we use new static(...) in a few places, which is unsafe as there's guarantee that child classes will share the same constructor signature (see this article). We can't fix this without a potential BC break, so have to ignore this for now

Design

This PR stops at level 6 because this is where the levels become tricky to update to. Level 6 specifically adds checking for iterable types, e.g. a bare array should really be array<KeyType, ValueType>. I've turned this option off for now as we get around 100 errors from it, but this will be turned on in a future PR. Until then, I've not advanced any further as additional errors can appear as a consequence of adding stricter iterable types

Testing

PHPStan now runs on CI on both PHP 7.1 & 8.1

xPaw and others added 29 commits January 31, 2022 11:35
These calls are dangerous but we can't change this without a BC break
Reserved memory will never be read from because it's only there to hold
some memory, not to actually be used as a value
Both of these properties rely on a method being called before they will
be set, otherwise they are null
Adding key/value types to everywhere we use 'array' is a huge job, so
I've ignored these errors for now as there are >100 of them

Ideally we would document what we actually mean by 'array' as some
places do have distinct shapes that the array should conform to
@xPaw
Copy link
Contributor

xPaw commented Jan 31, 2022

FYI, once PHP5 support is dropped, types can be added to function arguments

@imjoehaines imjoehaines merged commit f4d8219 into next Jan 31, 2022
@imjoehaines imjoehaines deleted the add-phpstan branch January 31, 2022 16:26
This was referenced Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants