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.

Sign up for GitHub

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

Introduction of phpstan #57

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Introduction of phpstan #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ff304a2
switch from psalm to phpstan.
Chrico Jan 14, 2025
da38ad5
switch from syde/phpcs to inpsyde/php-coding-standards to support PHP…
Chrico Jan 14, 2025
d638ed2
remove psalm.xml.
Chrico Jan 14, 2025
faed10a
Package // revert changes of da60d75 which introduced array_values() …
Chrico Jan 17, 2025
7687fce
composer.json // add "swissspidy/phpstan-no-private" and "phpstan/php…
Chrico Jan 17, 2025
cb1c7f9
Update .github/workflows/php-qa.yml
Chrico Jan 24, 2025
1d3608f
LibraryProperties // revert changes done for new syde/phpcs coding st…
Chrico Jan 24, 2025
77567a7
Merge remote-tracking branch 'origin/feature/phpstan' into feature/ph…
Chrico Jan 24, 2025
fe7709e
php-qa.yml // replace paths: '**psalm.xml' with '**phpstan.neon.xml'.
Chrico Jan 24, 2025
0995b5e
composer.json // update version constraint for phpstan and fixed "scr…
Chrico Jan 24, 2025
25d32c2
replaced psalm-assert-if-true, psalm-assert-if-false, psalm-import-ty…
Chrico Jan 24, 2025
f5153c1
tests // add PHPStan to "tests"-folder.
Chrico Jan 24, 2025
3e52f99
PackageTest // remove nullable return and added typecast to ensure a …
Chrico Jan 24, 2025
f005470
phpcs
Chrico Jan 24, 2025
641b840
ServiceExtensions // enable phpcs rules again.
Chrico Jan 28, 2025
9fec908
Update .github/workflows/php-unit-tests.yml
Chrico Jan 28, 2025
6eecb46
Update tests/unit/Container/ReadOnlyContainerTest.php
Chrico Jan 28, 2025
81f58ff
Update tests/unit/PackageTest.php
Chrico Jan 28, 2025
d5064db
address PHPStan issues
tfrommen Jan 28, 2025
13d9411
include PHP 8.4 in CI
tfrommen Jan 28, 2025
aa4c629
only lint on PHP 8.4, for now
tfrommen Jan 28, 2025
5ceef1a
refactor
tfrommen Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
* text eol=lf

tests export-ignore
.psalm export-ignore
.github export-ignore
.gitattributes export-ignore
.gitignore export-ignore
README.md export-ignore
phpcs.xml.dist export-ignore
phpunit.xml.dist export-ignore
psalm.xml export-ignore
*.php diff=php

/.github/ export-ignore
/docs/ export-ignore
/tests/ export-ignore
/.gitattributes export-ignore
/.gitignore export-ignore
/README.md export-ignore
/phpcs.xml.dist export-ignore
/phpstan.neon.dist export-ignore
/phpunit.xml.dist export-ignore
11 changes: 5 additions & 6 deletions .github/workflows/php-qa.yml
View file
Open in desktop
Chrico marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ on:
- '**workflows/php-qa.yml'
- '**.php'
- '**phpcs.xml.dist'
- '**psalm.xml'
- '**phpstan.neon.dist'
- '**composer.json'
pull_request:
paths:
- '**workflows/php-qa.yml'
- '**.php'
- '**phpcs.xml.dist'
- '**psalm.xml'
- '**phpstan.neon.dist'
Chrico marked this conversation as resolved.
Show resolved Hide resolved
- '**composer.json'
workflow_dispatch:
inputs:
Expand All @@ -25,7 +25,7 @@ on:
options:
- 'Run all'
- 'Run PHPCS only'
- 'Run Psalm only'
- 'Run PHPStan only'
- 'Run lint only'

concurrency:
Expand All @@ -38,7 +38,7 @@ jobs:
uses: inpsyde/reusable-workflows/.github/workflows/lint-php.yml@main
strategy:
matrix:
php: [ '7.4', '8.0', '8.1', '8.2', '8.3' ]
php: [ '7.4', '8.0', '8.1', '8.2', '8.3', '8.4' ]
with:
PHP_VERSION: ${{ matrix.php }}
LINT_ARGS: '-e php --colors --show-deprecated ./src'
Expand All @@ -50,11 +50,10 @@ jobs:
PHP_VERSION: '8.3'

static-code-analysis:
if: ${{ (github.event_name != 'workflow_dispatch') || ((github.event.inputs.jobs == 'Run all') || (github.event.inputs.jobs == 'Run Psalm only')) }}
if: ${{ (github.event_name != 'workflow_dispatch') || ((github.event.inputs.jobs == 'Run all') || (github.event.inputs.jobs == 'Run PHPStan only')) }}
uses: inpsyde/reusable-workflows/.github/workflows/static-analysis-php.yml@main
strategy:
matrix:
php: [ '7.4', '8.0', '8.1', '8.2', '8.3' ]
with:
PHP_VERSION: ${{ matrix.php }}
PSALM_ARGS: --output-format=github --no-suggestions --no-cache --no-diff --find-unused-psalm-suppress
2 changes: 1 addition & 1 deletion .github/workflows/php-unit-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ jobs:

- name: Setup dependencies for PSR-11 target version
run: |
composer remove inpsyde/php-coding-standards inpsyde/wp-stubs-versions vimeo/psalm --dev --no-update
composer remove inpsyde/php-coding-standards --dev --no-update
composer require "psr/container:${{ matrix.container-versions }}" --no-update
composer config --no-plugins allow-plugins.roots/wordpress-core-installer false

Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ When installed for development via Composer, the package also requires:

* inpsyde/php-coding-standards
* roots/wordpress
* vimeo/psalm
* phpstan/phpstan
* phpunit/phpunit
* brain/monkey
* mikey179/vfsstream
Expand Down
18 changes: 11 additions & 7 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,16 @@
},
"require-dev": {
"brain/monkey": "^2.6.1",
"inpsyde/php-coding-standards": "^2@dev",
"inpsyde/wp-stubs-versions": "dev-latest",
"inpsyde/php-coding-standards": "^2.0.0",
"roots/wordpress-no-content": "@dev",
"mikey179/vfsstream": "^v1.6.11",
"phpunit/phpunit": "^9.6.19",
"vimeo/psalm": "^5.24.0"
"phpstan/phpstan": "^2.1.1",
"phpstan/phpstan-mockery": "^2.0.0",
"phpstan/phpstan-phpunit": "^2.0.4",
"szepeviktor/phpstan-wordpress": "^2",
Copy link
Contributor

@gmazzap gmazzap Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"swissspidy/phpstan-no-private": "^v1.0.0",
"phpstan/phpstan-deprecation-rules": "^2.0.1"
},
"extra": {
"branch-alias": {
Expand All @@ -53,13 +57,13 @@
"minimum-stability": "dev",
"prefer-stable": true,
"scripts": {
"cs": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs",
"psalm": "@php ./vendor/vimeo/psalm/psalm --no-suggestions --report-show-info=false --find-unused-psalm-suppress --no-diff --no-cache --no-file-cache --output-format=compact",
"phpcs": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs",
"phpstan": "@php ./vendor/bin/phpstan analyse --memory-limit=1G",
"tests": "@php ./vendor/phpunit/phpunit/phpunit --no-coverage",
"tests:coverage": "@php ./vendor/phpunit/phpunit/phpunit",
"qa": [
"@cs",
"@psalm",
"@phpcs",
"@phpstan",
"@tests"
]
},
Expand Down
2 changes: 1 addition & 1 deletion docs/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ When installed for development, via Composer, the package also requires:

* inpsyde/php-coding-standards
* roots/wordpress
* vimeo/psalm
* phpstan/phpstan
* phpunit/phpunit
* brain/monkey
* mikey179/vfsstream
Expand Down
1 change: 1 addition & 0 deletions phpcs.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<ruleset>
<file>./src</file>
<file>./tests</file>
<exclude-pattern>./tests/unit/stubs.php</exclude-pattern>

<arg value="sp"/>
<arg name="colors"/>
Expand Down
16 changes: 16 additions & 0 deletions phpstan.neon.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
includes:
- vendor/phpstan/phpstan-deprecation-rules/rules.neon
- vendor/phpstan/phpstan-mockery/extension.neon
- vendor/phpstan/phpstan-phpunit/extension.neon
- vendor/swissspidy/phpstan-no-private/rules.neon
- vendor/szepeviktor/phpstan-wordpress/extension.neon
parameters:
level: 8
paths:
- src/
- tests/
treatPhpDocTypesAsCertain: false
ignoreErrors:
-
message: '#Call to static method PHPUnit\\Framework\\Assert::assertInstanceOf\(\) .* will always evaluate to true.#'
path: tests/*
32 changes: 0 additions & 32 deletions psalm.xml
View file
Open in desktop

This file was deleted.

6 changes: 3 additions & 3 deletions src/Container/ContainerConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
use Psr\Container\ContainerInterface;

/**
* @psalm-import-type Service from \Inpsyde\Modularity\Module\ServiceModule
* @psalm-import-type ExtendingService from \Inpsyde\Modularity\Module\ExtendingModule
* @phpstan-import-type Service from \Inpsyde\Modularity\Module\ServiceModule
* @phpstan-import-type ExtendingService from \Inpsyde\Modularity\Module\ExtendingModule
*/
class ContainerConfigurator
{
Expand Down Expand Up @@ -113,7 +113,7 @@ public function hasExtension(string $id): bool
/**
* @return ContainerInterface
*
* @psalm-assert ContainerInterface $this->compiledContainer
* @phpstan-assert ContainerInterface $this->compiledContainer
*/
public function createReadOnlyContainer(): ContainerInterface
{
Expand Down
6 changes: 3 additions & 3 deletions src/Container/PackageProxyContainer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ public function has(string $id): bool
/**
* @return bool
*
* @psalm-assert-if-true ContainerInterface $this->container
* @psalm-assert-if-false null $this->container
* @phpstan-assert-if-true ContainerInterface $this->container
* @phpstan-assert-if-false null $this->container
*/
private function tryContainer(): bool
{
Expand All @@ -67,7 +67,7 @@ private function tryContainer(): bool
* @param string $id
* @return void
*
* @psalm-assert ContainerInterface $this->container
* @phpstan-assert ContainerInterface $this->container
*/
private function assertPackageBooted(string $id): void
{
Expand Down
6 changes: 3 additions & 3 deletions src/Container/ReadOnlyContainer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
use Psr\Container\NotFoundExceptionInterface;

/**
* @psalm-import-type Service from \Inpsyde\Modularity\Module\ServiceModule
* @psalm-import-type ExtendingService from \Inpsyde\Modularity\Module\ExtendingModule
* @phpstan-import-type Service from \Inpsyde\Modularity\Module\ServiceModule
* @phpstan-import-type ExtendingService from \Inpsyde\Modularity\Module\ExtendingModule
*/
class ReadOnlyContainer implements ContainerInterface
{
Expand All @@ -26,7 +26,7 @@ class ReadOnlyContainer implements ContainerInterface
/**
* @param array<string, Service> $services
* @param array<string, bool> $factoryIds
* @param ServiceExtensions|array $extensions
* @param ServiceExtensions|array<string, ExtendingService> $extensions
* @param ContainerInterface[] $containers
*/
public function __construct(
Expand Down
21 changes: 14 additions & 7 deletions src/Container/ServiceExtensions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Psr\Container\ContainerInterface as Container;

/**
* @psalm-import-type ExtendingService from \Inpsyde\Modularity\Module\ExtendingModule
* @phpstan-import-type ExtendingService from \Inpsyde\Modularity\Module\ExtendingModule
*/
class ServiceExtensions
{
Expand All @@ -20,6 +20,7 @@ class ServiceExtensions

/**
* @param string $type
*
* @return string
*/
final public static function typeId(string $type): string
Expand All @@ -30,18 +31,22 @@ final public static function typeId(string $type): string
/**
* @param string $extensionId
* @param ExtendingService $extender
*
* @return static
*/
public function add(string $extensionId, callable $extender): ServiceExtensions
{
isset($this->extensions[$extensionId]) or $this->extensions[$extensionId] = [];
if (!isset($this->extensions[$extensionId])) {
$this->extensions[$extensionId] = [];
}
$this->extensions[$extensionId][] = $extender;

return $this;
}

/**
* @param string $extensionId
*
* @return bool
*/
public function has(string $extensionId): bool
Expand All @@ -53,6 +58,7 @@ public function has(string $extensionId): bool
* @param mixed $service
* @param string $id
* @param Container $container
*
* @return mixed
*/
final public function resolve($service, string $id, Container $container)
Expand All @@ -68,6 +74,7 @@ final public function resolve($service, string $id, Container $container)
* @param string $id
* @param mixed $service
* @param Container $container
*
* @return mixed
*/
protected function resolveById(string $id, $service, Container $container)
Expand All @@ -83,7 +90,8 @@ protected function resolveById(string $id, $service, Container $container)
* @param string $className
* @param object $service
* @param Container $container
* @param array $extendedClasses
* @param string[] $extendedClasses
*
* @return mixed
*
* phpcs:disable Generic.Metrics.CyclomaticComplexity
Expand All @@ -110,8 +118,7 @@ protected function resolveByType(
}

// 2nd group of extensions: targeting parent classes
$parents = class_parents($service, false);
($parents === false) and $parents = [];
$parents = class_parents($service, false) ?: [];
foreach ($parents as $parentName) {
$byParent = $this->extensions[self::typeId($parentName)] ?? null;
if (($byParent !== null) && ($byParent !== [])) {
Expand All @@ -120,8 +127,7 @@ protected function resolveByType(
}

// 3rd group of extensions: targeting implemented interfaces
$interfaces = class_implements($service, false);
($interfaces === false) and $interfaces = [];
$interfaces = class_implements($service, false) ?: [];
foreach ($interfaces as $interfaceName) {
$byInterface = $this->extensions[self::typeId($interfaceName)] ?? null;
if (($byInterface !== null) && ($byInterface !== [])) {
Expand Down Expand Up @@ -163,6 +169,7 @@ protected function resolveByType(
* @param object $service
* @param Container $container
* @param list<ExtendingService> $extenders
*
* @return list{mixed, int}
*/
private function extendByType(
Expand Down
2 changes: 1 addition & 1 deletion src/Module/ExtendingModule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Psr\Container\ContainerInterface;

/**
* @psalm-type ExtendingService = callable(mixed $service, ContainerInterface $container):mixed
* @phpstan-type ExtendingService callable(mixed $service, ContainerInterface $container): mixed
*/
interface ExtendingModule extends Module
{
Expand Down
2 changes: 1 addition & 1 deletion src/Module/FactoryModule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
namespace Inpsyde\Modularity\Module;

/**
* @psalm-import-type Service from ServiceModule
* @phpstan-import-type Service from ServiceModule
*/
interface FactoryModule extends Module
{
Expand Down
2 changes: 1 addition & 1 deletion src/Module/ServiceModule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Psr\Container\ContainerInterface;

/**
* @psalm-type Service = callable(ContainerInterface $container):mixed
* @phpstan-type Service callable(ContainerInterface $container): mixed
*/
interface ServiceModule extends Module
{
Expand Down
Loading