-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
WIP [Meta] Add PHPStan to build process #25536
Changes from 11 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 |
---|---|---|
|
@@ -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" | ||
}, | ||
"conflict": { | ||
"phpdocumentor/reflection-docblock": "<3.0||>=3.2.0,<3.2.2", | ||
|
@@ -126,7 +127,10 @@ | |
] | ||
}, | ||
"autoload-dev": { | ||
"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" | ||
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. this makes no sense, it should be removed 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. 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. 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. please do, this is on purpose |
||
] | ||
}, | ||
"minimum-stability": "dev", | ||
"extra": { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
parameters: | ||
autoload_files: | ||
- vendor/autoload.php | ||
- .phpunit/phpunit-6.0/vendor/autoload.php | ||
- vendor/bin/.phpunit/phpunit-5.7/vendor/autoload.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: | ||
- '#__construct\(\) does not call parent constructor from .+#' | ||
- '#Function xdebug_[^\s]* not found.#' | ||
|
||
# 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: | ||
- */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/* | ||
|
||
# Typo: lowercase "fixtures" | ||
- src/Symfony/Component/Translation/Tests/fixtures/ | ||
|
||
# temporary, it's currently incompatible with the interface | ||
- src/Symfony/Bridge/ProxyManager | ||
|
||
# temporary, currently crashing PHPStan 0.9.1 | ||
- src/Symfony/Bundle/FrameworkBundle/Tests/Controller/AbstractControllerTest.php | ||
- src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerTest.php | ||
|
||
# temporary, loading a non-existant class, these should probably be fixtures | ||
- src/symfony/Component/DependencyInjection/Tests/Compiler/OptionalServiceClass.php | ||
- src/Symfony/Component/VarDumper/Tests/Caster/ReflectionCasterTest.php | ||
|
||
# temporary, it's full of actual errors (which are triggers for the handler) | ||
- src/Symfony/Component/Debug/Tests/ErrorHandlerTest.php | ||
|
||
# temporary, PHPStan doesn't seem to understand namespaced functions properly | ||
- src/Symfony/Component/Debug/Tests/HeaderMock.php |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,6 @@ | |
namespace Symfony\Bridge\Doctrine; | ||
|
||
use ProxyManager\Proxy\LazyLoadingInterface; | ||
use Psr\Container\ContainerInterface; | ||
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. 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 commentThe 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 commentThe 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) 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. 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? |
||
use Symfony\Component\DependencyInjection\Container; | ||
use Doctrine\Common\Persistence\AbstractManagerRegistry; | ||
|
||
|
@@ -24,7 +23,7 @@ | |
abstract class ManagerRegistry extends AbstractManagerRegistry | ||
{ | ||
/** | ||
* @var ContainerInterface | ||
* @var Container | ||
*/ | ||
protected $container; | ||
|
||
|
@@ -58,7 +57,7 @@ function (&$wrappedInstance, LazyLoadingInterface $manager) use ($name) { | |
$name = $this->aliases[$name]; | ||
} | ||
if (isset($this->fileMap[$name])) { | ||
$wrappedInstance = $this->load($this->fileMap[$name], false); | ||
$wrappedInstance = $this->load($this->fileMap[$name]); | ||
} else { | ||
$method = $this->methodMap[$name] ?? 'get'.strtr($name, $this->underscoreMap).'Service'; // BC with DI v3.4 | ||
$wrappedInstance = $this->{$method}(false); | ||
|
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) { | ||
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. this should be kept: the class exists in cross component versions situations 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. Can you give an example? 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. any project that uses debug v4 + class-loader v3.4 |
||
foreach ($function[0]->getPrefixes() as $prefix => $paths) { | ||
foreach ($paths as $path) { | ||
$classes = array_merge($classes, $this->findClassInPath($path, $class, $prefix)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
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 function takes no arguments, why this here? 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. 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 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, should be removed imho as it's the default value, and may break BC 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. Sure, I'll report it to PHPStan, don't know why it was asked for here. |
||
foreach ($definedFunctionNames as $definedFunctionName) { | ||
if (false !== $namespaceSeparatorIndex = strrpos($definedFunctionName, '\\')) { | ||
$definedFunctionNameBasename = substr($definedFunctionName, $namespaceSeparatorIndex + 1); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,7 +40,6 @@ class PrototypeConfigurator extends AbstractServiceConfigurator | |
private $loader; | ||
private $resource; | ||
private $exclude; | ||
private $allowParent; | ||
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. this should be kept (same below), this is a false positive: the property is used by a trait 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. But that makes the trait dependent on the class using it? Which is in turn dependent on the trait it's using? |
||
|
||
public function __construct(ServicesConfigurator $parent, PhpFileLoader $loader, Definition $defaults, string $namespace, string $resource, bool $allowParent) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,6 @@ class ServiceConfigurator extends AbstractServiceConfigurator | |
|
||
private $container; | ||
private $instanceof; | ||
private $allowParent; | ||
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. to be reverted |
||
|
||
public function __construct(ContainerBuilder $container, array $instanceof, bool $allowParent, ServicesConfigurator $parent, Definition $definition, $id, array $defaultTags) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ | |
|
||
trait BindTrait | ||
{ | ||
protected $id; | ||
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 property is already defined in the classes that use the trait 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. As above, the trait should define the properties it uses, no? |
||
|
||
/** | ||
* Sets bindings. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ | |
|
||
trait ParentTrait | ||
{ | ||
protected $allowParent; | ||
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. visibility change, should be reverted 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. Same as above, the properties used by the trait were moved to the trait. 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. two traits defining a same property cannot be used by the same 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. 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. 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 is perfectly fine as is, and intended as is. So the answer is YES :) |
||
|
||
/** | ||
* Sets the Definition to inherit from. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -412,6 +412,7 @@ class ProjectServiceContainer extends Container | |
public $__foo_bar; | ||
public $__foo_baz; | ||
public $__internal; | ||
private $privates; | ||
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. it should be "protected", as in the dumped container |
||
protected $methodMap = array( | ||
'bar' => 'getBarService', | ||
'foo_bar' => 'getFooBarService', | ||
|
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.
why adding this one? it's not used in the code base so I wouldn't add it to please phpstan :)
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.
https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Monolog/Handler/SwiftMailerHandler.php is using it.
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.
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 :)