Feature/phpstan

[darylldoyle]

Description of the Change

This adds PHPStan static analysis to the scaffold. Included is:

• A new composer command compser run static which will analyse the entire codebase
• Static analysis added to lint-staged so that the feedback loop is fast.
• WordPress and 10up specific stubs

There are a few places of note:

• /phpstan/config/default.neon - This file handles the basic setup and configuration of PHPStan, this should only contain updates that need to be made on a scaffold level across all projects.
• /phpstan.neon - This file contains the project-specific setup, including which paths should be analysed. It also allows us to override any of the above settings, such as the level of analysis. The default is 6.
• /phpstan/constants.php - This contains all the default constants that are set throughout our custom plugins themes. It's used by PHPStan so that issues aren't flagged about undefined constants

Alternate Designs

None

Possible Drawbacks

It can slightly slow down each commit as it needs to run an additional script.

Verification Process

• Tested locally

Checklist:

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my change.
• All new and existing tests passed.

Changelog Entry

• Added PHPStan static analysis

[jeffpaul]

Assuming a project forks this into a GitHub repo, could adding a GitHub Action workflow file to run this as a check on PRs be worthwhile?

[darylldoyle]

@jeffpaul that'd definitely be worthwhile! Do you know of anything I can use as a template? I'm not that familiar with Github actions

[jeffpaul]

Noting that https://github.com/marketplace?type=actions&query=phpstan+ lists potential GitHub Actions to consider here with https://github.com/dingo-d/phpstan-wp-action at least being named as one to consider.

[claytoncollie-dmv]

I suggest we change this to max and then add a project-specific level in the phpstan.neon file. Like my comment below, if the company default config might possibly be extracted out into a Composer package, this will surely be set project to project.

[darylldoyle]

I'm concerned with pushing it too high by default. That requires engineers to be more concerned with union types and not using mixed as a return type.

Whilst I think that's good, I wonder if it will put too many engineers off. We need to balance it between being useful and being too restrictive.

I'd much rather we set a sensible default and then if a certain project wants it more restricting they can up it, rather than people turning it off.

[claytoncollie]

@darylldoyle I pushed some commits here to fix various errors at level 6. The last two errors for this level are Function TenUpToolkit\set_dist_url_path not found. inside the functions.php and plugins.php files. I searched high and low for where this package is being loaded but came up empty. It looks like it is related to the Hot Module Reload work done fairly recently. I wonder if @nicholasio can shed some light on how/where that package is being loaded and used on the following lines.

[darylldoyle]

@claytoncollie, thanks for handling that!

That function does indeed exist within the Hot Refresh module. You can find them here.

I wonder if we can get around those errors by wrapping the call in a function_exists() check. I think that will calm PHPStan down

[darylldoyle]

@claytoncollie, thanks for handling that!

That function does indeed exist within the Hot Refresh module. You can find them here.

I wonder if we can get around those errors by wrapping the call in a function_exists() check. I think that will calm PHPStan down

I've wrapped these function in a function_exists() check so they no longer error

[claytoncollie-dmv]

@darylldoyle Love these additions. Simplifying the phpstan.neon a little bit and ignoring those errors is perfect. I'm going to copy this over into CDCR and report back. I'll also look at the blog post next week.

Question. Do you want to try and bump the severity level up to 7 or 8 and if we can squash those errors? I went to 7 and saw a ton of errors related to core WP functionality and/or fundamental ways in which the scaffold is built.

[darylldoyle]

@claytoncollie I've made some updates to the phpstan.neon that you might want to pull over to CDCR. The previous iteration wasn't scanning vendor directories, so will throw errors if you call classes from them.

I've also added some basic docs to the repo, which should help with PHPStan setup.

Finally R.E.

Question. Do you want to try and bump the severity level up to 7 or 8 and if we can squash those errors? I went to 7 and saw a ton of errors related to core WP functionality and/or fundamental ways in which the scaffold is built.

I also had a look at bumping the level to 7. Whilst it'd be great to get there in the long run, I'd like us to get this into the scaffold. The amount of refactoring that would be required to get to Level 7 or 8 will hold us back there I think.

Let's think of that as a long-term goal and move forward at Level 6.

On that note, I'd love a review from @ivanlopez when he has a minute, to see if there's any issues he can see 🙂

[claytoncollie-dmv]

@darylldoyle I agree with sticking to level 6. I was seeing heaps of issues related to how the scaffold is built with the namespace helper function among others. I've only made it to level 3 on CDCR. Trying not to refactor too much before I get the initial setup into the project :)

I will integrate your changes into CDCR today.

[tobeycodes]

Could we add phpstan and add a baseline to ignore all current issues? We can use separate PR to fix any issues. I think we should set the baseline at the highest level possible and leave it up to individual projects to set the level they want. Even if we merge phpstan into the scaffold it is still opt-in. If you don't run the command or have a CI job to block merging with issues it essentially doesn't exist. We could detail in the docs how to remove it or add a command in composer.json to do it (similar to eject in the react ecosystem)