Introduction of phpstan

[Chrico]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes/features)
• Docs have been added/updated (for bug fixes/features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

This PR will replace "phpcs" with "phpstan" (level 8!) and adds additional extensions for phpstan/phpstan-deprecation-rules´ and swissspidy/phpstan-no-private`.

[codecov]

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.54%. Comparing base (c79bb36) to head (5ceef1a).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Properties/LibraryProperties.php	88.88%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #57      +/-   ##
============================================
- Coverage     98.84%   98.54%   -0.30%     
  Complexity      251      251              
============================================
  Files            10       10              
  Lines           607      620      +13     
============================================
+ Hits            600      611      +11     
- Misses            7        9       +2     

Flag	Coverage Δ
unittests	98.54% <94.11%> (-0.30%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tfrommen]

Thanks for this.

There are a few things that are incorrect or incomplete. And maybe we can add a test or two...?

[tfrommen]

This should do now.

[gmazzap]

I'm not a fan of this thing. This is designed to make PHPStan pass more easily, not to make the code safer. Take this example:

function filterTitle(string $title): string
{
    /**
     * @param string $title Page title.
     */
    return apply_filters('list_pages', $title);
} 

Using that plugin, this passes as-is. Nice! Nice?

What if you have code that does:

add_filter('list_pages', '__return_zero');

Now, the filterTitle() function will return an integer, with string declared as the return type, resulting in a fatal error that the static analysis did not catch.

One might say that the culprit is the add_filter that returned an integer where a string was expected. And that is true but was static analysis able to tell whoever wrote that add_filter they were doing something wrong? No, not at all.

In short, the behavior above is clearly a logic mistake that a static analysis could catch, but using that plugin, it is hiding from us.

Without using that plugin, PHPStan would report an error on filterTitle, telling that it returns mixed where a string is expected. That might be annoying but it is the absolute truth. apply_filters makes our function return mixed, and if we use a plugin to "mock" it returns a string, then we are not working to make our code better, we are just making the tool happy hiding a whole set of errors.

The goal is not to have the CI green; the goal is to catch possible errors. Otherwise, for our unit tests, we could use a tool like https://github.com/hugues-m/phpunit-vw to ensure we never have failing tests, right?

If you don't have that plugin, you have two possibilities. Either you do something like this:

function filterTitle(string $title): string
{
    $filtered = apply_filters('list_pages', $title);

    return is_string($filtered) ? $filtered : $title;
} 

Or you do:

function filterTitle(string $title): string
{
    $filtered = apply_filters('list_pages', $title);
    assert(is_string($filtered));

    return $filtered;
} 

In the first case, you are aiming at stability and defensiveness. Which is great for public APIs.

In the second case, you are making it explicit that you're trusting the filter consumers. This has the same net effect of using the PHPStan plugin (if someone returns an integer from the filter, it breaks) but:

1. it is explicit
2. forces the developer to think about it and decide to opt for this or for the defensive path

Having a tool that hides all of that from you, might look nice at first because the checks pass more easily, but that is not the goal, right?

[gmazzap]

This is fine for me.

I left a comment regarding the usage of the worpdress-phpstan plugin which, in general, I think is not a great idea.

However that plugin does more than one thing, and if we remove it then we have to have other things in place (e.g. our stubs).

I expect the impact on this specific codebase is minimal, so I don't think is much important here, but I think that is worth a discussion about it when we are bringing PHPStan as our tool of choice for all our codebase and not just here.