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.

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

Support phpstan v0.5 #43

Merged
merged 17 commits into from Jan 7, 2017
Merged

Support phpstan v0.5 #43

merged 17 commits into from Jan 7, 2017

Conversation

zdenekdrahos
Copy link
Member

@zdenekdrahos zdenekdrahos commented Dec 21, 2016

  • basic integration, html report
  • documentation, cleanup, ...

Unofrtunately there is no version :(
PHPStan - PHP Static Analysis Tool
Works without extra config
 phpqa --report --output cli --tools phpstan --analyzedDirs src,tests --ignoredDirs=vendor

Needs bootstrap
phpqa --report --output cli --tools phpstan --analyzedDirs ./ --ignoredDirs=vendor
@zdenekdrahos zdenekdrahos added this to the v1.9 milestone Dec 21, 2016
@zdenekdrahos
Copy link
Member Author

zdenekdrahos commented Dec 21, 2016

@ondrejmirtes:

  1. What you think about phpstan integration? phpqa defines level and location of phpstan.neon in .phpqa.yml + it adds excluded files at runtime.
  2. Can you pass more variables to neon? Something like %currentWorkingDir% could be useful when phpqa is runned globally and phpstan is not installed in required-dev - solved in 017c994
  3. Is there another way than ignoring errors how to supress undefined method and property?
  4. Do you plan adding phpstan version to --version?
 [Exec] Running vendor/bin/phpstan --version
PHPStan - PHP Static Analysis Tool

@ondrejmirtes
Copy link
Contributor

I have quite a few remarks about this :) But it all boils down to the fact that it's probably too soon to integrate PHPStan into a toolsuite like yours.

  • You run composer require phpstan/phpstan which downloads the latest stable version. But you also use configuration files and command arguments that work with current stable version and can/probably will break in the future. Semantic versioning says that each 0.x release can break compatibility which I take advantage of quite often. You should always lock to a specific 0.x version when you also include specific usage of the tool.
  • PHPStan is not ready to be used outside of composer.json of a project. It currently works best as a Composer dev dependency included directly in a project, not as a globally installed package in the system. The main problem is if incompatible versions of PHPStan dependencies are used in the project (most probably Symfony Console or some specific Nette packages), PHPStan will read reflection data of its dependencies and not project dependencies, because they were loaded first. So it will report errors even if the project does not contain any bugs. The solution to this is to distribute PHPStan as a phar with prefixed namespaces. PHPUnit currently has the same problem and inspired me to use this tool to generate the prefixes. It's on the roadmap for the upcoming months.

And now to your questions:

  • Can you pass more variables to neon? Something like %currentWorkingDir% could be useful when phpqa is runned globally and phpstan is not installed in required-dev

I will add %currentWorkingDirectory% to the upcoming release, so you don't have to hack it this way.

  • Is there another way than ignoring errors how to supress undefined method and property?

Not right now, but local ignoring using annotations is on the roadmap.

  • Do you plan adding phpstan version to --version?

I don't like it, it's yet another thing to think about or automate during the release process. I'd suggest to you to use this excellent package by Ocramius so you don't have to rely on tools implementing this command line option.

Thank you very much for integrating PHPStan into phpqa. I am looking forward to work with you on this integration so it ends up being awesome and really usable for your users :)

@zdenekdrahos
Copy link
Member Author

I don't think that your arguments are blockers. Phpstan is optional, not installed by default.
I've planned to add column status to readme (parallel-lint is stable, phpstan is experimental).

You run composer require phpstan/phpstan which downloads the latest stable version

phpstan is suggested tool. User can install whatever version he likes/needs/... I cannot restrict version in suggest section. phpqa is very slim wrapper. It cannot ensure that content of phpstan.neon is compatible with installed version (I don't want to know such implementation details).

PHPStan is not ready to be used outside of composer.json of a project

My preference of global phpqa doesn't mean that all users are like that. In some cases there is huge resistance to anything global.

Especially with currentWorkingDirectory parameter it's not that hard to analyze project with "global" phpqa (that was problem in version 0.1). Just need to specify bootstrap and autoloaded files. I don't see such big difference between my private project configuration and configuration used when phpstan is installed in require-dev.

The phpunit issue seems to like edge-case that happens very rarely. Nothing that couldn't be solved by warning in readme.

phpstan offtopic (not related to the integration)

Not right now, but local ignoring using annotations is on the roadmap.

Annotations aren't solution for mocking tools (Prophecy, mockista). It's impossible to add annotation to 3rd party code. Maybe more advanced universalObjectCratesClasses?

? Bugs found in private project

I didn't want to create multiple issues because I'm not sure if something is bug or the code is just bad.
https://gist.github.com/zdenekdrahos/9856da19f42832bcfa6115004ab778de

Do you plan adding phpstan version to --version?

Thanks. I'll look into it.

I wouldn't underestimate --version. Running command -v or something like that is powerful when something doesn't work (in any language, not just in php). Better DX than remembering how to find version in composer.

But my point is from user perspective, yours from "publisher". I'm ok with your response, no additional discussion is needed :)

@ondrejmirtes
Copy link
Contributor

My point is that PHPStan is going to change rapidly before it's ready for stable 1.0 release (well, I don't even want to use .neon files in the long run and who knows what else will change to be able to analyze code in a great way) so it's up to you if you will be able to catch up with new versions regularly. I hope so :) I'm also delighted that you find PHPStan something worth to integrate!

It's impossible to add annotation to 3rd party code.

You don't have a reason to add annotation to 3rd party code, because PHPStan should not analyze 3rd party code. You should run it only on your src/tests directories and not on vendor. Its purpose is to find bugs in your code, not in code you can't do anything about. If you want to analyze for example Symfony, you should create a branch with phpstan integration in their repository, not analyze it through your project.

PHPStan only reads metadata (reflection, phpDocs) from 3rd party code.

I wouldn't underestimate --version. Running command -v or something like that

It will make sense to have it when PHPStan will support being installed as a global system tool :) I'll figure that out then.

I will post a comment about the code in that linked gist to its comments.

@ondrejmirtes
Copy link
Contributor

ondrejmirtes commented Dec 22, 2016

@zdenekdrahos
Copy link
Member Author

Versions solved in #44 (https://travis-ci.org/EdgedesignCZ/phpqa/jobs/186305450#L480)
I've explained my annotation comment in gist.

I've got all the information I needed so I am locking this discussion.
Everything else is about phpstan issues, not about integrating phpstan into phpqa.

@EdgedesignCZ EdgedesignCZ locked and limited conversation to collaborators Dec 23, 2016
@zdenekdrahos zdenekdrahos removed the wip label Jan 7, 2017
@zdenekdrahos zdenekdrahos changed the title Support phpstan Support phpstan v0.5 Jan 7, 2017
@EdgedesignCZ EdgedesignCZ unlocked this conversation Jan 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants