Skip to content

Implement JsonValidateTypeSpecifyingExtension #2491

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 6 commits into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jun 27, 2023

PHP8.3 Support for https://wiki.php.net/rfc/json_validate

this type-specifying extension narrows the $json arg to a non-empty-string

@staabm staabm changed the base branch from 1.11.x to 1.10.x June 27, 2023 15:20
@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

use PHPStan\Type\FunctionTypeSpecifyingExtension;
use function count;

class JsonValidateTypeSpecifyingExtension implements FunctionTypeSpecifyingExtension, TypeSpecifierAwareExtension
Copy link
Contributor Author

@staabm staabm Jun 27, 2023

Choose a reason for hiding this comment

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

I tried using a stub signature instead, but this did not work for me locally:

/**
 * @phpstan-assert-if-true non-falsy-string $json
 * @return bool
 */
function json_validate(string $json, int $depth = 512, int $flags = 0): bool {}

as I can see this extension also only work in CI PHP 8.3.. I will retry using a stub signature on a different composer which has PHP 8.3 installed

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, did a few more tests. It does not work with pure stub signatures, because these would also affect the ELSE case in a wrong way:

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/data/json_validate.php:13" ('type', '/Users/staabm/workspace/phpst...te.php', PHPStan\Type\Constant\ConstantStringType Object (...), PHPStan\Type\MixedType Object (...), 13)
Expected type mixed, got type mixed~non-falsy-string in /Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/data/json_validate.php on line 13.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'mixed'
+'mixed~non-falsy-string'

@staabm staabm marked this pull request as ready for review June 27, 2023 18:28
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

*/
return [
'new' => [
'json_validate' => ['bool', 'json'=>'string', 'depth='=>'positive-int', 'flags='=>'int-mask<JSON_INVALID_UTF8_IGNORE>'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only a single flag is supported, see php/php-src#9399 (comment)

return new SpecifiedTypes([], []);
}

return $this->typeSpecifier->create($node->getArgs()[0]->value, new AccessoryNonFalsyStringType(), $context, false, $scope);
Copy link
Contributor

@mad-briller mad-briller Jun 28, 2023

Choose a reason for hiding this comment

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

is non falsy string type correct here?

https://3v4l.org/5IGU2/rfc#vgit.master

'0' is a valid json string (according to php anyway) but is also falsey

Copy link
Contributor Author

@staabm staabm Jun 28, 2023

Choose a reason for hiding this comment

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

You are right. I did a 3v4l before with this exact case in mind. Seems I fucked it up.

Need to go with non-empty-string then :-)

@ondrejmirtes
Copy link
Member

Hi, I usually start preparing next PHP version support after feature freeze but I decided to make an exception here.

Here's what I did:

@staabm staabm deleted the json-vaidate branch June 28, 2023 11:39
@staabm
Copy link
Contributor Author

staabm commented Jun 28, 2023

thank you

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.

4 participants