WIP [Meta] Add PHPStan to build process #25536

dkarlovi · 2017-12-18T11:26:23Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#25459
License	MIT

Idea is to provide a baseline from which PHPStan could be further integrated into the build process. Milestone is to get PHPStan's level 0 being functional.

nicolas-grekas · 2017-12-18T11:28:53Z

src/Symfony/Bridge/Doctrine/ManagerRegistry.php

@@ -12,7 +12,6 @@
 namespace Symfony\Bridge\Doctrine;

 use ProxyManager\Proxy\LazyLoadingInterface;
-use Psr\Container\ContainerInterface;


can you please submit these as separate PRs, on the lowest maintained branch where they apply?

Do you think it would make sense to introduce this in master only? The way I see it, getting the entire framework to run will be an undertaking and, getting it to run in all branches might be quite the PITA.

I can't tell if we're going to merge this yet (adding phpstan)
But this change (and many others in this PR) are not related to adding phpstan, but are like code cleanups detected by phpstan, isn't it?
So these should be in their own PR to me.

That's exactly my point, don't know if this is even possible to get running yet. Let's see if this can be done, if so, I'll separate bugfixes into separate PRs once we see how it'll even look and what gets caught.

Sounds good?

stof · 2017-12-18T14:23:13Z

src/Symfony/Component/Debug/Tests/Exception/FlattenExceptionTest.php

@@ -258,6 +258,9 @@ function () {},

    public function testRecursionInArguments()
    {
+        if (!isset($a)) {


useless. It will never be set there

It's used for the variable to not be undeclared in the following line.

The correct fix to help PHPStan understand the code is

$a = null; $a = array('foo', array(2, &$a));

dkarlovi · 2017-12-18T16:10:36Z

Currently, PHPStan 0.9.1 at level 0 still finds 387 errors (519 at level 1, 4035 as max level), the number halved after I introduced the custom Docker image (with almost all the PHP exts used).

Stuff which is an issue so far:

classes with invalid namespaces (broken autoloading)
files with multiple classes (broken autoloading)
missing functions / classes from extensions (fixed with custom Docker image)
missing functions / classes from dependencies (updated require-dev)

IMO this should be doable soonish. @nicolas-grekas does this look like something worthwhile?

nicolas-grekas · 2017-12-18T18:59:13Z

composer.json

-        "files": [ "src/Symfony/Component/VarDumper/Resources/functions/dump.php" ]
+        "files": [
+            "src/Symfony/Component/VarDumper/Resources/functions/dump.php",
+            "vendor/twig/twig/src/Extension/CoreExtension.php"


this makes no sense, it should be removed
you should generally consider that the code base is pretty clean and most phpstan reports should be considered first as false positives. This one is for sure.

Codebase is using functions defined in this file. Of course, this extension is loaded by the Twig environment, but it works only as a side-effect. This makes it explicit.

I can move it to PHPStan's own autoloader config, as Ondrej mentioned.

please do, this is on purpose

nicolas-grekas · 2017-12-18T19:00:35Z

src/Symfony/Component/Debug/FatalErrorHandler/UndefinedFunctionFatalErrorHandler.php

@@ -54,7 +54,7 @@ public function handleError(array $error, FatalErrorException $exception)
        }

        $candidates = array();
-        foreach (get_defined_functions() as $type => $definedFunctionNames) {
+        foreach (get_defined_functions(false) as $type => $definedFunctionNames) {


the function takes no arguments, why this here?

This might be a false positive, it takes an optional param since 7.0.15: http://php.net/manual/en/function.get-defined-functions.php#refsect1-function.get-defined-functions-parameters

ok, should be removed imho as it's the default value, and may break BC

Sure, I'll report it to PHPStan, don't know why it was asked for here.

nicolas-grekas · 2017-12-18T19:01:41Z

src/Symfony/Component/DependencyInjection/Loader/Configurator/PrototypeConfigurator.php

@@ -40,7 +40,6 @@ class PrototypeConfigurator extends AbstractServiceConfigurator
    private $loader;
    private $resource;
    private $exclude;
-    private $allowParent;


this should be kept (same below), this is a false positive: the property is used by a trait

But that makes the trait dependent on the class using it? Which is in turn dependent on the trait it's using?

nicolas-grekas · 2017-12-18T19:02:04Z

src/Symfony/Component/DependencyInjection/Loader/Configurator/Traits/ParentTrait.php

@@ -16,6 +16,8 @@

 trait ParentTrait
 {
+    protected $allowParent;


visibility change, should be reverted

Same as above, the properties used by the trait were moved to the trait.

two traits defining a same property cannot be used by the same class
so nope, it shouldn't

Again, SHOULD traits share a property?

Having a property inside a class is similar as being in concrete instead of abstract class, your abstraction depends on the implementation and vice-versa at the same time.

I mean, I can easily ignore these traits, but this seems broken. PHPStorm and PHPStan seem to agree.

The code is perfectly fine as is, and intended as is. So the answer is YES :)

nicolas-grekas · 2017-12-18T19:02:54Z

src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php

@@ -412,6 +412,7 @@ class ProjectServiceContainer extends Container
    public $__foo_bar;
    public $__foo_baz;
    public $__internal;
+    private $privates;


it should be "protected", as in the dumped container

nicolas-grekas · 2017-12-18T19:03:50Z

src/Symfony/Component/DependencyInjection/Loader/Configurator/Traits/BindTrait.php

@@ -16,6 +16,8 @@

 trait BindTrait
 {
+    protected $id;


the property is already defined in the classes that use the trait

As above, the trait should define the properties it uses, no?

nicolas-grekas · 2017-12-18T19:04:02Z

src/Symfony/Component/DependencyInjection/Loader/Configurator/ServiceConfigurator.php

@@ -44,7 +44,6 @@ class ServiceConfigurator extends AbstractServiceConfigurator

    private $container;
    private $instanceof;
-    private $allowParent;


to be reverted

ondrejmirtes · 2017-12-18T19:04:27Z

You can use phpstan.neon’s autoload_files and autoload_directories if you don’t want to add unnecessary things to composer.json. Also, if PHPStan finds something that is not broken, please report it as an issue to its bug tracker, thanks!

On Mon, 18 Dec 2017 at 20:02, Nicolas Grekas ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Symfony/Component/DependencyInjection/Loader/Configurator/PrototypeConfigurator.php <#25536 (comment)>: > @@ -40,7 +40,6 @@ class PrototypeConfigurator extends AbstractServiceConfigurator private $loader; private $resource; private $exclude; - private $allowParent; this should be kept (same below), this is a false positive: the property is used by a trait — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#25536 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuClUdTWvQbiiZlKPHe1wzip9Jqm3ks5tBra3gaJpZM4RFVB5> .

-- Ondřej Mirtes

nicolas-grekas · 2017-12-18T19:04:38Z

src/Symfony/Component/Debug/FatalErrorHandler/ClassNotFoundFatalErrorHandler.php

@@ -105,7 +104,7 @@ private function getClassCandidates(string $class): array
                }
            }

-            if ($function[0] instanceof ComposerClassLoader || $function[0] instanceof SymfonyClassLoader) {
+            if ($function[0] instanceof ComposerClassLoader) {


this should be kept: the class exists in cross component versions situations

Can you give an example?

any project that uses debug v4 + class-loader v3.4

nicolas-grekas · 2017-12-18T19:08:14Z

composer.json

@@ -96,7 +96,8 @@
        "egulias/email-validator": "~1.2,>=1.2.8|~2.0",
        "symfony/phpunit-bridge": "~3.4|~4.0",
        "symfony/security-acl": "~2.8|~3.0",
-        "phpdocumentor/reflection-docblock": "^3.0|^4.0"
+        "phpdocumentor/reflection-docblock": "^3.0|^4.0",
+        "swiftmailer/swiftmailer": "^6.0@dev"


why adding this one? it's not used in the code base so I wouldn't add it to please phpstan :)

https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Monolog/Handler/SwiftMailerHandler.php is using it.

it's not: there is no test case for it, so the code just doens't run - so there is no need to add this in require-dev (it's not required)
unless you want to add a test case :)

See https://github.com/symfony/symfony/pull/25536/files#r157608526

GromNaN · 2017-12-19T22:10:22Z

src/Symfony/Component/Debug/FatalErrorHandler/ClassNotFoundFatalErrorHandler.php

@@ -105,7 +104,7 @@ private function getClassCandidates(string $class): array
                }
            }

-            if ($function[0] instanceof ComposerClassLoader || $function[0] instanceof SymfonyClassLoader) {
+            if ($function[0] instanceof ComposerClassLoader || is_subclass_of($function[0], '\Symfony\Component\ClassLoader\ClassLoader')) {


Should be is_subclass_of($function[0], ClassLoader::class), no ?

Or be just reverted as the previous code was just fine.

The code refers to a non-existent class?

Already discussed, see #25536 (comment)

OK, let's try this: there exists a (common) situation where this code refers to a non-existent class, that's the reason PHPStan found it and complained about it.

There exists a situation where this class exists. They both exists. The new code is valid in both cases, existing code is not.

based on what? how do you define "valid"?
on my side it's: it works as expected.

dkarlovi · 2017-12-20T08:46:05Z

Closing this as the current code is fine.

ondrejmirtes · 2017-12-20T08:49:04Z

@dkarlovi What? 😊 So you're abandoning this effort because of the resistance from the Symfony maintainers?

nicolas-grekas · 2017-12-20T08:50:18Z

Which resistance? Seriously, this is going emotional. My comments are purely based on technical facts. Is phpstan better at reading code than humans? Not yet. Can we discuss its suggestions? I hope yes.

dkarlovi · 2017-12-20T08:51:49Z

@ondrejmirtes it feels like a bad use of my time to find and fix stuff across the whole framework only to find out "it's fine as it is" where I cannot understand the understanding how bugs can be fine. :)

@nicolas-grekas NHF, see you in other PRs. 👍

ondrejmirtes · 2017-12-20T08:53:50Z

@nicolas-grekas As PHPStan author, I (obviously) believe that having PHPStan in the CI build is beneficial than not having it. I also believe that if it does not understand some code, that code is weird in some way and could use improvements.

So I try to make PHPStan as best as possible at understanding code, but I also acknowledge that if you want to start using it, you should prepare yourself for some changes in your code which will make it better in the end. That's my two cents.

Unless we share this world view, it's not possible to continue this effort 😊

dkarlovi · 2017-12-20T08:54:23Z

And this is only level 0, mind you. How are we ever going to introduce any kind of changes needed for level 1, let alone anything higher? 😆

Also, I'm conflicted here as I tend to have meritocratic view on things and the Symfony team gets huge props for the work they do and, in the end, it's their baby and their call, I respect that immensely.

On the other hand, reasoning behind some of these arguments is peculiar to say the least. Is it just because I don't understand the whole picture and tradeoffs as deeply as @nicolas-grekas?

javiereguiluz · 2017-12-20T09:24:30Z

I haven't followed this discussion ... but I propose you to do the following: instead of fully introducing PHPStan into Symfony builds, you may follow the same workflow of @kalessil with his great PHPInspections EA Extended PhpStorm plugin for static code analysis.

From time to time he creates PRs to fix the issues found with this plugin: https://github.com/symfony/symfony/search?q=php+inspections+EA&type=Commits&utf8=%E2%9C%93 Each PR goes through a round of reviews because, for different reasons, some of the proposed changes are not introduced in Symfony.

You could run PHPStan on your local Symfony copy and send some PRs to fix issues. If you agree, the PRs should be small but not tiny, to make things easier to merge. And please, bear in mind that in a project so complex, big and "old" as Symfony, there may be some valid reasons to reject some proposed changes, even if they are technically correct. Thanks!

ondrejmirtes · 2017-12-20T09:33:12Z

But PHPStan is meant to run in CI, it's a great addition to tests. When fully embracing it, you can move your workflow closer to compiled languages, e. g. you don't have to even write tests for everything because a lot of code is covered by checking strong and static types. That saves a lot of time and a lot of headaches. I personally would love to know in each PR whether its types conform to the way they should work or whether there's some hole.

This is nothing new - a plenty of open-source projects already use PHPStan to their benefit. The biggest of them are probably Doctrine and CakePHP.

dkarlovi · 2017-12-20T09:40:35Z

@javiereguiluz I understand there's plenty of nuance in decisions here. This PR wasn't actually meant to be merged as-is (as discussed), more to see what can be done and for the discussion to be had on the changes needed.

Discussion indicated we're basically not on the same page and I can't figure out the reasoning process behind the decision made, it seems pretty arbitary. ¯\_(ツ)_/¯

This makes me doing this very inefficient as it boils down to doing the work and never being sure if it's OK this time or should I just beat PHPStan into submission to admit Symfony's code is already fine. :)

A better person to do this would probably be a core team member which can make the decision what's by design and what's broken.

@dkarlovi

This PR was merged into the 2.7 branch. Discussion ---------- [2.7] Fix issues found by PHPStan | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This is the subpart of #25536 that applies on 2.7. ping @dkarlovi FYI. Commits ------- afa1f14 [2.7] Fix issues found by PHPStan

exussum12 · 2017-12-20T18:01:39Z

Not sure if it helps at all but for older code bases I use phpstan filtered though https://github.com/exussum12/coverageChecker to ignore old errors, but still get the huge benefits of phpstan going forward.

This has made phpstan easy for us to put in to a CI process and not having to spend time fixing all of the pre-existing issues, they are fixed when someone is next making a change in that area.

It does mean that who ever next modifies the some code which has a pre existing error, the person doing the pull request will need to fix the issue first, so requires a little bit of the boy scout mentality of leaving the code base cleaner than how you found it.

One of the code bases I run this in dropped ~ 25% of issues reported by phpstan in a few months by doing a bit at a time when a change is required.

gedimin45 · 2017-12-20T19:33:58Z

IMHO quirky code should be avoided if there is an alternative that does not provide significant drawbacks.

dkarlovi · 2017-12-21T09:21:45Z

@exussum12 it was not a question of old errors, I was willing to go through the entire framework and get PHPStan integrated with the CI process, even backporting fixes to <v4.

theofidry · 2017-12-21T21:44:06Z

@dkarlovi IMO when you are going through a codebase like Symfony your are bound to find a lot of oddities & false positive.

I think the idea is ok: try it with level 0 and report all the issues. It's not always easy to know what is a legitimate issue and what is a false positive. Also there will be legitimate issues which won't be changed, because BC or would imply to much changes.

I've tried to go through that process once with Scrutinizer, my experience was not pleasant mostly because there is a lot of things which are objectively not that important, just a matter of opinion, and an awful lot of false positive. I think however phpstan is better at this

dkarlovi · 2017-12-22T08:19:01Z

@theofidry

when you are going through a codebase like Symfony your are bound to find a lot of oddities & false positive.

Of course, that's very much understood. If you look at my phpstan.neon file, you'll see I've organized the exclusion into categories: by design (of Symfony), false positive (of PHPStan), etc. This assumes things will be broken in way way or another and tries to work around them, that's the reason I'm assuming PHPStan has support for these features to begin with. :)

But, the idea of introducing SCA tool into the build is to improve the codebase which, obviously, means changing stuff in it (within other constraints such as BC requirements, of course). So far, only changes which were not dismissed were a bit above syntax errors, a linter might have caught those, doesn't really make sense to introduce a nuanced high quality SCA tool like PHPStan and treat it as a linter. :)

Doing

run PHPStan
find and fix errors
push for review
gets rejected
undo 2) and add to ignored errors

doesn't make sense to me, adding all your errors and the files in your codebase to the ignored list does not a static code analyzed codebase make.

flip111 · 2018-01-02T15:16:58Z

Discussion indicated we're basically not on the same page and I can't figure out the reasoning process behind the decision made, it seems pretty arbitary. ¯_(ツ)_/¯

Yeah this is strange .. the entire framework is not meant to run by itself (that's why it's a framework d0h). So let's declare all the dependencies of code that could be called. What are we saving here? A few MB in a dev environment? Prefer consistency like other dependencies.

[Meta] add PHPStan via jakzal/phpqa

e1be251

carsonbot added Status: Needs Review Feature labels Dec 18, 2017

dkarlovi force-pushed the feature/phpstan branch from 634c3b1 to 450fc16 Compare December 18, 2017 11:28

nicolas-grekas reviewed Dec 18, 2017

View reviewed changes

dkarlovi added 2 commits December 18, 2017 13:12

[Meta] fix issues found via PHPStan in Bridge

bfb3048

[Meta] fix issues found via PHPStan in Bundle

9183062

dkarlovi force-pushed the feature/phpstan branch from 450fc16 to 9183062 Compare December 18, 2017 12:16

[Meta] fix issues found via PHPStan in Config component

d9e30ae

dkarlovi force-pushed the feature/phpstan branch from 54cc2b5 to d9e30ae Compare December 18, 2017 13:04

dkarlovi added 3 commits December 18, 2017 14:07

[Meta] fix issues found via PHPStan in Debug component

83c1208

[Meta] fix issues found via PHPStan in DI component

bdeda31

[Meta] fix issues found via PHPStan in Filesystem component

0a33eb1

stof reviewed Dec 18, 2017

View reviewed changes

dkarlovi added 4 commits December 18, 2017 16:54

[Meta] fix issues found via PHPStan in Debug component

a7dff25

[Meta] fix issues found via PHPStan in HttpFoundation component

03daa41

[Meta] fix issues found via PHPStan in Workflow component

da9d0e4

[Meta] PHPStan, switch to a custom Docker image with all required exts

75403df

nicolas-grekas reviewed Dec 18, 2017

View reviewed changes

[Meta] PHPStan, move autoloaded files from Composer to PHPStan

c5467e0

dkarlovi added 2 commits December 19, 2017 15:57

[Meta] PHPStan, undo changes to Debug component

dbdf9de

[Meta] PHPStan, undo changes to DependencyInjection component

5fc8d8e

See https://github.com/symfony/symfony/pull/25536/files#r157608526

GromNaN reviewed Dec 19, 2017

View reviewed changes

dkarlovi closed this Dec 20, 2017

dkarlovi mentioned this pull request Dec 20, 2017

[Meta] Introduce a static code analyzer (like PHPStan) to the build process #25459

Closed

nicolas-grekas mentioned this pull request Dec 20, 2017

[2.7] Fix issues found by PHPStan #25558

Merged

dkarlovi mentioned this pull request Feb 16, 2018

Make symfony/thanks a suggestion rather than a hard requirement hechoendrupal/drupal-console#3789

Merged

dkarlovi mentioned this pull request Sep 10, 2018

[RFC] Require strict typing for new components and/or classes #28423

Closed

WIP [Meta] Add PHPStan to build process #25536

WIP [Meta] Add PHPStan to build process #25536

Conversation

dkarlovi commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkarlovi commented Dec 18, 2017 • edited

nicolas-grekas Dec 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Dec 18, 2017 via email

nicolas-grekas Dec 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkarlovi commented Dec 20, 2017

ondrejmirtes commented Dec 20, 2017

nicolas-grekas commented Dec 20, 2017

dkarlovi commented Dec 20, 2017

ondrejmirtes commented Dec 20, 2017

dkarlovi commented Dec 20, 2017 • edited

javiereguiluz commented Dec 20, 2017

ondrejmirtes commented Dec 20, 2017

dkarlovi commented Dec 20, 2017

exussum12 commented Dec 20, 2017

gedimin45 commented Dec 20, 2017

dkarlovi commented Dec 21, 2017

theofidry commented Dec 21, 2017

dkarlovi commented Dec 22, 2017

flip111 commented Jan 2, 2018 • edited

dkarlovi commented Dec 18, 2017 •

edited

nicolas-grekas Dec 18, 2017 •

edited

nicolas-grekas Dec 18, 2017 •

edited

nicolas-grekas Dec 20, 2017 •

edited

dkarlovi commented Dec 20, 2017 •

edited

flip111 commented Jan 2, 2018 •

edited