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 PHP static analysis via PHPStan #4342

Closed
westonruter opened this issue Mar 1, 2020 · 11 comments · Fixed by #4373
Closed

Add PHP static analysis via PHPStan #4342

westonruter opened this issue Mar 1, 2020 · 11 comments · Fixed by #4373
Assignees
Labels
Infrastructure Changes impacting testing infrastructure or build tooling
Milestone

Comments

@westonruter
Copy link
Member

Feature description

I've only recently become aware of PHPStan, but it seems like it could help with catching subtle bugs. See GoogleForCreators/web-stories-wp#334 and GoogleChromeLabs/pwa-wp#243.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added the Infrastructure Changes impacting testing infrastructure or build tooling label Mar 1, 2020
@westonruter westonruter added this to the v1.5 milestone Mar 1, 2020
@schlessera
Copy link
Collaborator

Yes, PHPStan is great. It will only make sense with clean and strongly typed code of course, but we're having more and more of this, so we'll probably start reaping benefits from it by now.

I've been using it with a WordPress stubs file for WP plugins: https://github.com/mwpd/basic-scaffold/blob/master/phpstan.neon.dist

@schlessera schlessera self-assigned this Mar 2, 2020
@schlessera
Copy link
Collaborator

This will need a bit of a work-around, as PHP-Stan requires PHP 7.1.

@szepeviktor
Copy link

There is a WordPress Extension for PHPStan.

@swissspidy
Copy link
Collaborator

Thanks for opening this, @westonruter! I was about to do it after my experience with the Web Stories plugin but you beat me to it. 🙂

The extension @szepeviktor maintains is indeed super helpful to get off the ground quickly.

@schlessera In the Web Stories plugin we run PHPStan only on 7.1 in a separate job, installing it via composer require --no-interaction --dev szepeviktor/phpstan-wordpress --ignore-platform-reqs. That means you cannot easily use it locally though, but it works fine for us so far. LMK if you come up with a better solution.

@schlessera
Copy link
Collaborator

I've done a bit of a more manual approach, as @szepeviktor's extension requires PHP 7.1+.

PHPStan is downloaded as a Phar instead, and the needed stubs are provided in the tests/ folder (some copied from the above extension, some written as needed).

This now works so that you can run composer analyse on your local installation as well, without any extra effort. We could also add this to a precommit then.

@swissspidy
Copy link
Collaborator

Pretty cool! I also like how you snuck autoloading through Composer in there 🎉

@schlessera
Copy link
Collaborator

Composer was already a requirement in the plugin because of the CSS Parser library.

@swissspidy
Copy link
Collaborator

But we still had AMP_Autoloader 🙂

@szepeviktor
Copy link

as @szepeviktor's extension requires PHP 7.1+.

It is PHPStan's requirement :)

@schlessera
Copy link
Collaborator

schlessera commented Mar 10, 2020

To run it, yes. But the codebase that you want to check does not need to be PHP 7.1+. The plugin is currently at PHP 5.6+, and it makes perfect sense to check the code with PHPStan. But we can't install it directly via Composer because of its requirement. That's why I'm downloading it as a Phar instead.

@schlessera
Copy link
Collaborator

But we still had AMP_Autoloader

Yes, presumably because it is "extensible" for plugins wanting to extend the AMP support. However, this was broken from 2017 onwards and never fixed.

So all it does is provide a class map we need to maintain manually and that is less optimized than the one that Composer generates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants