10000 [Meta] Introduce a static code analyzer (like PHPStan) to the build process · Issue #25459 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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

Jump to bottom
Closed
dkarlovi opened this issue Dec 12, 2017 · 13 comments
Closed

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

dkarlovi opened this issue Dec 12, 2017 · 13 comments

Comments

@dkarlovi
Copy link
Contributor
Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version master

Having a tool like PHPStan introduced to the build process would mean we'd be able to have much more instances like #21802 caught all over the codebase. It would require fewer resources doing code reviews as plenty of thing will get pointed out by PHPStan before even getting to the review process.
@Simperfit
Copy link
Contributor

I guess I would be 👍 because we implemented it in Api-Platform and in our clients project and it's quite nice.

But it could be alot of work to get everything to green, we should ensure that this cause no BC too.

@dkarlovi
Copy link
Contributor Author
dkarlovi commented Dec 12, 2017 •
edited
Loading

PHPStan's level system is meant for exactly that purpose: you're able to gradually ramp up your code quality over time, but have new code conform to the level once reached. The first step might be level 0 which is, AFAIK, only a lint.

I'm sure @ondrejmirtes might weight in how to do this integration properly.

@ondrejmirtes
Copy link
Contributor
ondrejmirtes commented Dec 12, 2017 •
edited
Loading

Hi,
author of PHPStan here 😊 I think that introducing PHPStan to the codebase would be very beneficial because it brings PHP much closer to compiled languages, i. e. in testing, you can focus on actual business logic because type errors like calling an undefined method or passing a wrong argument type is already covered.

PHPStan is already used by Doctrine, CakePHP, Prooph and other popular open-source libraries.

CakePHP introduced PHPStan around a year ago, with level 0 and they gradually increased the level now to 2 (which is checked in CI build) and they are now working on fixing issues from level 3.

I should summarise what each rule level is about:

  • Level 0: Checking calls and property accesses on $this and static names, function calls (wrong argument counts) and some misc stuff. This ensures that all type info is correct because I can be sure about $this and static types.
  • Level 1: Checking accessing undefined variables.
  • Level 2: Like level 0, but checking method calls and property accesses on everything. This level begins reading phpDoc @param and @return annotations to enrich info about types.
  • Level 3: More checks about correct arrays usage and assigning correct types to properties.
  • Level 4: Looking for wrong and dead code by checking wrong instanceof, is_int (and other functions), !==, ===, isset that can never be true.
  • Level 5: Checking types of arguments going into method and function calls. Until now, only number of arguments is checked.
  • Level 6: Be more strict about union types.
  • Level 7: Be more strict about nullables, e. g. tell the developer that calling something on Foo|null is potentially dangerous and one should first check that the variable is not null.

Unique feature of PHPStan is that it's possible to define magic behaviour of classes/method/functions (__get, __set, __call, return types that depend on passed argument types) for the purpose of static analysis.

You can read more about PHPStan in the introductory article and also in the most recent article about the latest version.

Let's not rush this; I'm not yet sure whether the better approach would be first to focus on analysing a Symfony-based project (which can bring a lot of benefits to Symfony users), or analyse Symfony framework itself. Because the extensions that one would need to write should be usable in both cases.

What are your opinions?

@dkarlovi
Copy link
Contributor Author

@ondrejmirtes thanks for the exhaustive answer. 👍

My suggestion here is definitely the framework itself, I'm using PHPStan on my Symfony project already and see HUGE benefits from doing it, it's not even a question if you should be doing it (from my POV, anyway).

If you're talking about the container-related extension, agreed that it would be quite useful (it's already tracked at phpstan/phpstan#645).

My reason is, I've gotten quite used to PHPStan pointing out errors I'm making as I'm working and, as I'm currently working on #24781, it feels quite bad to have it be missing. I'm sure Symfony would benefit from it, if only by developers having access to "a code review before code review".

@javiereguiluz
Copy link
Member
javiereguiluz commented Dec 12, 2017 •
edited
Loading

(Note: I use PHPStan and I think it's a very nice tool)

The problem with introducing PHPStan is that the current codebase doesn't comply with its analysis, so it would take us a lot of time to make all branches pass all PHPStan checks ... and if we don't make the branches pass the analysis, we can't activate it for new pull requests because you'll find lots of unrelated issues.

The same happens to us with something much simpler: PHP CS Fixer. Our code doesn't comply with it entirely (because some old code) so we can't recommend contributors to run "php-cs-fixer fix" in their PRs because they'll find some unrelated problems (we have fabbot checker that does the PHP Cs Fixer only for the changes of the PR, not the entire codebase).

@ondrejmirtes
Copy link
Contributor

I think that once we integrate PHPStan at least with level 0 (which is very basic and the errors pointed out are objectively bugs), it's benefitial that also all the branches and pull requests are also analysed and correct before merging - that means that no new bugs are introduced because there are guards set up by CI and PHPStan.

@dkarlovi
Copy link
Contributor Author

integrate PHPStan at least with level 0

That's exactly my suggestion here, it should be rather easy to start from here and attach PRs to increase the level once possible.

@dkarlovi dkarlovi mentioned this issue Dec 18, 2017
Closed
@keradus
Copy link
Member
keradus commented Dec 18, 2017

note: SCA is already performed via fabbot.io.

@ostrolucky
Copy link
Contributor
ostrolucky commented Dec 19, 2017 •
edited
Loading

You guys said issues found on level 0 are objectively bugs. So far it doesn't seem so. PhpStan does wrong assumptions. I'm not blaming PHPStan though, those assumptions are logical and are made because PHPStan lacks the context.

@dkarlovi
Copy link
Contributor Author

@ostrolucky in what way wrong? There's only one false positive found as of yet, IIRC (I'm on mobile)

@ondrejmirtes
Copy link
Contributor
ondrejmirtes commented Dec 19, 2017 via email

@nicolas-grekas
Copy link
Member

the code in question in traits is weird or wrong in some ways

Can we move on? The code is working as expected and perfectly right for the purpose.
I understand why phpstan raises this notice, and I'm fine with it. But it doesn't write the code itself, we do.

@dkarlovi
Copy link
Contributor Author

Closing as it seems the current code is fine as-is. See #25536 discussion for details.

@wouterj wouterj mentioned this issue Mar 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@javiereguiluz @ondrejmirtes @dkarlovi @nicolas-grekas @ostrolucky @keradus @Simperfit
0