Skip to content
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

PHPStan level 7 #1045

Merged
merged 35 commits into from Jan 4, 2022
Merged

PHPStan level 7 #1045

merged 35 commits into from Jan 4, 2022

Conversation

spawnia
Copy link
Collaborator

@spawnia spawnia commented Dec 29, 2021

No description provided.

@spawnia spawnia marked this pull request as ready for review January 2, 2022 12:13
@spawnia spawnia requested a review from simPod January 2, 2022 12:13
phpstan.neon.dist Outdated Show resolved Hide resolved
src/Language/AST/NodeList.php Outdated Show resolved Hide resolved
src/Language/BlockString.php Outdated Show resolved Hide resolved
src/Language/Lexer.php Outdated Show resolved Hide resolved
src/Utils/SchemaPrinter.php Show resolved Hide resolved
src/Utils/Utils.php Show resolved Hide resolved
src/Utils/Value.php Outdated Show resolved Hide resolved
spawnia and others added 2 commits January 2, 2022 16:42
Co-authored-by: Simon Podlipsky <simon@podlipsky.net>
src/Language/Lexer.php Outdated Show resolved Hide resolved
src/Language/Lexer.php Outdated Show resolved Hide resolved
src/Utils/Utils.php Outdated Show resolved Hide resolved
tests/Type/ScalarSerializationTest.php Show resolved Hide resolved
Copy link
Collaborator

@simPod simPod left a comment

Choose a reason for hiding this comment

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

wow btw

spawnia and others added 2 commits January 4, 2022 15:53
Co-authored-by: Simon Podlipsky <simon@podlipsky.net>
Co-authored-by: Simon Podlipsky <simon@podlipsky.net>
src/Error/FormattedError.php Outdated Show resolved Hide resolved
@spawnia
Copy link
Collaborator Author

spawnia commented Jan 4, 2022

@simPod anything else before I 🚢 ?

@simPod
Copy link
Collaborator

simPod commented Jan 4, 2022

:shipit:

@spawnia spawnia merged commit 09b81c8 into master Jan 4, 2022
@spawnia spawnia deleted the phpstan-level-7 branch January 4, 2022 16:58
@shmax
Copy link
Contributor

shmax commented Jan 4, 2022

Nice one, way to kill it 👍

@@ -8,6 +8,5 @@ class FloatValueNode extends Node implements ValueNode
{
public string $kind = NodeKind::FLOAT;

/** @var string */
public $value;
public string $value;
Copy link

Choose a reason for hiding this comment

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

Why the value is not a float in the FloatValueNode ?
Same for IntValueNode. For BooleanValueNode this is fine, the value is a boolean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the case of BooleanValueNode, the PHP type can accurately represent all possible values. However, that is not the case for IntValueNode or FloatValueNode - the PHP types are limited in their range, whereas the GraphQL AST is not (see https://spec.graphql.org/draft/#sec-Int-Value - it can be an arbitrarily long sequence of digits, PHP int is limited to 32 bytes).

Scalar types are based upon those value nodes and impose certain restrictions (see https://spec.graphql.org/draft/#sec-Int), which allows us to parse incoming values of Int to a native PHP int (see

public function parseLiteral(Node $valueNode, ?array $variables = null): int
{
if ($valueNode instanceof IntValueNode) {
$val = (int) $valueNode->value;
if ($valueNode->value === (string) $val && $val >= self::MIN_INT && $val <= self::MAX_INT) {
return $val;
}
}
$notInt = Printer::doPrint($valueNode);
throw new Error("Int cannot represent non-integer value: {$notInt}", $valueNode);
}
). There may be other numeric scalar types based on the AST literal IntValueNode which do not fit a PHP int, e.g. BigInt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants