-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
WIP [Meta] Add PHPStan to build process #25536
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
Changes from all commits
e1be251
bfb3048
9183062
d9e30ae
83c1208
bdeda31
0a33eb1
a7dff25
03daa41
da9d0e4
75403df
c5467e0
dbdf9de
5fc8d8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
QA_DOCKER_IMAGE=dkarlovi/phpqa-toolbox:latest | ||
QA_DOCKER_COMMAND=docker run -it --rm -v "$(shell pwd):/project" -w /project ${QA_DOCKER_IMAGE} | ||
|
||
phpstan: | ||
sh -c "${QA_DOCKER_COMMAND} phpstan analyse --configuration phpstan.neon --level 0 src/" | ||
|
||
## | ||
# Special operations | ||
## | ||
|
||
.PHONY: phpstan |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
parameters: | ||
autoload_files: | ||
- vendor/autoload.php | ||
- .phpunit/phpunit-6.0/vendor/autoload.php | ||
- vendor/bin/.phpunit/phpunit-5.7/vendor/autoload.php | ||
|
||
# Twig functions are called by the codebase, but are not autoloaded by default | ||
- vendor/twig/twig/src/Extension/CoreExtension.php | ||
|
||
# files containing multiple classes are not autoloaded properly | ||
- src/Symfony/Component/Form/Tests/Extension/Csrf/Type/FormTypeCsrfExtensionTest.php | ||
- src/Symfony/Component/Form/Tests/Guess/GuessTest.php | ||
- src/Symfony/Component/Form/Tests/SimpleFormTest.php | ||
- src/Symfony/Component/Process/Tests/NonStopableProcess.php | ||
ignoreErrors: | ||
## | ||
## False positive | ||
## | ||
# incorrect, no params is valid | ||
- '#Function get_defined_functions invoked with 0 parameters, 1 required.#' | ||
|
||
## | ||
## By design | ||
## | ||
- '#__construct\(\) does not call parent constructor from .+#' | ||
- '#Function xdebug_[^\s]* not found.#' | ||
# traits do not own their own properties: https://github.com/symfony/symfony/pull/25536/files#r157608526 | ||
- '#Access to an undefined property Symfony\\Component\\DependencyInjection\\Loader\\Configurator\\InlineServiceConfigurator::\$id.#' | ||
- '#Access to an undefined property Symfony\\Component\\DependencyInjection\\Loader\\Configurator\\InlineServiceConfigurator::\$allowParent.#' | ||
# not errors, actually expected to fail | ||
- '#Class Symfony\\Bundle\\FrameworkBundle\\Tests\\DependencyInjection\\Compiler\\NotFound not found.#' | ||
- '#Class Symfony\\Component\\DependencyInjection\\Tests\\Compiler\\NotExist not found.#' | ||
excludes_analyse: | ||
## | ||
## False positive | ||
## | ||
# PHPStan doesn't seem to understand namespaced functions properly | ||
- src/Symfony/Component/Debug/Tests/HeaderMock.php | ||
# currently crashing PHPStan 0.9.1 | ||
- src/Symfony/Bundle/FrameworkBundle/Tests/Controller/AbstractControllerTest.php | ||
- src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerTest.php | ||
|
||
## | ||
## By design | ||
## | ||
# broken require-dev: https://github.com/symfony/symfony/pull/25536/files#r157591181 | ||
- src/Symfony/Bridge/Monolog/Handler/SwiftMailerHandler.php | ||
|
||
## | ||
## Temporary | ||
## | ||
# loading a non-existent class, these should probably be fixtures | ||
- src/symfony/Component/DependencyInjection/Tests/Compiler/OptionalServiceClass.php | ||
- src/Symfony/Component/VarDumper/Tests/Caster/ReflectionCasterTest.php | ||
# it's full of actual errors (which are triggers for the handler), move to fixtures? | ||
- src/Symfony/Component/Debug/Tests/ErrorHandlerTest.php | ||
# typo, lowercase "fixtures" | ||
- src/Symfony/Component/Translation/Tests/fixtures/ | ||
# it's currently incompatible with the interface | ||
- src/Symfony/Bridge/ProxyManager | ||
|
||
## | ||
## Uninteresting for analysis (mostly test fixtures or other test resources) | ||
## | ||
- */src/Symfony/Bridge/*/Tests/Fixtures/* | ||
- */src/Symfony/Bridge/*/Tests/*/Fixtures/* | ||
- src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/fake_vendor/ | ||
- */src/Symfony/Bundle/*/Tests/Fixtures/* | ||
- */src/Symfony/Bundle/*/Tests/*/Fixtures/* | ||
- src/Symfony/Bundle/FrameworkBundle/Resources/views | ||
- src/Symfony/Bundle/FrameworkBundle/Tests/Functional/app/Resources/views | ||
- src/Symfony/Bundle/FrameworkBundle/Tests/Templating/Helper/Resources | ||
- */src/Symfony/Component/*/Tests/Fixtures/* | ||
- */src/Symfony/Component/*/Tests/*/Fixtures/* | ||
- */src/Symfony/Component/*/Tests/Resources/* |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,6 @@ | |
use Symfony\Component\Debug\Exception\FatalErrorException; | ||
use Symfony\Component\Debug\DebugClassLoader; | ||
use Composer\Autoload\ClassLoader as ComposerClassLoader; | ||
use Symfony\Component\ClassLoader\ClassLoader as SymfonyClassLoader; | ||
|
||
/** | ||
* ErrorHandler for classes that do not exist. | ||
|
@@ -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')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or be just reverted as the previous code was just fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code refers to a non-existent class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already discussed, see #25536 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. based on what? how do you define "valid"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. 👍 |
||
foreach ($function[0]->getPrefixes() as $prefix => $paths) { | ||
foreach ($paths as $path) { | ||
$classes = array_merge($classes, $this->findClassInPath($path, $class, $prefix)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please submit these as separate PRs, on the lowest maintained branch where they apply?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?