From 21592f8f280e592c23502f72b5c9e306f562b612 Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Wed, 12 Jun 2019 10:22:18 +0200 Subject: [PATCH 1/4] Upgrade PHPStan --- composer.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/composer.json b/composer.json index 113e79d..b688618 100644 --- a/composer.json +++ b/composer.json @@ -16,9 +16,9 @@ "require-dev": { "doctrine/coding-standard": "^6.0", "phpbench/phpbench": "^0.14.0", - "phpstan/phpstan": "^0.11.4", - "phpstan/phpstan-phpunit": "^0.11.0", - "phpstan/phpstan-strict-rules": "^0.11.0", + "phpstan/phpstan": "^0.11.8", + "phpstan/phpstan-phpunit": "^0.11.2", + "phpstan/phpstan-strict-rules": "^0.11.1", "phpunit/phpcov": "^5.0", "phpunit/phpunit": "^7.2", "psr/http-message": "^1.0", From a22a08322096ff020b1e327743a425cc88729c67 Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Wed, 12 Jun 2019 10:33:08 +0200 Subject: [PATCH 2/4] Cleanup Warning --- src/Error/Warning.php | 32 +++++++++++++++---------- src/Exception/InvalidArgument.php | 20 ++++++++++++++++ tests/Exception/InvalidArgumentTest.php | 18 ++++++++++++++ 3 files changed, 57 insertions(+), 13 deletions(-) create mode 100644 src/Exception/InvalidArgument.php create mode 100644 tests/Exception/InvalidArgumentTest.php diff --git a/src/Error/Warning.php b/src/Error/Warning.php index 9157b05..25828b1 100644 --- a/src/Error/Warning.php +++ b/src/Error/Warning.php @@ -4,6 +4,8 @@ declare(strict_types=1); namespace GraphQL\Error; +use GraphQL\Exception\InvalidArgument; +use function is_int; use function trigger_error; use const E_USER_WARNING; @@ -15,12 +17,12 @@ use const E_USER_WARNING; */ final class Warning { - const WARNING_ASSIGN = 2; - const WARNING_CONFIG = 4; - const WARNING_FULL_SCHEMA_SCAN = 8; - const WARNING_CONFIG_DEPRECATION = 16; - const WARNING_NOT_A_TYPE = 32; - const ALL = 63; + public const WARNING_ASSIGN = 2; + public const WARNING_CONFIG = 4; + public const WARNING_FULL_SCHEMA_SCAN = 8; + public const WARNING_CONFIG_DEPRECATION = 16; + public const WARNING_NOT_A_TYPE = 32; + public const ALL = 63; /** @var int */ private static $enableWarnings = self::ALL; @@ -37,7 +39,7 @@ final class Warning * * @api */ - public static function setWarningHandler(?callable $warningHandler = null) + public static function setWarningHandler(?callable $warningHandler = null) : void { self::$warningHandler = $warningHandler; } @@ -54,14 +56,16 @@ final class Warning * * @api */ - public static function suppress($suppress = true) + public static function suppress($suppress = true) : void { if ($suppress === true) { self::$enableWarnings = 0; } elseif ($suppress === false) { self::$enableWarnings = self::ALL; - } else { + } elseif (is_int($suppress)) { self::$enableWarnings &= ~$suppress; + } else { + throw InvalidArgument::fromExpectedTypeAndArgument('bool|int', $suppress); } } @@ -77,18 +81,20 @@ final class Warning * * @api */ - public static function enable($enable = true) + public static function enable($enable = true) : void { if ($enable === true) { self::$enableWarnings = self::ALL; } elseif ($enable === false) { self::$enableWarnings = 0; - } else { + } elseif (is_int($enable)) { self::$enableWarnings |= $enable; + } else { + throw InvalidArgument::fromExpectedTypeAndArgument('bool|int', $enable); } } - public static function warnOnce($errorMessage, $warningId, $messageLevel = null) + public static function warnOnce(string $errorMessage, int $warningId, ?int $messageLevel = null) : void { if (self::$warningHandler) { $fn = self::$warningHandler; @@ -99,7 +105,7 @@ final class Warning } } - public static function warn($errorMessage, $warningId, $messageLevel = null) + public static function warn(string $errorMessage, int $warningId, ?int $messageLevel = null) : void { if (self::$warningHandler) { $fn = self::$warningHandler; diff --git a/src/Exception/InvalidArgument.php b/src/Exception/InvalidArgument.php new file mode 100644 index 0000000..eea34d5 --- /dev/null +++ b/src/Exception/InvalidArgument.php @@ -0,0 +1,20 @@ +getMessage()); + } +} From e87460880c49393fa8bb823409a0e65233f6d873 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20David?= Date: Wed, 12 Jun 2019 11:59:42 +0200 Subject: [PATCH 3/4] QueryPlan can now be used on interfaces not only objects. It's often the case to use interfaces in queries: interface Pet { name: String! } Query { pets: [Pet] } --- src/Type/Definition/QueryPlan.php | 4 +- tests/Type/QueryPlanTest.php | 113 ++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/src/Type/Definition/QueryPlan.php b/src/Type/Definition/QueryPlan.php index 235066d..641686d 100644 --- a/src/Type/Definition/QueryPlan.php +++ b/src/Type/Definition/QueryPlan.php @@ -140,9 +140,11 @@ class QueryPlan /** * @return mixed[] * + * $parentType InterfaceType|ObjectType. + * * @throws Error */ - private function analyzeSelectionSet(SelectionSetNode $selectionSet, ObjectType $parentType) : array + private function analyzeSelectionSet(SelectionSetNode $selectionSet, Type $parentType) : array { $fields = []; foreach ($selectionSet->selections as $selectionNode) { diff --git a/tests/Type/QueryPlanTest.php b/tests/Type/QueryPlanTest.php index 50ec6ae..73f8ae7 100644 --- a/tests/Type/QueryPlanTest.php +++ b/tests/Type/QueryPlanTest.php @@ -5,6 +5,8 @@ declare(strict_types=1); namespace GraphQL\Tests\Type; use GraphQL\GraphQL; +use GraphQL\Tests\Executor\TestClasses\Dog; +use GraphQL\Type\Definition\InterfaceType; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\QueryPlan; use GraphQL\Type\Definition\ResolveInfo; @@ -295,6 +297,117 @@ final class QueryPlanTest extends TestCase self::assertFalse($queryPlan->hasType('Test')); } + public function testQueryPlanOnInterface() : void + { + $petType = new InterfaceType([ + 'name' => 'Pet', + 'fields' => static function () { + return [ + 'name' => ['type' => Type::string()], + ]; + }, + ]); + + $dogType = new ObjectType([ + 'name' => 'Dog', + 'interfaces' => [$petType], + 'isTypeOf' => static function ($obj) { + return $obj instanceof Dog; + }, + 'fields' => static function () { + return [ + 'name' => ['type' => Type::string()], + 'woofs' => ['type' => Type::boolean()], + ]; + }, + ]); + + $query = 'query Test { + pets { + name + ... on Dog { + woofs + } + } + }'; + + $expectedQueryPlan = [ + 'woofs' => [ + 'type' => Type::boolean(), + 'fields' => [], + 'args' => [], + ], + 'name' => [ + 'type' => Type::string(), + 'args' => [], + 'fields' => [], + ], + ]; + + $expectedReferencedTypes = [ + 'Dog', + 'Pet', + ]; + + $expectedReferencedFields = [ + 'woofs', + 'name', + ]; + + /** @var QueryPlan $queryPlan */ + $queryPlan = null; + $hasCalled = false; + + $petsQuery = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'pets' => [ + 'type' => Type::listOf($petType), + 'resolve' => static function ( + $value, + $args, + $context, + ResolveInfo $info + ) use ( + &$hasCalled, + &$queryPlan +) { + $hasCalled = true; + $queryPlan = $info->lookAhead(); + + return []; + }, + ], + ], + ]); + + $schema = new Schema([ + 'query' => $petsQuery, + 'types' => [$dogType], + 'typeLoader' => static function ($name) use ($dogType, $petType) { + switch ($name) { + case 'Dog': + return $dogType; + case 'Pet': + return $petType; + } + }, + ]); + $result = GraphQL::executeQuery($schema, $query)->toArray(); + + self::assertTrue($hasCalled); + self::assertEquals($expectedQueryPlan, $queryPlan->queryPlan()); + self::assertEquals($expectedReferencedTypes, $queryPlan->getReferencedTypes()); + self::assertEquals($expectedReferencedFields, $queryPlan->getReferencedFields()); + self::assertEquals(['woofs'], $queryPlan->subFields('Dog')); + + self::assertTrue($queryPlan->hasField('name')); + self::assertFalse($queryPlan->hasField('test')); + + self::assertTrue($queryPlan->hasType('Dog')); + self::assertFalse($queryPlan->hasType('Test')); + } + public function testMergedFragmentsQueryPlan() : void { $image = new ObjectType([ From 6a5325a44847e55b088a857c1ec952e971932aab Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Wed, 12 Jun 2019 15:02:27 +0200 Subject: [PATCH 4/4] Another code cleanup --- src/Error/Warning.php | 4 +- src/Executor/Promise/Adapter/SyncPromise.php | 18 +++---- src/Executor/Values.php | 6 +-- src/Experimental/Executor/Collector.php | 2 +- .../Executor/CoroutineExecutor.php | 6 +-- src/Language/AST/Location.php | 2 +- src/Language/AST/Node.php | 4 +- src/Language/Lexer.php | 10 ++-- src/Language/Parser.php | 4 +- src/Language/Token.php | 53 +++++++++---------- src/Server/OperationParams.php | 3 +- src/Type/Definition/Directive.php | 15 +++--- src/Type/Definition/ObjectType.php | 2 +- src/Type/SchemaConfig.php | 2 +- 14 files changed, 64 insertions(+), 67 deletions(-) diff --git a/src/Error/Warning.php b/src/Error/Warning.php index 25828b1..6728941 100644 --- a/src/Error/Warning.php +++ b/src/Error/Warning.php @@ -96,7 +96,7 @@ final class Warning public static function warnOnce(string $errorMessage, int $warningId, ?int $messageLevel = null) : void { - if (self::$warningHandler) { + if (self::$warningHandler !== null) { $fn = self::$warningHandler; $fn($errorMessage, $warningId); } elseif ((self::$enableWarnings & $warningId) > 0 && ! isset(self::$warned[$warningId])) { @@ -107,7 +107,7 @@ final class Warning public static function warn(string $errorMessage, int $warningId, ?int $messageLevel = null) : void { - if (self::$warningHandler) { + if (self::$warningHandler !== null) { $fn = self::$warningHandler; $fn($errorMessage, $warningId); } elseif ((self::$enableWarnings & $warningId) > 0) { diff --git a/src/Executor/Promise/Adapter/SyncPromise.php b/src/Executor/Promise/Adapter/SyncPromise.php index 26aecd6..8d490ed 100644 --- a/src/Executor/Promise/Adapter/SyncPromise.php +++ b/src/Executor/Promise/Adapter/SyncPromise.php @@ -38,16 +38,16 @@ class SyncPromise */ private $waiting = []; - public static function runQueue() + public static function runQueue() : void { $q = self::$queue; - while ($q && ! $q->isEmpty()) { + while ($q !== null && ! $q->isEmpty()) { $task = $q->dequeue(); $task(); } } - public function resolve($value) + public function resolve($value) : self { switch ($this->state) { case self::PENDING: @@ -83,7 +83,7 @@ class SyncPromise return $this; } - public function reject($reason) + public function reject($reason) : self { if (! $reason instanceof Exception && ! $reason instanceof Throwable) { throw new Exception('SyncPromise::reject() has to be called with an instance of \Throwable'); @@ -107,7 +107,7 @@ class SyncPromise return $this; } - private function enqueueWaitingPromises() + private function enqueueWaitingPromises() : void { Utils::invariant( $this->state !== self::PENDING, @@ -116,7 +116,7 @@ class SyncPromise foreach ($this->waiting as $descriptor) { self::getQueue()->enqueue(function () use ($descriptor) { - /** @var $promise self */ + /** @var self $promise */ [$promise, $onFulfilled, $onRejected] = $descriptor; if ($this->state === self::FULFILLED) { @@ -145,17 +145,17 @@ class SyncPromise $this->waiting = []; } - public static function getQueue() + public static function getQueue() : SplQueue { return self::$queue ?: self::$queue = new SplQueue(); } public function then(?callable $onFulfilled = null, ?callable $onRejected = null) { - if ($this->state === self::REJECTED && ! $onRejected) { + if ($this->state === self::REJECTED && $onRejected === null) { return $this; } - if ($this->state === self::FULFILLED && ! $onFulfilled) { + if ($this->state === self::FULFILLED && $onFulfilled === null) { return $this; } $tmp = new self(); diff --git a/src/Executor/Values.php b/src/Executor/Values.php index 2f74e8c..82e26c8 100644 --- a/src/Executor/Values.php +++ b/src/Executor/Values.php @@ -92,7 +92,7 @@ class Values ), [$varDefNode] ); - } elseif ($varDefNode->defaultValue) { + } elseif ($varDefNode->defaultValue !== null) { $coercedValues[$varName] = AST::valueFromAST($varDefNode->defaultValue, $varType); } } @@ -196,7 +196,7 @@ class Values $argType = $argumentDefinition->getType(); $argumentValueNode = $argumentValueMap[$name] ?? null; - if (! $argumentValueNode) { + if ($argumentValueNode === null) { if ($argumentDefinition->defaultValueExists()) { $coercedValues[$name] = $argumentDefinition->defaultValue; } elseif ($argType instanceof NonNull) { @@ -209,7 +209,7 @@ class Values } elseif ($argumentValueNode instanceof VariableNode) { $variableName = $argumentValueNode->name->value; - if ($variableValues && array_key_exists($variableName, $variableValues)) { + if ($variableValues !== null && array_key_exists($variableName, $variableValues)) { // Note: this does not check that this variable value is correct. // This assumes that this query has been validated and the variable // usage here is of the correct type. diff --git a/src/Experimental/Executor/Collector.php b/src/Experimental/Executor/Collector.php index 8639335..843db9b 100644 --- a/src/Experimental/Executor/Collector.php +++ b/src/Experimental/Executor/Collector.php @@ -199,7 +199,7 @@ class Collector if ($selection instanceof FieldNode) { /** @var FieldNode $selection */ - $resultName = $selection->alias ? $selection->alias->value : $selection->name->value; + $resultName = $selection->alias === null ? $selection->name->value : $selection->alias->value; if (! isset($this->fields[$resultName])) { $this->fields[$resultName] = []; diff --git a/src/Experimental/Executor/CoroutineExecutor.php b/src/Experimental/Executor/CoroutineExecutor.php index 91091b4..d90035b 100644 --- a/src/Experimental/Executor/CoroutineExecutor.php +++ b/src/Experimental/Executor/CoroutineExecutor.php @@ -498,7 +498,7 @@ class CoroutineExecutor implements Runtime, ExecutorImplementation if ($type !== $this->schema->getType($type->name)) { $hint = ''; - if ($this->schema->getConfig()->typeLoader) { + if ($this->schema->getConfig()->typeLoader !== null) { $hint = sprintf( 'Make sure that type loader returns the same instance as defined in %s.%s', $ctx->type, @@ -646,7 +646,7 @@ class CoroutineExecutor implements Runtime, ExecutorImplementation } else { if ($type !== $this->schema->getType($type->name)) { $hint = ''; - if ($this->schema->getConfig()->typeLoader) { + if ($this->schema->getConfig()->typeLoader !== null) { $hint = sprintf( 'Make sure that type loader returns the same instance as defined in %s.%s', $ctx->type, @@ -904,7 +904,7 @@ class CoroutineExecutor implements Runtime, ExecutorImplementation return $this->schema->getType($value['__typename']); } - if ($abstractType instanceof InterfaceType && $this->schema->getConfig()->typeLoader) { + if ($abstractType instanceof InterfaceType && $this->schema->getConfig()->typeLoader !== null) { Warning::warnOnce( sprintf( 'GraphQL Interface Type `%s` returned `null` from its `resolveType` function ' . diff --git a/src/Language/AST/Location.php b/src/Language/AST/Location.php index 72135fc..6f2e6d4 100644 --- a/src/Language/AST/Location.php +++ b/src/Language/AST/Location.php @@ -69,7 +69,7 @@ class Location $this->endToken = $endToken; $this->source = $source; - if (! $startToken || ! $endToken) { + if ($startToken === null || $endToken === null) { return; } diff --git a/src/Language/AST/Node.php b/src/Language/AST/Node.php index 2205d6d..0e192af 100644 --- a/src/Language/AST/Node.php +++ b/src/Language/AST/Node.php @@ -106,7 +106,7 @@ abstract class Node $tmp = (array) $this; - if ($this->loc) { + if ($this->loc !== null) { $tmp['loc'] = [ 'start' => $this->loc->start, 'end' => $this->loc->end, @@ -125,7 +125,7 @@ abstract class Node 'kind' => $node->kind, ]; - if ($node->loc) { + if ($node->loc !== null) { $result['loc'] = [ 'start' => $node->loc->start, 'end' => $node->loc->end, diff --git a/src/Language/Lexer.php b/src/Language/Lexer.php index 3aad4dd..d2c1d5f 100644 --- a/src/Language/Lexer.php +++ b/src/Language/Lexer.php @@ -314,11 +314,11 @@ class Lexer $start = $this->position; [$char, $code] = $this->readChar(); - while ($code && ( + while ($code !== null && ( $code === 95 || // _ - $code >= 48 && $code <= 57 || // 0-9 - $code >= 65 && $code <= 90 || // A-Z - $code >= 97 && $code <= 122 // a-z + ($code >= 48 && $code <= 57) || // 0-9 + ($code >= 65 && $code <= 90) || // A-Z + ($code >= 97 && $code <= 122) // a-z )) { $value .= $char; [$char, $code] = $this->moveStringCursor(1, 1)->readChar(); @@ -695,7 +695,7 @@ class Lexer do { [$char, $code, $bytes] = $this->moveStringCursor(1, $bytes)->readChar(); $value .= $char; - } while ($code && + } while ($code !== null && // SourceCharacter but not LineTerminator ($code > 0x001F || $code === 0x0009) ); diff --git a/src/Language/Parser.php b/src/Language/Parser.php index f5c9280..a0c123f 100644 --- a/src/Language/Parser.php +++ b/src/Language/Parser.php @@ -1655,9 +1655,7 @@ class Parser $name = $this->parseName(); $directives = $this->parseDirectives(true); $types = $this->parseUnionMemberTypes(); - if (count($directives) === 0 && - ! $types - ) { + if (count($directives) === 0 && count($types) === 0) { throw $this->unexpected(); } diff --git a/src/Language/Token.php b/src/Language/Token.php index 831b3ad..eb0bbdf 100644 --- a/src/Language/Token.php +++ b/src/Language/Token.php @@ -11,28 +11,28 @@ namespace GraphQL\Language; class Token { // Each kind of token. - const SOF = ''; - const EOF = ''; - const BANG = '!'; - const DOLLAR = '$'; - const AMP = '&'; - const PAREN_L = '('; - const PAREN_R = ')'; - const SPREAD = '...'; - const COLON = ':'; - const EQUALS = '='; - const AT = '@'; - const BRACKET_L = '['; - const BRACKET_R = ']'; - const BRACE_L = '{'; - const PIPE = '|'; - const BRACE_R = '}'; - const NAME = 'Name'; - const INT = 'Int'; - const FLOAT = 'Float'; - const STRING = 'String'; - const BLOCK_STRING = 'BlockString'; - const COMMENT = 'Comment'; + public const SOF = ''; + public const EOF = ''; + public const BANG = '!'; + public const DOLLAR = '$'; + public const AMP = '&'; + public const PAREN_L = '('; + public const PAREN_R = ')'; + public const SPREAD = '...'; + public const COLON = ':'; + public const EQUALS = '='; + public const AT = '@'; + public const BRACKET_L = '['; + public const BRACKET_R = ']'; + public const BRACE_L = '{'; + public const PIPE = '|'; + public const BRACE_R = '}'; + public const NAME = 'Name'; + public const INT = 'Int'; + public const FLOAT = 'Float'; + public const STRING = 'String'; + public const BLOCK_STRING = 'BlockString'; + public const COMMENT = 'Comment'; /** * The kind of Token (see one of constants above). @@ -104,18 +104,15 @@ class Token $this->value = $value; } - /** - * @return string - */ - public function getDescription() + public function getDescription() : string { - return $this->kind . ($this->value ? ' "' . $this->value . '"' : ''); + return $this->kind . ($this->value === null ? '' : ' "' . $this->value . '"'); } /** * @return (string|int|null)[] */ - public function toArray() + public function toArray() : array { return [ 'kind' => $this->kind, diff --git a/src/Server/OperationParams.php b/src/Server/OperationParams.php index 3ab12de..a976ec0 100644 --- a/src/Server/OperationParams.php +++ b/src/Server/OperationParams.php @@ -9,6 +9,7 @@ use function is_string; use function json_decode; use function json_last_error; use const CASE_LOWER; +use const JSON_ERROR_NONE; /** * Structure representing parsed HTTP parameters for GraphQL operation @@ -93,7 +94,7 @@ class OperationParams } $tmp = json_decode($params[$param], true); - if (json_last_error()) { + if (json_last_error() !== JSON_ERROR_NONE) { continue; } diff --git a/src/Type/Definition/Directive.php b/src/Type/Definition/Directive.php index 0ffe496..3ad4b8a 100644 --- a/src/Type/Definition/Directive.php +++ b/src/Type/Definition/Directive.php @@ -9,6 +9,7 @@ use GraphQL\Language\DirectiveLocation; use GraphQL\Utils\Utils; use function array_key_exists; use function array_keys; +use function count; use function in_array; use function is_array; @@ -16,11 +17,11 @@ class Directive { public const DEFAULT_DEPRECATION_REASON = 'No longer supported'; - const INCLUDE_NAME = 'include'; - const IF_ARGUMENT_NAME = 'if'; - const SKIP_NAME = 'skip'; - const DEPRECATED_NAME = 'deprecated'; - const REASON_ARGUMENT_NAME = 'reason'; + public const INCLUDE_NAME = 'include'; + public const IF_ARGUMENT_NAME = 'if'; + public const SKIP_NAME = 'skip'; + public const DEPRECATED_NAME = 'deprecated'; + public const REASON_ARGUMENT_NAME = 'reason'; /** @var Directive[] */ public static $internalDirectives; @@ -84,9 +85,9 @@ class Directive /** * @return Directive[] */ - public static function getInternalDirectives() + public static function getInternalDirectives() : array { - if (! self::$internalDirectives) { + if (count(self::$internalDirectives) === 0) { self::$internalDirectives = [ 'include' => new self([ 'name' => self::INCLUDE_NAME, diff --git a/src/Type/Definition/ObjectType.php b/src/Type/Definition/ObjectType.php index f509293..532a9b0 100644 --- a/src/Type/Definition/ObjectType.php +++ b/src/Type/Definition/ObjectType.php @@ -168,7 +168,7 @@ class ObjectType extends Type implements OutputType, CompositeType, NullableType private function getInterfaceMap() { - if (! $this->interfaceMap) { + if ($this->interfaceMap === null) { $this->interfaceMap = []; foreach ($this->getInterfaces() as $interface) { $this->interfaceMap[$interface->name] = $interface; diff --git a/src/Type/SchemaConfig.php b/src/Type/SchemaConfig.php index 9f33626..1b21775 100644 --- a/src/Type/SchemaConfig.php +++ b/src/Type/SchemaConfig.php @@ -42,7 +42,7 @@ class SchemaConfig /** @var Directive[] */ public $directives; - /** @var callable */ + /** @var callable|null */ public $typeLoader; /** @var SchemaDefinitionNode */