Merge pull request #333 from mcg-web/static_analysis

Static analysis

.gitignore

@@ -1,4 +1,6 @@
+.phpcs-cache
 composer.phar
 composer.lock
 phpcs.xml
+phpstan.neon
 vendor/

.travis.yml

@@ -6,10 +6,6 @@ php:
   - 7.2
   - nightly
 
-matrix:
-  allow_failures:
-    - php: nightly
-
 cache:
   directories:
     - $HOME/.composer/cache
@@ -18,10 +14,7 @@ before_install:
   - mv ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/xdebug.ini{,.disabled} || echo "xdebug not available"
   - travis_retry composer self-update
 
-install:
-  - composer require react/promise:2.*
-  - composer require psr/http-message:1.*
-  - travis_retry composer update --prefer-dist
+install: travis_retry composer update --prefer-dist
 
 script: ./vendor/bin/phpunit --group default,ReactPromise
 
@@ -46,8 +39,15 @@ jobs:
         - wget https://scrutinizer-ci.com/ocular.phar
         - php ocular.phar code-coverage:upload --format=php-clover clover.xml
 
-    - stage: Coding standard
+    - stage: Code Quality
       php: 7.1
+      env: CODING_STANDARD
       install: travis_retry composer install --prefer-dist
       script:
         - ./vendor/bin/phpcs
+
+    - stage: Code Quality
+      php: 7.1
+      env: STATIC_ANALYSIS
+      install: travis_retry composer install --prefer-dist
+      script: composer static-analysis

composer.json

@@ -15,8 +15,11 @@
   },
   "require-dev": {
     "doctrine/coding-standard": "^4.0",
+    "phpstan/phpstan": "^0.10.3",
+    "phpstan/phpstan-phpunit": "^0.10.0",
     "phpunit/phpunit": "^7.2",
-    "psr/http-message": "^1.0"
+    "psr/http-message": "^1.0",
+    "react/promise": "2.*"
   },
   "config": {
     "preferred-install": "dist",
@@ -39,6 +42,7 @@
     "psr/http-message": "To use standard GraphQL server"
   },
   "scripts": {
-    "lint" : "phpcs"
+    "lint" : "phpcs",
+    "static-analysis": "phpstan analyse --ansi -l 1 -c phpstan.neon.dist src tests"
   }
 }

phpstan.neon.dist

@@ -0,0 +1,3 @@
+includes:
+	- vendor/phpstan/phpstan-phpunit/extension.neon
+	- vendor/phpstan/phpstan-phpunit/rules.neon

src/Executor/Promise/Promise.php

@@ -6,6 +6,7 @@ namespace GraphQL\Executor\Promise;
 
 use GraphQL\Executor\Promise\Adapter\SyncPromise;
 use GraphQL\Utils\Utils;
+use React\Promise\Promise as ReactPromise;
 
 /**
  * Convenience wrapper for promises represented by Promise Adapter

src/Language/Visitor.php

@@ -264,6 +264,7 @@ class Visitor
 
                 if ($visitFn) {
                     $result    = call_user_func($visitFn, $node, $key, $parent, $path, $ancestors);
+                    $editValue = null;
 
                     if ($result !== null) {
                         if ($result instanceof VisitorOperation) {

src/Server/StandardServer.php

@@ -8,7 +8,7 @@ use GraphQL\Error\FormattedError;
 use GraphQL\Error\InvariantViolation;
 use GraphQL\Executor\ExecutionResult;
 use GraphQL\Executor\Promise\Promise;
-use GraphQL\Utils;
+use GraphQL\Utils\Utils;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Message\StreamInterface;

src/Type/Definition/InputObjectField.php

@@ -21,7 +21,7 @@ class InputObjectField
     /** @var string|null */
     public $description;
 
-    /** @var callback|InputType */
+    /** @var callable|InputType */
     public $type;
 
     /** @var InputValueDefinitionNode|null */

src/Type/Definition/InputObjectType.php

@@ -12,7 +12,6 @@ use function is_array;
 use function is_callable;
 use function is_string;
 use function sprintf;
-use function spritnf;
 
 /**
  * Class InputObjectType
@@ -70,7 +69,7 @@ class InputObjectType extends Type implements InputType, NamedType
 
             if (! is_array($fields)) {
                 throw new InvariantViolation(
-                    spritnf('%s fields must be an array or a callable which returns such an array.', $this->name)
+                    sprintf('%s fields must be an array or a callable which returns such an array.', $this->name)
                 );
             }
 

src/Validator/Rules/KnownDirectives.php

@@ -7,7 +7,9 @@ namespace GraphQL\Validator\Rules;
 use GraphQL\Error\Error;
 use GraphQL\Language\AST\DirectiveNode;
 use GraphQL\Language\AST\InputObjectTypeDefinitionNode;
+use GraphQL\Language\AST\Node;
 use GraphQL\Language\AST\NodeKind;
+use GraphQL\Language\AST\NodeList;
 use GraphQL\Language\DirectiveLocation;
 use GraphQL\Validator\ValidationContext;
 use function count;
@@ -59,7 +61,9 @@ class KnownDirectives extends ValidationRule
     }
 
     /**
-     * @param (Node|NodeList)[] $ancestors
+     * @param Node[]|NodeList[] $ancestors The type is actually (Node|NodeList)[] but this PSR-5 syntax is so far not supported by most of the tools
+     *
+     * @return string
      */
     private function getDirectiveLocationForASTPath(array $ancestors)
     {

src/Validator/Rules/VariablesDefaultValueAllowed.php

@@ -44,10 +44,10 @@ class VariablesDefaultValueAllowed extends ValidationRule
 
                 return Visitor::skipNode();
             },
-            NodeKind::SELECTION_SET       => function (SelectionSetNode $node) use ($context) {
+            NodeKind::SELECTION_SET       => function (SelectionSetNode $node) {
                 return Visitor::skipNode();
             },
-            NodeKind::FRAGMENT_DEFINITION => function (FragmentDefinitionNode $node) use ($context) {
+            NodeKind::FRAGMENT_DEFINITION => function (FragmentDefinitionNode $node) {
                 return Visitor::skipNode();
             },
         ];

tests/Executor/ExecutorTest.php

@@ -329,7 +329,7 @@ class ExecutorTest extends TestCase
                 'fields' => [
                     'a' => [
                         'type'    => Type::string(),
-                        'resolve' => function ($context) use ($doc, &$gotHere) {
+                        'resolve' => function ($context) use (&$gotHere) {
                             $this->assertEquals('thing', $context['contextThing']);
                             $gotHere = true;
                         },

tests/Executor/Promise/ReactPromiseAdapterTest.php

@@ -31,26 +31,24 @@ class ReactPromiseAdapterTest extends TestCase
     {
         $reactAdapter = new ReactPromiseAdapter();
 
-        $this->assertSame(
-            true,
+        $this->assertTrue(
             $reactAdapter->isThenable(new ReactPromise(function () {
             }))
         );
-        $this->assertSame(true, $reactAdapter->isThenable(new FulfilledPromise()));
-        $this->assertSame(true, $reactAdapter->isThenable(new RejectedPromise()));
-        $this->assertSame(
-            true,
+        $this->assertTrue($reactAdapter->isThenable(new FulfilledPromise()));
+        $this->assertTrue($reactAdapter->isThenable(new RejectedPromise()));
+        $this->assertTrue(
             $reactAdapter->isThenable(new LazyPromise(function () {
             }))
         );
-        $this->assertSame(false, $reactAdapter->isThenable(false));
-        $this->assertSame(false, $reactAdapter->isThenable(true));
-        $this->assertSame(false, $reactAdapter->isThenable(1));
-        $this->assertSame(false, $reactAdapter->isThenable(0));
-        $this->assertSame(false, $reactAdapter->isThenable('test'));
-        $this->assertSame(false, $reactAdapter->isThenable(''));
-        $this->assertSame(false, $reactAdapter->isThenable([]));
-        $this->assertSame(false, $reactAdapter->isThenable(new \stdClass()));
+        $this->assertFalse($reactAdapter->isThenable(false));
+        $this->assertFalse($reactAdapter->isThenable(true));
+        $this->assertFalse($reactAdapter->isThenable(1));
+        $this->assertFalse($reactAdapter->isThenable(0));
+        $this->assertFalse($reactAdapter->isThenable('test'));
+        $this->assertFalse($reactAdapter->isThenable(''));
+        $this->assertFalse($reactAdapter->isThenable([]));
+        $this->assertFalse($reactAdapter->isThenable(new \stdClass()));
     }
 
     public function testConvertsReactPromisesToGraphQlOnes() : void

tests/Language/ParserTest.php

@@ -331,7 +331,7 @@ GRAPHQL
 ');
         $result = Parser::parse($source);
 
-        $loc = function ($start, $end) use ($source) {
+        $loc = function ($start, $end) {
             return [
                 'start' => $start,
                 'end'   => $end,
@@ -442,7 +442,7 @@ GRAPHQL
 ');
         $result = Parser::parse($source);
 
-        $loc = function ($start, $end) use ($source) {
+        $loc = function ($start, $end) {
             return [
                 'start' => $start,
                 'end'   => $end,

tests/Language/SchemaPrinterTest.php

@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 
-namespace GraphQL\Tests;
+namespace GraphQL\Tests\Language;
 
 use GraphQL\Language\AST\NameNode;
 use GraphQL\Language\AST\ScalarTypeDefinitionNode;

tests/Language/SerializationTest.php

@@ -51,7 +51,7 @@ class SerializationTest extends TestCase
 
         $expectedVars = get_object_vars($expected);
         $actualVars   = get_object_vars($actual);
-        $this->assertSame(count($expectedVars), count($actualVars), $err);
+        $this->assertCount(count($expectedVars), $actualVars, $err);
         $this->assertEquals(array_keys($expectedVars), array_keys($actualVars), $err);
 
         foreach ($expectedVars as $name => $expectedValue) {

tests/Language/TokenTest.php

@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 
-namespace GraphQL\Tests;
+namespace GraphQL\Tests\Language;
 
 use GraphQL\Language\Token;
 use PHPUnit\Framework\TestCase;

tests/Language/VisitorTest.php

@@ -1222,7 +1222,7 @@ class VisitorTest extends ValidatorTestCase
             $ast,
             Visitor::visitInParallel([
                 [
-                    'enter' => function ($node) use (&$visited, $ast) {
+                    'enter' => function ($node) use ($ast) {
                         $this->checkVisitorFnArgs($ast, func_get_args());
                         if ($node->kind === 'Field' && isset($node->name->value) && $node->name->value === 'b') {
                             return Visitor::removeNode();
@@ -1292,7 +1292,7 @@ class VisitorTest extends ValidatorTestCase
             $ast,
             Visitor::visitInParallel([
                 [
-                    'leave' => function ($node) use (&$visited, $ast) {
+                    'leave' => function ($node) use ($ast) {
                         $this->checkVisitorFnArgs($ast, func_get_args(), true);
                         if ($node->kind === 'Field' && isset($node->name->value) && $node->name->value === 'b') {
                             return Visitor::removeNode();

tests/Server/QueryExecutionTest.php

@@ -66,7 +66,7 @@ class QueryExecutionTest extends ServerTestCase
         $query = '{f1';
 
         $result = $this->executeQuery($query);
-        $this->assertSame(null, $result->data);
+        $this->assertNull($result->data);
         $this->assertCount(1, $result->errors);
         $this->assertContains(
             'Syntax Error: Expected Name, found <EOF>',
@@ -202,7 +202,7 @@ class QueryExecutionTest extends ServerTestCase
         $called1 = false;
         $called2 = false;
 
-        $this->config->setValidationRules(function (OperationParams $params) use ($q1, $q2, &$called1, &$called2) {
+        $this->config->setValidationRules(function (OperationParams $params) use ($q1, &$called1, &$called2) {
             if ($params->query === $q1) {
                 $called1 = true;
 
@@ -376,7 +376,7 @@ class QueryExecutionTest extends ServerTestCase
             'Persistent query loader must return query string or instance of GraphQL\Language\AST\DocumentNode ' .
             'but got: {"err":"err"}'
         );
-        $this->config->setPersistentQueryLoader(function ($queryId, OperationParams $params) use (&$called) {
+        $this->config->setPersistentQueryLoader(function () {
             return ['err' => 'err'];
         });
         $this->executePersistedQuery('some-id');

tests/Server/ServerConfigTest.php

@@ -17,17 +17,17 @@ class ServerConfigTest extends TestCase
     public function testDefaults() : void
     {
         $config = ServerConfig::create();
-        $this->assertEquals(null, $config->getSchema());
-        $this->assertEquals(null, $config->getContext());
-        $this->assertEquals(null, $config->getRootValue());
-        $this->assertEquals(null, $config->getErrorFormatter());
-        $this->assertEquals(null, $config->getErrorsHandler());
-        $this->assertEquals(null, $config->getPromiseAdapter());
-        $this->assertEquals(null, $config->getValidationRules());
-        $this->assertEquals(null, $config->getFieldResolver());
-        $this->assertEquals(null, $config->getPersistentQueryLoader());
-        $this->assertEquals(false, $config->getDebug());
-        $this->assertEquals(false, $config->getQueryBatching());
+        $this->assertNull($config->getSchema());
+        $this->assertNull($config->getContext());
+        $this->assertNull($config->getRootValue());
+        $this->assertNull($config->getErrorFormatter());
+        $this->assertNull($config->getErrorsHandler());
+        $this->assertNull($config->getPromiseAdapter());
+        $this->assertNull($config->getValidationRules());
+        $this->assertNull($config->getFieldResolver());
+        $this->assertNull($config->getPersistentQueryLoader());
+        $this->assertFalse($config->getDebug());
+        $this->assertFalse($config->getQueryBatching());
     }
 
     public function testAllowsSettingSchema() : void
@@ -166,10 +166,10 @@ class ServerConfigTest extends TestCase
         $config = ServerConfig::create();
 
         $config->setDebug(true);
-        $this->assertSame(true, $config->getDebug());
+        $this->assertTrue($config->getDebug());
 
         $config->setDebug(false);
-        $this->assertSame(false, $config->getDebug());
+        $this->assertFalse($config->getDebug());
     }
 
     public function testAcceptsArray() : void
@@ -204,8 +204,8 @@ class ServerConfigTest extends TestCase
         $this->assertSame($arr['validationRules'], $config->getValidationRules());
         $this->assertSame($arr['fieldResolver'], $config->getFieldResolver());
         $this->assertSame($arr['persistentQueryLoader'], $config->getPersistentQueryLoader());
-        $this->assertSame(true, $config->getDebug());
-        $this->assertSame(true, $config->getQueryBatching());
+        $this->assertTrue($config->getDebug());
+        $this->assertTrue($config->getQueryBatching());
     }
 
     public function testThrowsOnInvalidArrayKey() : void

tests/Type/EnumTypeTest.php

@@ -132,7 +132,7 @@ class EnumTypeTest extends TestCase
                             'type' => Type::boolean(),
                         ],
                     ],
-                    'resolve' => function ($value, $args) use ($Complex1, $Complex2) {
+                    'resolve' => function ($value, $args) use ($Complex2) {
                         if (! empty($args['provideGoodValue'])) {
                             // Note: this is one of the references of the internal values which
                             // ComplexEnum allows.

tests/Type/ResolutionTest.php

@@ -287,7 +287,7 @@ class ResolutionTest extends TestCase
         ];
         $this->assertEquals($expectedDescriptor, $eagerTypeResolution->getDescriptor());
 
-        $this->assertSame(null, $eagerTypeResolution->resolveType('User'));
+        $this->assertNull($eagerTypeResolution->resolveType('User'));
         $this->assertSame([], $eagerTypeResolution->resolvePossibleTypes($this->node));
         $this->assertSame([], $eagerTypeResolution->resolvePossibleTypes($this->content));
         $this->assertSame([], $eagerTypeResolution->resolvePossibleTypes($this->mention));

tests/Type/ScalarSerializationTest.php

@@ -186,13 +186,13 @@ class ScalarSerializationTest extends TestCase
     {
         $boolType = Type::boolean();
 
-        $this->assertSame(true, $boolType->serialize('string'));
-        $this->assertSame(false, $boolType->serialize(''));
-        $this->assertSame(true, $boolType->serialize('1'));
-        $this->assertSame(true, $boolType->serialize(1));
-        $this->assertSame(false, $boolType->serialize(0));
-        $this->assertSame(true, $boolType->serialize(true));
-        $this->assertSame(false, $boolType->serialize(false));
+        $this->assertTrue($boolType->serialize('string'));
+        $this->assertFalse($boolType->serialize(''));
+        $this->assertTrue($boolType->serialize('1'));
+        $this->assertTrue($boolType->serialize(1));
+        $this->assertFalse($boolType->serialize(0));
+        $this->assertTrue($boolType->serialize(true));
+        $this->assertFalse($boolType->serialize(false));
         // TODO: how should it behave on '0'?
     }
 

tests/Type/ValidationTest.php

@@ -1974,7 +1974,7 @@ class ValidationTest extends TestCase
     public function testRejectsDifferentInstancesOfTheSameType() : void
     {
         // Invalid: always creates new instance vs returning one from registry
-        $typeLoader = function ($name) use (&$typeLoader) {
+        $typeLoader = function ($name) {
             switch ($name) {
                 case 'Query':
                     return new ObjectType([

tests/Utils/AstFromValueTest.php

@@ -126,7 +126,7 @@ class AstFromValueTest extends TestCase
      */
     public function testDoesNotConvertsNonNullValuestoNullValue() : void
     {
-        $this->assertSame(null, AST::astFromValue(null, Type::nonNull(Type::boolean())));
+        $this->assertNull(AST::astFromValue(null, Type::nonNull(Type::boolean())));
     }
 
     /**
@@ -141,10 +141,10 @@ class AstFromValueTest extends TestCase
         );
 
         // Note: case sensitive
-        $this->assertEquals(null, AST::astFromValue('hello', $this->myEnum()));
+        $this->assertNull(AST::astFromValue('hello', $this->myEnum()));
 
         // Note: Not a valid enum value
-        $this->assertEquals(null, AST::astFromValue('VALUE', $this->myEnum()));
+        $this->assertNull(AST::astFromValue('VALUE', $this->myEnum()));
     }
 
     /**