From fe5c3bdee5185422d5df89c55e0c7bc76cf1ca91 Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Fri, 15 Jun 2018 23:49:40 +0200 Subject: [PATCH 1/3] Introduce PHPCS --- .gitignore | 3 +- .travis.yml | 20 +++++++++-- composer.json | 3 +- phpcs.xml.dist | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 phpcs.xml.dist diff --git a/.gitignore b/.gitignore index 4e3728b..6586246 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,4 @@ -.idea/ composer.phar composer.lock +phpcs.xml vendor/ -bin/ diff --git a/.travis.yml b/.travis.yml index 648e187..45e4636 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,8 +2,6 @@ dist: trusty language: php php: - - 5.6 - - 7.0 - 7.1 - 7.2 - nightly @@ -47,3 +45,21 @@ jobs: after_script: - wget https://scrutinizer-ci.com/ocular.phar - php ocular.phar code-coverage:upload --format=php-clover clover.xml + - stage: Pull request coding standard + if: type = pull_request + install: travis_retry composer install --prefer-dist + script: + - | + if [ $TRAVIS_BRANCH != "master" ]; then + git remote set-branches --add origin $TRAVIS_BRANCH; + git fetch origin $TRAVIS_BRANCH; + fi + - git merge-base origin/$TRAVIS_BRANCH $TRAVIS_PULL_REQUEST_SHA || git fetch origin +refs/pull/$TRAVIS_PULL_REQUEST/merge --unshallow + - wget https://github.com/diff-sniffer/git/releases/download/0.1.0/git-phpcs.phar + - php git-phpcs.phar origin/$TRAVIS_BRANCH...$TRAVIS_PULL_REQUEST_SHA + - stage: Coding standard + if: NOT type = pull_request + php: 7.1 + install: travis_retry composer install --prefer-dist + script: + - ./vendor/bin/phpcs diff --git a/composer.json b/composer.json index 27bfc82..d4de73a 100644 --- a/composer.json +++ b/composer.json @@ -9,10 +9,11 @@ "API" ], "require": { - "php": ">=5.6", + "php": "^7.1", "ext-mbstring": "*" }, "require-dev": { + "doctrine/coding-standard": "^4.0", "phpunit/phpunit": "^4.8", "psr/http-message": "^1.0" }, diff --git a/phpcs.xml.dist b/phpcs.xml.dist new file mode 100644 index 0000000..4be54dd --- /dev/null +++ b/phpcs.xml.dist @@ -0,0 +1,93 @@ + + + + + + + + + + + + src + tests + + + + + + + + + + + + + + + + + + + + + From 4bc3b7885c2231aff71ccc10d4ed01edcd212012 Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Tue, 19 Jun 2018 14:53:56 +0200 Subject: [PATCH 2/3] Fix CS in Error folder --- src/Error/ClientAware.php | 3 + src/Error/Debug.php | 7 +- src/Error/Error.php | 245 ++++++++++++++++--------------- src/Error/FormattedError.php | 227 ++++++++++++++++------------ src/Error/InvariantViolation.php | 5 +- src/Error/SyntaxError.php | 9 +- src/Error/UserError.php | 5 +- src/Error/Warning.php | 48 +++--- 8 files changed, 309 insertions(+), 240 deletions(-) diff --git a/src/Error/ClientAware.php b/src/Error/ClientAware.php index 70c6987..c347004 100644 --- a/src/Error/ClientAware.php +++ b/src/Error/ClientAware.php @@ -1,4 +1,7 @@ nodes = $nodes; + $this->source = $source; + $this->positions = $positions; + $this->path = $path; + $this->extensions = $extensions ?: ( + $previous && $previous instanceof self + ? $previous->extensions + : [] + ); + + if ($previous instanceof ClientAware) { + $this->isClientSafe = $previous->isClientSafe(); + $this->category = $previous->getCategory() ?: static::CATEGORY_INTERNAL; + } elseif ($previous) { + $this->isClientSafe = false; + $this->category = static::CATEGORY_INTERNAL; + } else { + $this->isClientSafe = true; + $this->category = static::CATEGORY_GRAPHQL; + } + } + /** * Given an arbitrary Error, presumably thrown while attempting to execute a * GraphQL operation, produce a new GraphQLError aware of the location in the * document responsible for the original Error. * - * @param $error - * @param array|null $nodes - * @param array|null $path + * @param mixed $error + * @param Node[]|null $nodes + * @param mixed[]|null $path * @return Error */ public static function createLocatedError($error, $nodes = null, $path = null) @@ -99,22 +145,22 @@ class Error extends \Exception implements \JsonSerializable, ClientAware return $error; } else { $nodes = $nodes ?: $error->nodes; - $path = $path ?: $error->path; + $path = $path ?: $error->path; } } - $source = $positions = $originalError = null; + $source = $positions = $originalError = null; $extensions = []; if ($error instanceof self) { - $message = $error->getMessage(); + $message = $error->getMessage(); $originalError = $error; - $nodes = $error->nodes ?: $nodes; - $source = $error->source; - $positions = $error->positions; - $extensions = $error->extensions; - } else if ($error instanceof \Exception || $error instanceof \Throwable) { - $message = $error->getMessage(); + $nodes = $error->nodes ?: $nodes; + $source = $error->source; + $positions = $error->positions; + $extensions = $error->extensions; + } elseif ($error instanceof \Exception || $error instanceof \Throwable) { + $message = $error->getMessage(); $originalError = $error; } else { $message = (string) $error; @@ -131,66 +177,14 @@ class Error extends \Exception implements \JsonSerializable, ClientAware ); } - /** - * @param Error $error - * @return array + * @return mixed[] */ public static function formatError(Error $error) { return $error->toSerializableArray(); } - /** - * @param string $message - * @param array|Node|null $nodes - * @param Source $source - * @param array|null $positions - * @param array|null $path - * @param \Throwable $previous - * @param array $extensions - */ - public function __construct( - $message, - $nodes = null, - Source $source = null, - $positions = null, - $path = null, - $previous = null, - array $extensions = [] - ) - { - parent::__construct($message, 0, $previous); - - // Compute list of blame nodes. - if ($nodes instanceof \Traversable) { - $nodes = iterator_to_array($nodes); - } else if ($nodes && !is_array($nodes)) { - $nodes = [$nodes]; - } - - $this->nodes = $nodes; - $this->source = $source; - $this->positions = $positions; - $this->path = $path; - $this->extensions = $extensions ?: ( - $previous && $previous instanceof self - ? $previous->extensions - : [] - ); - - if ($previous instanceof ClientAware) { - $this->isClientSafe = $previous->isClientSafe(); - $this->category = $previous->getCategory() ?: static::CATEGORY_INTERNAL; - } else if ($previous) { - $this->isClientSafe = false; - $this->category = static::CATEGORY_INTERNAL; - } else { - $this->isClientSafe = true; - $this->category = static::CATEGORY_GRAPHQL; - } - } - /** * @inheritdoc */ @@ -212,29 +206,36 @@ class Error extends \Exception implements \JsonSerializable, ClientAware */ public function getSource() { - if (null === $this->source) { - if (!empty($this->nodes[0]) && !empty($this->nodes[0]->loc)) { + if ($this->source === null) { + if (! empty($this->nodes[0]) && ! empty($this->nodes[0]->loc)) { $this->source = $this->nodes[0]->loc->source; } } + return $this->source; } /** - * @return array + * @return int[] */ public function getPositions() { - if (null === $this->positions) { - if (!empty($this->nodes)) { - $positions = array_map(function($node) { + if ($this->positions === null && ! empty($this->nodes)) { + $positions = array_map( + function ($node) { return isset($node->loc) ? $node->loc->start : null; - }, $this->nodes); - $this->positions = array_filter($positions, function($p) { + }, + $this->nodes + ); + + $this->positions = array_filter( + $positions, + function ($p) { return $p !== null; - }); - } + } + ); } + return $this->positions; } @@ -254,21 +255,29 @@ class Error extends \Exception implements \JsonSerializable, ClientAware */ public function getLocations() { - if (null === $this->locations) { + if ($this->locations === null) { $positions = $this->getPositions(); - $source = $this->getSource(); - $nodes = $this->nodes; + $source = $this->getSource(); + $nodes = $this->nodes; if ($positions && $source) { - $this->locations = array_map(function ($pos) use ($source) { - return $source->getLocation($pos); - }, $positions); - } else if ($nodes) { - $this->locations = array_filter(array_map(function ($node) { - if ($node->loc) { - return $node->loc->source->getLocation($node->loc->start); - } - }, $nodes)); + $this->locations = array_map( + function ($pos) use ($source) { + return $source->getLocation($pos); + }, + $positions + ); + } elseif ($nodes) { + $this->locations = array_filter( + array_map( + function ($node) { + if ($node->loc) { + return $node->loc->source->getLocation($node->loc->start); + } + }, + $nodes + ) + ); } else { $this->locations = []; } @@ -278,7 +287,7 @@ class Error extends \Exception implements \JsonSerializable, ClientAware } /** - * @return array|Node[]|null + * @return Node[]|null */ public function getNodes() { @@ -290,7 +299,7 @@ class Error extends \Exception implements \JsonSerializable, ClientAware * Only included for execution errors. * * @api - * @return array|null + * @return mixed[]|null */ public function getPath() { @@ -298,7 +307,7 @@ class Error extends \Exception implements \JsonSerializable, ClientAware } /** - * @return array + * @return mixed[] */ public function getExtensions() { @@ -309,26 +318,29 @@ class Error extends \Exception implements \JsonSerializable, ClientAware * Returns array representation of error suitable for serialization * * @deprecated Use FormattedError::createFromException() instead - * @return array + * @return mixed[] */ public function toSerializableArray() { $arr = [ - 'message' => $this->getMessage() + 'message' => $this->getMessage(), ]; if ($this->getExtensions()) { $arr = array_merge($this->getExtensions(), $arr); } - $locations = Utils::map($this->getLocations(), function(SourceLocation $loc) { - return $loc->toSerializableArray(); - }); + $locations = Utils::map( + $this->getLocations(), + function (SourceLocation $loc) { + return $loc->toSerializableArray(); + } + ); - if (!empty($locations)) { + if (! empty($locations)) { $arr['locations'] = $locations; } - if (!empty($this->path)) { + if (! empty($this->path)) { $arr['path'] = $this->path; } @@ -340,9 +352,8 @@ class Error extends \Exception implements \JsonSerializable, ClientAware * @link http://php.net/manual/en/jsonserializable.jsonserialize.php * @return mixed data which can be serialized by json_encode, * which is a value of any type other than a resource. - * @since 5.4.0 */ - function jsonSerialize() + public function jsonSerialize() { return $this->toSerializableArray(); } diff --git a/src/Error/FormattedError.php b/src/Error/FormattedError.php index 82bb0cb..b73e641 100644 --- a/src/Error/FormattedError.php +++ b/src/Error/FormattedError.php @@ -1,4 +1,7 @@ nodes) { /** @var Node $node */ - foreach($error->nodes as $node) { - if ($node->loc) { - $printedLocations[] = self::highlightSourceAtLocation( - $node->loc->source, - $node->loc->source->getLocation($node->loc->start) - ); + foreach ($error->nodes as $node) { + if (! $node->loc) { + continue; } + + if ($node->loc->source === null) { + continue; + } + + $printedLocations[] = self::highlightSourceAtLocation( + $node->loc->source, + $node->loc->source->getLocation($node->loc->start) + ); } - } else if ($error->getSource() && $error->getLocations()) { + } elseif ($error->getSource() && $error->getLocations()) { $source = $error->getSource(); - foreach($error->getLocations() as $location) { + foreach ($error->getLocations() as $location) { $printedLocations[] = self::highlightSourceAtLocation($source, $location); } } - return !$printedLocations + return ! $printedLocations ? $error->getMessage() - : join("\n\n", array_merge([$error->getMessage()], $printedLocations)) . "\n"; + : implode("\n\n", array_merge([$error->getMessage()], $printedLocations)) . "\n"; } /** * Render a helpful description of the location of the error in the GraphQL * Source document. * - * @param Source $source - * @param SourceLocation $location * @return string */ private static function highlightSourceAtLocation(Source $source, SourceLocation $location) { - $line = $location->line; - $lineOffset = $source->locationOffset->line - 1; - $columnOffset = self::getColumnOffset($source, $location); - $contextLine = $line + $lineOffset; + $line = $location->line; + $lineOffset = $source->locationOffset->line - 1; + $columnOffset = self::getColumnOffset($source, $location); + $contextLine = $line + $lineOffset; $contextColumn = $location->column + $columnOffset; - $prevLineNum = (string) ($contextLine - 1); - $lineNum = (string) $contextLine; - $nextLineNum = (string) ($contextLine + 1); - $padLen = strlen($nextLineNum); - $lines = preg_split('/\r\n|[\n\r]/', $source->body); + $prevLineNum = (string) ($contextLine - 1); + $lineNum = (string) $contextLine; + $nextLineNum = (string) ($contextLine + 1); + $padLen = strlen($nextLineNum); + $lines = preg_split('/\r\n|[\n\r]/', $source->body); $lines[0] = self::whitespace($source->locationOffset->column - 1) . $lines[0]; $outputLines = [ - "{$source->name} ($contextLine:$contextColumn)", + sprintf('%s (%s:%s)', $source->name, $contextLine, $contextColumn), $line >= 2 ? (self::lpad($padLen, $prevLineNum) . ': ' . $lines[$line - 2]) : null, self::lpad($padLen, $lineNum) . ': ' . $lines[$line - 1], self::whitespace(2 + $padLen + $contextColumn - 1) . '^', - $line < count($lines)? self::lpad($padLen, $nextLineNum) . ': ' . $lines[$line] : null + $line < count($lines) ? self::lpad($padLen, $nextLineNum) . ': ' . $lines[$line] : null, ]; - return join("\n", array_filter($outputLines)); + return implode("\n", array_filter($outputLines)); } /** - * @param Source $source - * @param SourceLocation $location * @return int */ private static function getColumnOffset(Source $source, SourceLocation $location) @@ -109,7 +134,8 @@ class FormattedError * @param int $len * @return string */ - private static function whitespace($len) { + private static function whitespace($len) + { return str_repeat(' ', $len); } @@ -117,7 +143,8 @@ class FormattedError * @param int $len * @return string */ - private static function lpad($len, $str) { + private static function lpad($len, $str) + { return self::whitespace($len - mb_strlen($str)) . $str; } @@ -132,16 +159,16 @@ class FormattedError * * @api * @param \Throwable $e - * @param bool|int $debug - * @param string $internalErrorMessage - * @return array + * @param bool|int $debug + * @param string $internalErrorMessage + * @return mixed[] * @throws \Throwable */ public static function createFromException($e, $debug = false, $internalErrorMessage = null) { Utils::invariant( $e instanceof \Exception || $e instanceof \Throwable, - "Expected exception, got %s", + 'Expected exception, got %s', Utils::getVariableType($e) ); @@ -149,13 +176,13 @@ class FormattedError if ($e instanceof ClientAware) { $formattedError = [ - 'message' => $e->isClientSafe() ? $e->getMessage() : $internalErrorMessage, - 'category' => $e->getCategory() + 'message' => $e->isClientSafe() ? $e->getMessage() : $internalErrorMessage, + 'category' => $e->getCategory(), ]; } else { $formattedError = [ - 'message' => $internalErrorMessage, - 'category' => Error::CATEGORY_INTERNAL + 'message' => $internalErrorMessage, + 'category' => Error::CATEGORY_INTERNAL, ]; } @@ -164,14 +191,17 @@ class FormattedError $formattedError = array_merge($e->getExtensions(), $formattedError); } - $locations = Utils::map($e->getLocations(), function(SourceLocation $loc) { - return $loc->toSerializableArray(); - }); + $locations = Utils::map( + $e->getLocations(), + function (SourceLocation $loc) { + return $loc->toSerializableArray(); + } + ); - if (!empty($locations)) { + if (! empty($locations)) { $formattedError['locations'] = $locations; } - if (!empty($e->path)) { + if (! empty($e->path)) { $formattedError['path'] = $e->path; } } @@ -187,35 +217,37 @@ class FormattedError * Decorates spec-compliant $formattedError with debug entries according to $debug flags * (see GraphQL\Error\Debug for available flags) * - * @param array $formattedError + * @param mixed[] $formattedError * @param \Throwable $e - * @param bool $debug - * @return array + * @param bool $debug + * @return mixed[] * @throws \Throwable */ public static function addDebugEntries(array $formattedError, $e, $debug) { - if (!$debug) { + if (! $debug) { return $formattedError; } Utils::invariant( $e instanceof \Exception || $e instanceof \Throwable, - "Expected exception, got %s", + 'Expected exception, got %s', Utils::getVariableType($e) ); $debug = (int) $debug; if ($debug & Debug::RETHROW_INTERNAL_EXCEPTIONS) { - if (!$e instanceof Error) { + if (! $e instanceof Error) { throw $e; - } else if ($e->getPrevious()) { + } + + if ($e->getPrevious()) { throw $e->getPrevious(); } } - $isInternal = !$e instanceof ClientAware || !$e->isClientSafe(); + $isInternal = ! $e instanceof ClientAware || ! $e->isClientSafe(); if (($debug & Debug::INCLUDE_DEBUG_MESSAGE) && $isInternal) { // Displaying debugMessage as a first entry: @@ -230,13 +262,14 @@ class FormattedError ]; } - $isTrivial = $e instanceof Error && !$e->getPrevious(); + $isTrivial = $e instanceof Error && ! $e->getPrevious(); - if (!$isTrivial) { - $debugging = $e->getPrevious() ?: $e; + if (! $isTrivial) { + $debugging = $e->getPrevious() ?: $e; $formattedError['trace'] = static::toSafeTrace($debugging); } } + return $formattedError; } @@ -244,20 +277,20 @@ class FormattedError * Prepares final error formatter taking in account $debug flags. * If initial formatter is not set, FormattedError::createFromException is used * - * @param callable|null $formatter - * @param $debug + * @param bool $debug * @return callable|\Closure */ - public static function prepareFormatter(callable $formatter = null, $debug) + public static function prepareFormatter(?callable $formatter = null, $debug) { - $formatter = $formatter ?: function($e) { + $formatter = $formatter ?: function ($e) { return FormattedError::createFromException($e); }; if ($debug) { - $formatter = function($e) use ($formatter, $debug) { + $formatter = function ($e) use ($formatter, $debug) { return FormattedError::addDebugEntries($formatter($e), $e, $debug); }; } + return $formatter; } @@ -266,45 +299,45 @@ class FormattedError * * @api * @param \Throwable $error - * @return array + * @return mixed[] */ public static function toSafeTrace($error) { $trace = $error->getTrace(); - // Remove invariant entries as they don't provide much value: - if ( - isset($trace[0]['function']) && isset($trace[0]['class']) && - ('GraphQL\Utils\Utils::invariant' === $trace[0]['class'].'::'.$trace[0]['function'])) { + if (isset($trace[0]['function']) && isset($trace[0]['class']) && + // Remove invariant entries as they don't provide much value: + ($trace[0]['class'] . '::' . $trace[0]['function'] === 'GraphQL\Utils\Utils::invariant')) { + array_shift($trace); + } elseif (! isset($trace[0]['file'])) { + // Remove root call as it's likely error handler trace: array_shift($trace); } - // Remove root call as it's likely error handler trace: - else if (!isset($trace[0]['file'])) { - array_shift($trace); - } + return array_map( + function ($err) { + $safeErr = array_intersect_key($err, ['file' => true, 'line' => true]); - return array_map(function($err) { - $safeErr = array_intersect_key($err, ['file' => true, 'line' => true]); + if (isset($err['function'])) { + $func = $err['function']; + $args = ! empty($err['args']) ? array_map([__CLASS__, 'printVar'], $err['args']) : []; + $funcStr = $func . '(' . implode(', ', $args) . ')'; - if (isset($err['function'])) { - $func = $err['function']; - $args = !empty($err['args']) ? array_map([__CLASS__, 'printVar'], $err['args']) : []; - $funcStr = $func . '(' . implode(", ", $args) . ')'; - - if (isset($err['class'])) { - $safeErr['call'] = $err['class'] . '::' . $funcStr; - } else { - $safeErr['function'] = $funcStr; + if (isset($err['class'])) { + $safeErr['call'] = $err['class'] . '::' . $funcStr; + } else { + $safeErr['function'] = $funcStr; + } } - } - return $safeErr; - }, $trace); + return $safeErr; + }, + $trace + ); } /** - * @param $var + * @param mixed $var * @return string */ public static function printVar($var) @@ -314,6 +347,7 @@ class FormattedError if ($var instanceof WrappingType) { $var = $var->getWrappedType(true); } + return 'GraphQLType: ' . $var->name; } @@ -323,7 +357,7 @@ class FormattedError if (is_array($var)) { return 'array(' . count($var) . ')'; } - if ('' === $var) { + if ($var === '') { return '(empty string)'; } if (is_string($var)) { @@ -335,42 +369,45 @@ class FormattedError if (is_scalar($var)) { return $var; } - if (null === $var) { + if ($var === null) { return 'null'; } + return gettype($var); } /** * @deprecated as of v0.8.0 - * @param $error + * @param string $error * @param SourceLocation[] $locations - * @return array + * @return mixed[] */ public static function create($error, array $locations = []) { - $formatted = [ - 'message' => $error - ]; + $formatted = ['message' => $error]; - if (!empty($locations)) { - $formatted['locations'] = array_map(function($loc) { return $loc->toArray();}, $locations); + if (! empty($locations)) { + $formatted['locations'] = array_map( + function ($loc) { + return $loc->toArray(); + }, + $locations + ); } return $formatted; } /** - * @param \ErrorException $e * @deprecated as of v0.10.0, use general purpose method createFromException() instead - * @return array + * @return mixed[] */ public static function createFromPHPError(\ErrorException $e) { return [ - 'message' => $e->getMessage(), + 'message' => $e->getMessage(), 'severity' => $e->getSeverity(), - 'trace' => self::toSafeTrace($e) + 'trace' => self::toSafeTrace($e), ]; } } diff --git a/src/Error/InvariantViolation.php b/src/Error/InvariantViolation.php index f52793f..f108109 100644 --- a/src/Error/InvariantViolation.php +++ b/src/Error/InvariantViolation.php @@ -1,4 +1,7 @@ 0 && !isset(self::$warned[$warningId])) { + } elseif ((self::$enableWarnings & $warningId) > 0 && ! isset(self::$warned[$warningId])) { self::$warned[$warningId] = true; trigger_error($errorMessage, $messageLevel ?: E_USER_WARNING); } } - static function warn($errorMessage, $warningId, $messageLevel = null) + public static function warn($errorMessage, $warningId, $messageLevel = null) { if (self::$warningHandler) { $fn = self::$warningHandler; $fn($errorMessage, $warningId); - } else if ((self::$enableWarnings & $warningId) > 0) { + } elseif ((self::$enableWarnings & $warningId) > 0) { trigger_error($errorMessage, $messageLevel ?: E_USER_WARNING); } } From f44ff2cfe7d02cd438fdf7abefd961bad00a55be Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Thu, 21 Jun 2018 10:18:34 +0200 Subject: [PATCH 3/3] Add composer script for linting --- composer.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/composer.json b/composer.json index d4de73a..dc8e02f 100644 --- a/composer.json +++ b/composer.json @@ -36,5 +36,8 @@ "suggest": { "react/promise": "To leverage async resolving on React PHP platform", "psr/http-message": "To use standard GraphQL server" + }, + "scripts": { + "lint" : "vendor/bin/phpcs" } }