8000 WIP [Meta] Add PHPStan to build process by dkarlovi · Pull Request #25536 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

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

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Makefile
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
75 changes: 75 additions & 0 deletions phpstan.neon
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/*
5 changes: 2 additions & 3 deletions src/Symfony/Bridge/Doctrine/ManagerRegistry.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
namespace Symfony\Bridge\Doctrine;

use ProxyManager\Proxy\LazyLoadingInterface;
use Psr\Container\ContainerInterface;
Copy link
Member

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?

6D4E

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

use Symfony\Component\DependencyInjection\Container;
use Doctrine\Common\Persistence\AbstractManagerRegistry;

Expand All @@ -24,7 +23,7 @@
abstract class ManagerRegistry extends AbstractManagerRegistry
{
/**
* @var ContainerInterface
* @var Container
*/
protected $container;

Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ class RegisterMappingsPassTest extends TestCase
*/
public function testNoDriverParmeterException()
{
$container = $this->createBuilder(array(
));
$container = $this->createBuilder();
$this->process($container, array(
'manager.param.one',
'manager.param.two',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,18 @@
use Symfony\Bridge\Doctrine\Form\ChoiceList\EntityLoaderInterface;
use Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader;
use Symfony\Component\Form\ChoiceList\ArrayChoiceList;
use Symfony\Component\Form\ChoiceList\Factory\ChoiceListFactoryInterface;

/**
* @author Bernhard Schussek <bschussek@gmail.com>
*/
class DoctrineChoiceLoaderTest extends TestCase
{
/**
* @var ChoiceListFactoryInterface|\PHPUnit_Framework_MockObject_MockObject
*/
private $factory;

/**
* @var ObjectManager|\PHPUnit_Framework_MockObject_MockObject
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
use Symfony\Component\Form\Forms;
use Symfony\Component\Form\Tests\Extension\Core\Type\BaseTypeTest;
use Symfony\Component\Form\Tests\Extension\Core\Type\FormTypeTest;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Bridge\Doctrine\Tests\Fixtures\SingleAssociationToIntIdEntity;
use Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIntIdNoToStringEntity;

Expand Down Expand Up @@ -1121,8 +1120,7 @@ public function testLoaderCaching()
$repo = $this->em->getRepository(self::SINGLE_IDENT_CLASS);

$entityType = new EntityType(
$this->emRegistry,
PropertyAccess::createPropertyAccessor()
$this->emRegistry
);

$entityTypeGuesser = new DoctrineOrmTypeGuesser($this->emRegistry);
Expand Down Expand Up @@ -1184,8 +1182,7 @@ public function testLoaderCachingWithParameters()
$repo = $this->em->getRepository(self::SINGLE_IDENT_CLASS);

$entityType = new EntityType(
$this->emRegistry,
PropertyAccess::createPropertyAccessor()
$this->emRegistry
);

$entityTypeGuesser = new DoctrineOrmTypeGuesser($this->emRegistry);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace Symfony\Bridge\Doctrine\PropertyInfo\Tests;
namespace Symfony\Bridge\Doctrine\Tests\PropertyInfo;

use Doctrine\DBAL\Types\Type as DBALType;
use Doctrine\ORM\EntityManager;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public function fileExcerpt($file, $line)
{
if (is_readable($file)) {
if (extension_loaded('fileinfo')) {
$finfo = new \Finfo();
$finfo = new \finfo();

// Check if the file is an application/octet-stream (eg. Phar file) because highlight_file cannot parse these files
if ('application/octet-stream' === $finfo->file($file, FILEINFO_MIME_TYPE)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\SecurityBundle\Tests;
namespace Symfony\Bundle\SecurityBundle\Tests\Debug;

use PHPUnit\Framework\TestCase;
use Symfony\Bundle\SecurityBundle\Debug\TraceableFirewallListener;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\TwigBundle\Tests;
namespace Symfony\Bundle\TwigBundle\Tests\Functional;

use Symfony\Component\HttpKernel\Kernel;
use Symfony\Component\Config\Loader\LoaderInterface;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Bundle\FrameworkBundle\FrameworkBundle;
use Symfony\Bundle\TwigBundle\Tests\TestCase;
use Symfony\Bundle\TwigBundle\TwigBundle;

class CacheWarmingTest extends TestCase
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\TwigBundle\Tests;
namespace Symfony\Bundle\TwigBundle\Tests\Functional;

use Symfony\Component\HttpKernel\Kernel;
use Symfony\Component\Config\Loader\LoaderInterface;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Bundle\FrameworkBundle\FrameworkBundle;
use Symfony\Bundle\TwigBundle\Tests\TestCase;
use Symfony\Bundle\TwigBundle\TwigBundle;

class NoTemplatingEntryTest extends TestCase
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,9 @@
namespace Symfony\Component\Config\Tests\Definition\Builder;

use PHPUnit\Framework\TestCase;
use Symfony\Component\Config\Tests\Definition\Builder\NodeBuilder as CustomNodeBuilder;
use Symfony\Component\Config\Tests\Fixtures\Builder\NodeBuilder as CustomNodeBuilder;
use Symfony\Component\Config\Definition\Builder\TreeBuilder;

require __DIR__.'/../../Fixtures/Builder/NodeBuilder.php';
require __DIR__.'/../../Fixtures/Builder/BarNodeDefinition.php';
require __DIR__.'/../../Fixtures/Builder/VariableNodeDefinition.php';

class TreeBuilderTest extends TestCase
{
public function testUsingACustomNodeBuilder()
Expand All @@ -28,11 +24,11 @@ public function testUsingACustomNodeBuilder()

$nodeBuilder = $root->children();

$this->assertInstanceOf('Symfony\Component\Config\Tests\Definition\Builder\NodeBuilder', $nodeBuilder);
$this->assertInstanceOf('Symfony\Component\Config\Tests\Fixtures\Builder\NodeBuilder', $nodeBuilder);

$nodeBuilder = $nodeBuilder->arrayNode('deeper')->children();

$this->assertInstanceOf('Symfony\Component\Config\Tests\Definition\Builder\NodeBuilder', $nodeBuilder);
$this->assertInstanceOf('Symfony\Component\Config\Tests\Fixtures\Builder\NodeBuilder', $nodeBuilder);
}

public function testOverrideABuiltInNodeType()
Expand All @@ -42,7 +38,7 @@ public function testOverrideABuiltInNodeType()

$definition = $root->children()->variableNode('variable');

$this->assertInstanceOf('Symfony\Component\Config\Tests\Definition\Builder\VariableNodeDefinition', $definition);
$this->assertInstanceOf('Symfony\Component\Config\Tests\Fixtures\Builder\VariableNodeDefinition', $definition);
}

public function testAddANodeType()
Expand All @@ -52,7 +48,7 @@ public function testAddANodeType()

$definition = $root->children()->barNode('variable');

$this->assertInstanceOf('Symfony\Component\Config\Tests\Definition\Builder\BarNodeDefinition', $definition);
$this->assertInstanceOf('Symfony\Component\Config\Tests\Fixtures\Builder\BarNodeDefinition', $definition);
}

public function testCreateABuiltInNodeTypeWithACustomNodeBuilder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace Symfony\Component\Config\Tests\Definition\Builder;
namespace Symfony\Component\Config\Tests\Fixtures\Builder;

use Symfony\Component\Config\Definition\Builder\NodeDefinition;
use Symfony\Component\Config\Tests\Fixtures\BarNode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace Symfony\Component\Config\Tests\Definition\Builder;
namespace Symfony\Component\Config\Tests\Fixtures\Builder;

use Symfony\Component\Config\Definition\Builder\NodeBuilder as BaseNodeBuilder;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace Symfony\Component\Config\Tests\Definition\Builder;
namespace Symfony\Component\Config\Tests\Fixtures\Builder;

use Symfony\Component\Config\Definition\Builder\VariableNodeDefinition as BaseVariableNodeDefinition;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code refers to a non-existent class?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already discussed, see #25536 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member
@nicolas-grekas nicolas-grekas Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ function () {},

public function testRecursionInArguments()
{
$a = null;
$a = array('foo', array(2, &$a));
$exception = $this->createException($a);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public function testSimpleProcessor()
(new RegisterEnvVarProcessorsPass())->process($container);

$this->assertTrue($container->has('container.env_var_processors_locator'));
$this->assertInstanceof(SimpleProcessor::class, $container->get('container.env_var_processors_locator')->get('foo'));
$this->assertInstanceOf(SimpleProcessor::class, $container->get('container.env_var_processors_locator')->get('foo'));

$expected = array(
'foo' => array('string'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public function testProcess()
$parent = 'instanceof.'.parent::class.'.0.foo';
$def = $container->getDefinition('foo');
$this->assertEmpty($def->getInstanceofConditionals());
$this->assertInstanceof(ChildDefinition::class, $def);
$this->assertInstanceOf(ChildDefinition::class, $def);
$this->assertTrue($def->isAutowired());
$this->assertSame($parent, $def->getParent());
$this->assertSame(array('tag' => array(array()), 'baz' => array(array('attr' => 123))), $def->getTags());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ class ProjectServiceContainer extends Container
public $__foo_bar;
public $__foo_baz;
public $__internal;
protected $privates;
protected $methodMap = array(
'bar' => 'getBarService',
'foo_bar' => 'getFooBarService',
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Filesystem/Tests/FilesystemTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1519,7 +1519,7 @@ public function testAppendToFile()

// skip mode check on Windows
if ('\\' !== DIRECTORY_SEPARATOR) {
$this->assertFilePermissions(664, $filename, 'The written file should keep the same permissions as before.');
$this->assertFilePermissions(664, $filename);
umask($oldMask);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,7 @@ public function ianaCodesReasonPhrasesProvider()

$ianaCodesReasonPhrases = array();

$xpath = new \DomXPath($ianaHttpStatusCodes);
$xpath = new \DOMXPath($ianaHttpStatusCodes);
$xpath->registerNamespace('ns', 'http://www.iana.org/assignments');

$records = $xpath->query('//ns:record');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ public function testOpen()
->with('path', 'name')->willReturn(true);
$proxy = new StrictSessionHandler($handler);

$this->assertInstanceof('SessionUpdateTimestampHandlerInterface', $proxy);
$this->assertInstanceof(AbstractSessionHandler::class, $proxy);
$this->assertInstanceOf('SessionUpdateTimestampHandlerInterface', $proxy);
$this->assertInstanceOf(AbstractSessionHandler::class, $proxy);
$this->assertTrue($proxy->open('path', 'name'));
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php

namespace Symfony\Component\Workflow\Tests;
namespace Symfony\Component\Workflow\Tests\DependencyInjection;

use PHPUnit\Framework\TestCase;
use Symfony\Component\DependencyInjection\ContainerBuilder;
Expand Down
0