From 750ec00d7eb67d68a9bdaca7fed5f83e2b36a413 Mon Sep 17 00:00:00 2001 From: Andrii Date: Tue, 19 Nov 2024 01:12:28 +0200 Subject: [PATCH 1/4] :sparkles: Support for using attribute as a target to parameter. Deprecate using as a target to the method. Add warning about next major release changes Refs: https://github.com/thecodingmachine/graphqlite/issues/708#issuecomment-2484300197 --- src/Annotations/Assertion.php | 22 +++++-- tests/Annotations/AssertionTest.php | 9 +-- tests/Fixtures/Controllers/UserController.php | 11 +++- .../InvalidControllers/InvalidController.php | 7 +- tests/IntegrationTest.php | 65 ++++++++++++++++++- 5 files changed, 94 insertions(+), 20 deletions(-) diff --git a/src/Annotations/Assertion.php b/src/Annotations/Assertion.php index 51bc4ea..0d383c8 100644 --- a/src/Annotations/Assertion.php +++ b/src/Annotations/Assertion.php @@ -15,6 +15,10 @@ /** * Use this annotation to validate a parameter for a query or mutation. * + * Note 1: using this attribute as a target to the method (not parameter) is deprecated and will be removed in 8.0. + * Note 2: support for `doctrine/annotations` will be removed in 8.0. + * Note 3: this class won't implement `ParameterAnnotationInterface` in 8.0. + * * @Annotation * @Target({"METHOD"}) * @Attributes({ @@ -22,10 +26,10 @@ * @Attribute("constraint", type = "Symfony\Component\Validator\Constraint[]|Symfony\Component\Validator\Constraint") * }) */ -#[Attribute(Attribute::TARGET_METHOD)] +#[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_PARAMETER)] class Assertion implements ParameterAnnotationInterface { - private string $for; + private ?string $for = null; /** @var Constraint[] */ private array $constraint; @@ -40,20 +44,24 @@ public function __construct( ) { $for = $for ?? $values['for'] ?? null; $constraint = $constraint ?? $values['constraint'] ?? null; - if ($for === null) { - throw new BadMethodCallException('The Assert attribute must be passed a target. For instance: "#[Assert(for: "$email", constraint: new Email())"'); - } if ($constraint === null) { - throw new BadMethodCallException('The Assert attribute must be passed one or many constraints. For instance: "#[Assert(for: "$email", constraint: new Email())"'); + throw new BadMethodCallException('The Assertion attribute must be passed one or many constraints. For instance: "#[Assertion(constraint: new Email())"'); } - $this->for = ltrim($for, '$'); $this->constraint = is_array($constraint) ? $constraint : [$constraint]; + + if (null !== $for) { + $this->for = ltrim($for, '$'); + } } public function getTarget(): string { + if ($this->for === null) { + throw new BadMethodCallException('The Assertion attribute must be passed a target. For instance: "#[Assertion(for: "$email", constraint: new Email())"'); + } + return $this->for; } diff --git a/tests/Annotations/AssertionTest.php b/tests/Annotations/AssertionTest.php index 72d1e6a..8bde87d 100644 --- a/tests/Annotations/AssertionTest.php +++ b/tests/Annotations/AssertionTest.php @@ -9,17 +9,10 @@ class AssertionTest extends TestCase { - public function testException1(): void - { - $this->expectException(BadMethodCallException::class); - $this->expectExceptionMessage('The Assert attribute must be passed a target. For instance: "#[Assert(for: "$email", constraint: new Email())"'); - new Assertion([]); - } - public function testException2(): void { $this->expectException(BadMethodCallException::class); - $this->expectExceptionMessage('The Assert attribute must be passed one or many constraints. For instance: "#[Assert(for: "$email", constraint: new Email())"'); + $this->expectExceptionMessage('The Assertion attribute must be passed one or many constraints. For instance: "#[Assertion(constraint: new Email())"'); new Assertion(['for' => 'foo']); } } diff --git a/tests/Fixtures/Controllers/UserController.php b/tests/Fixtures/Controllers/UserController.php index 8041f61..811d391 100644 --- a/tests/Fixtures/Controllers/UserController.php +++ b/tests/Fixtures/Controllers/UserController.php @@ -35,8 +35,17 @@ public function createUser(string $email, string $password): User #[Query] #[Assertion(for: '$email', constraint: new Assert\Email())] - public function findByMail(string $email = 'a@a.com'): User + public function findByMailTargetMethod(string $email = 'a@a.com'): User { + // deprecated way of using the Assertion annotation - as a target of the method + return new User($email, 'foo'); + } + + #[Query] + public function findByMail( + #[Assertion(constraint: new Assert\Email())] + string $email = 'a@a.com', + ): User { return new User($email, 'foo'); } } diff --git a/tests/Fixtures/InvalidControllers/InvalidController.php b/tests/Fixtures/InvalidControllers/InvalidController.php index 428072d..ebf166f 100644 --- a/tests/Fixtures/InvalidControllers/InvalidController.php +++ b/tests/Fixtures/InvalidControllers/InvalidController.php @@ -12,9 +12,10 @@ class InvalidController { #[Query] - #[Assertion(for: '$resolveInfo', constraint: new Assert\Email())] - public function invalid(ResolveInfo $resolveInfo): string - { + public function invalid( + #[Assertion(for: '$resolveInfo', constraint: new Assert\Email())] + ResolveInfo $resolveInfo, + ): string { return 'foo'; } } diff --git a/tests/IntegrationTest.php b/tests/IntegrationTest.php index a9048ad..f472e9d 100644 --- a/tests/IntegrationTest.php +++ b/tests/IntegrationTest.php @@ -108,7 +108,6 @@ public function testEndToEndAssert(): void $errors = $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['errors']; - // TODO: find why message is not in French... $this->assertSame('This value is not a valid email address.', $errors[0]['message']); $this->assertSame('email', $errors[0]['extensions']['field']); $this->assertSame('Validate', $errors[0]['extensions']['category']); @@ -151,6 +150,70 @@ public function testEndToEndAssert(): void $this->assertSame('a@a.com', $data['findByMail']['email']); } + public function testEndToEndAssertForDeprecatedWay(): void + { + $schema = $this->getSchema(); + $schema->assertValid(); + + $queryString = ' + { + findByMailTargetMethod(email: "notvalid") { + email + } + } + '; + + $result = GraphQL::executeQuery( + $schema, + $queryString, + ); + $result->setErrorsHandler([WebonyxErrorHandler::class, 'errorHandler']); + $result->setErrorFormatter([WebonyxErrorHandler::class, 'errorFormatter']); + + $errors = $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['errors']; + + $this->assertSame('This value is not a valid email address.', $errors[0]['message']); + $this->assertSame('email', $errors[0]['extensions']['field']); + $this->assertSame('Validate', $errors[0]['extensions']['category']); + + $queryString = ' + { + findByMailTargetMethod(email: "valid@valid.com") { + email + } + } + '; + + $result = GraphQL::executeQuery( + $schema, + $queryString, + ); + $result->setErrorsHandler([WebonyxErrorHandler::class, 'errorHandler']); + $result->setErrorFormatter([WebonyxErrorHandler::class, 'errorFormatter']); + + $data = $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['data']; + $this->assertSame('valid@valid.com', $data['findByMailTargetMethod']['email']); + + // Test default parameter + $queryString = ' + { + findByMailTargetMethod { + email + } + } + '; + + $result = GraphQL::executeQuery( + $schema, + $queryString, + ); + $result->setErrorsHandler([WebonyxErrorHandler::class, 'errorHandler']); + $result->setErrorFormatter([WebonyxErrorHandler::class, 'errorFormatter']); + + $data = $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['data']; + $this->assertSame('a@a.com', $data['findByMailTargetMethod']['email']); + } + public function testException(): void { $schemaFactory = $this->getSchemaFactory(); From 037f9ee8877d81d54fef9ed4ff231769f8bf7a08 Mon Sep 17 00:00:00 2001 From: Andrii Date: Wed, 27 Nov 2024 00:18:03 +0200 Subject: [PATCH 2/4] :package: Add runtime deprecation when there is used deprecated way to configure attribute --- src/Annotations/Assertion.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Annotations/Assertion.php b/src/Annotations/Assertion.php index 0d383c8..ac73acd 100644 --- a/src/Annotations/Assertion.php +++ b/src/Annotations/Assertion.php @@ -52,6 +52,12 @@ public function __construct( $this->constraint = is_array($constraint) ? $constraint : [$constraint]; if (null !== $for) { + trigger_error( + "Using #[Assertion(for='" . $for . "', constaint='...')] on methods is deprecated in favor " . + "of #[Assertion(constraint='...')] the parameter itself.", + E_USER_DEPRECATED, + ); + $this->for = ltrim($for, '$'); } } From dd402ec50c145681a6890d0b79bb0f59a9cd1140 Mon Sep 17 00:00:00 2001 From: Andrii Date: Wed, 27 Nov 2024 00:18:37 +0200 Subject: [PATCH 3/4] :lipstick: Fix CS --- src/Annotations/Assertion.php | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/Annotations/Assertion.php b/src/Annotations/Assertion.php index ac73acd..a33a74e 100644 --- a/src/Annotations/Assertion.php +++ b/src/Annotations/Assertion.php @@ -11,6 +11,9 @@ use function is_array; use function ltrim; +use function trigger_error; + +use const E_USER_DEPRECATED; /** * Use this annotation to validate a parameter for a query or mutation. @@ -29,7 +32,7 @@ #[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_PARAMETER)] class Assertion implements ParameterAnnotationInterface { - private ?string $for = null; + private string|null $for = null; /** @var Constraint[] */ private array $constraint; @@ -51,15 +54,17 @@ public function __construct( $this->constraint = is_array($constraint) ? $constraint : [$constraint]; - if (null !== $for) { - trigger_error( - "Using #[Assertion(for='" . $for . "', constaint='...')] on methods is deprecated in favor " . - "of #[Assertion(constraint='...')] the parameter itself.", - E_USER_DEPRECATED, - ); - - $this->for = ltrim($for, '$'); + if ($for === null) { + return; } + + trigger_error( + "Using #[Assertion(for='" . $for . "', constaint='...')] on methods is deprecated in favor " . + "of #[Assertion(constraint='...')] the parameter itself.", + E_USER_DEPRECATED, + ); + + $this->for = ltrim($for, '$'); } public function getTarget(): string From 0e6a7b79730cd7845da9e46d4e7dd8ddbbe43237 Mon Sep 17 00:00:00 2001 From: Andrii Date: Wed, 27 Nov 2024 00:21:00 +0200 Subject: [PATCH 4/4] :package: Fulfill phpstan config to improve DX --- composer.json | 2 +- phpstan.neon | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 0dd224e..fc7345b 100644 --- a/composer.json +++ b/composer.json @@ -30,7 +30,7 @@ "doctrine/coding-standard": "^11.1|^12.0" }, "scripts": { - "phpstan": "phpstan analyse src/ -c phpstan.neon --level=7 --no-progress", + "phpstan": "phpstan analyse --no-progress", "cs-check": "phpcs", "cs-fix": "phpcbf", "test": ["@cs-check", "@phpstan", "phpunit"] diff --git a/phpstan.neon b/phpstan.neon index cb5b24e..66327bc 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,4 +1,9 @@ parameters: + level: 7 + + paths: + - src + ignoreErrors: excludePaths: @@ -7,4 +12,4 @@ parameters: - .phpstan-cache #includes: -# - vendor/thecodingmachine/phpstan-strict-rules/phpstan-strict-rules.neon \ No newline at end of file +# - vendor/thecodingmachine/phpstan-strict-rules/phpstan-strict-rules.neon