From 065fad5dd716558d954075bbc32093cec55594e2 Mon Sep 17 00:00:00 2001 From: Jochem Klaver Date: Fri, 13 May 2022 16:53:49 +0200 Subject: [PATCH 1/6] Catch and process errors that occure during annotation instantiation When an annotation class is instantiated and values are passed to the constructor, TypeErrors and ArgumentCountErrors can be thrown. This change catches those exceptions and shows an AnnotationException containing the annotation name and context. --- .../Annotations/AnnotationException.php | 5 +- lib/Doctrine/Common/Annotations/DocParser.php | 36 ++++++- phpstan.neon | 1 + .../Common/Annotations/DocParserTest.php | 100 +++++++++++++++--- 4 files changed, 122 insertions(+), 20 deletions(-) diff --git a/lib/Doctrine/Common/Annotations/AnnotationException.php b/lib/Doctrine/Common/Annotations/AnnotationException.php index b1ea64e6f..4d91825e5 100644 --- a/lib/Doctrine/Common/Annotations/AnnotationException.php +++ b/lib/Doctrine/Common/Annotations/AnnotationException.php @@ -3,6 +3,7 @@ namespace Doctrine\Common\Annotations; use Exception; +use Throwable; use function get_class; use function gettype; @@ -47,9 +48,9 @@ public static function semanticalError($message) * * @return AnnotationException */ - public static function creationError($message) + public static function creationError($message, ?Throwable $previous = null) { - return new self('[Creation Error] ' . $message); + return new self('[Creation Error] ' . $message, 0, $previous); } /** diff --git a/lib/Doctrine/Common/Annotations/DocParser.php b/lib/Doctrine/Common/Annotations/DocParser.php index ae530c50f..80f307cab 100644 --- a/lib/Doctrine/Common/Annotations/DocParser.php +++ b/lib/Doctrine/Common/Annotations/DocParser.php @@ -12,6 +12,7 @@ use ReflectionProperty; use RuntimeException; use stdClass; +use Throwable; use function array_keys; use function array_map; @@ -941,7 +942,7 @@ private function Annotation() if (self::$annotationMetadata[$name]['has_named_argument_constructor']) { if (PHP_VERSION_ID >= 80000) { - return new $name(...$values); + return $this->instantiateAnnotiation($originalName, $this->context, $name, $values); } $positionalValues = []; @@ -968,16 +969,16 @@ private function Annotation() $positionalValues[self::$annotationMetadata[$name]['constructor_args'][$property]['position']] = $value; } - return new $name(...$positionalValues); + return $this->instantiateAnnotiation($originalName, $this->context, $name, $positionalValues); } // check if the annotation expects values via the constructor, // or directly injected into public properties if (self::$annotationMetadata[$name]['has_constructor'] === true) { - return new $name($values); + return $this->instantiateAnnotiation($originalName, $this->context, $name, [$values]); } - $instance = new $name(); + $instance = $this->instantiateAnnotiation($originalName, $this->context, $name, []); foreach ($values as $property => $value) { if (! isset(self::$annotationMetadata[$name]['properties'][$property])) { @@ -1456,4 +1457,31 @@ private function resolvePositionalValues(array $arguments, string $name): array return $values; } + + /** + * Try to instantiate the annotation and catch and process any exceptions related to failure + * + * @param class-string $name + * @param array $arguments + * + * @return object + * + * @throws AnnotationException + */ + private function instantiateAnnotiation(string $originalName, string $context, string $name, array $arguments) + { + try { + return new $name(...$arguments); + } catch (Throwable $exception) { + throw AnnotationException::creationError( + sprintf( + 'An error occurred while instantiating the annotation @%s declared on %s: "%s".', + $originalName, + $context, + $exception->getMessage() + ), + $exception + ); + } + } } diff --git a/phpstan.neon b/phpstan.neon index ad8f23951..78663a129 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -13,6 +13,7 @@ parameters: ignoreErrors: - '#Instantiated class Doctrine_Tests_Common_Annotations_Fixtures_ClassNoNamespaceNoComment not found#' - '#Property Doctrine\\Tests\\Common\\Annotations\\DummyClassNonAnnotationProblem::\$foo has unknown class#' + - '#Call to an undefined static method PHPUnit\\Framework\\TestCase::expectExceptionMessageRegExp\(\)#' # That tag is empty on purpose - '#PHPDoc tag @var has invalid value \(\)\: Unexpected token "\*/", expected type at offset 9#' diff --git a/tests/Doctrine/Tests/Common/Annotations/DocParserTest.php b/tests/Doctrine/Tests/Common/Annotations/DocParserTest.php index fad9bf93b..5e6f35cb2 100644 --- a/tests/Doctrine/Tests/Common/Annotations/DocParserTest.php +++ b/tests/Doctrine/Tests/Common/Annotations/DocParserTest.php @@ -14,15 +14,19 @@ use Doctrine\Tests\Common\Annotations\Fixtures\ClassWithConstants; use Doctrine\Tests\Common\Annotations\Fixtures\InterfaceWithConstants; use InvalidArgumentException; +use PHPUnit\Framework\Constraint\ExceptionMessage; use PHPUnit\Framework\TestCase; use ReflectionClass; +use TypeError; use function array_column; use function array_combine; use function assert; use function class_exists; use function extension_loaded; +use function get_parent_class; use function ini_get; +use function method_exists; use function sprintf; use function ucfirst; @@ -886,9 +890,16 @@ public function testAnnotationEnumInvalidTypeDeclarationException(): void $docblock = '@Doctrine\Tests\Common\Annotations\Fixtures\AnnotationEnumInvalid("foo")'; $parser->setIgnoreNotImportedAnnotations(false); - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('@Enum supports only scalar values "array" given.'); - $parser->parse($docblock); + $this->expectException(AnnotationException::class); + try { + $parser->parse($docblock); + } catch (AnnotationException $exc) { + $previous = $exc->getPrevious(); + $this->assertInstanceOf(InvalidArgumentException::class, $previous); + $this->assertThat($previous, new ExceptionMessage('@Enum supports only scalar values "array" given.')); + + throw $exc; + } } public function testAnnotationEnumInvalidLiteralDeclarationException(): void @@ -897,9 +908,19 @@ public function testAnnotationEnumInvalidLiteralDeclarationException(): void $docblock = '@Doctrine\Tests\Common\Annotations\Fixtures\AnnotationEnumLiteralInvalid("foo")'; $parser->setIgnoreNotImportedAnnotations(false); - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('Undefined enumerator value "3" for literal "AnnotationEnumLiteral::THREE".'); - $parser->parse($docblock); + $this->expectException(AnnotationException::class); + try { + $parser->parse($docblock); + } catch (AnnotationException $exc) { + $previous = $exc->getPrevious(); + $this->assertInstanceOf(InvalidArgumentException::class, $previous); + $this->assertThat( + $previous, + new ExceptionMessage('Undefined enumerator value "3" for literal "AnnotationEnumLiteral::THREE".') + ); + + throw $exc; + } } /** @@ -1100,11 +1121,21 @@ public function testAnnotationWithInvalidTargetDeclarationError(): void DOCBLOCK; $parser->setTarget(Target::TARGET_CLASS); - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage( - 'Invalid Target "Foo". Available targets: [ALL, CLASS, METHOD, PROPERTY, FUNCTION, ANNOTATION]' - ); - $parser->parse($docblock, $context); + $this->expectException(AnnotationException::class); + try { + $parser->parse($docblock, $context); + } catch (AnnotationException $exc) { + $previous = $exc->getPrevious(); + $this->assertInstanceOf(InvalidArgumentException::class, $previous); + $this->assertThat( + $previous, + new ExceptionMessage( + 'Invalid Target "Foo". Available targets: [ALL, CLASS, METHOD, PROPERTY, FUNCTION, ANNOTATION]' + ) + ); + + throw $exc; + } } public function testAnnotationWithTargetEmptyError(): void @@ -1118,9 +1149,19 @@ public function testAnnotationWithTargetEmptyError(): void DOCBLOCK; $parser->setTarget(Target::TARGET_CLASS); - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('@Target expects either a string value, or an array of strings, "NULL" given.'); - $parser->parse($docblock, $context); + $this->expectException(AnnotationException::class); + try { + $parser->parse($docblock, $context); + } catch (AnnotationException $exc) { + $previous = $exc->getPrevious(); + $this->assertInstanceOf(InvalidArgumentException::class, $previous); + $this->assertThat( + $previous, + new ExceptionMessage('@Target expects either a string value, or an array of strings, "NULL" given.') + ); + + throw $exc; + } } /** @@ -1683,6 +1724,37 @@ public function testNamedArgumentsConstructorAnnotationWithInvalidArguments(): v ); $parser->parse('/** @AnotherNamedAnnotation("foo", bar=666, "hey") */'); } + + public function testNamedArgumentsConstructorAnnotationWithWrongArgumentType(): void + { + $context = 'property SomeClassName::invalidProperty.'; + $docblock = '@NamedAnnotationWithArray(foo = "no array!")'; + $parser = $this->createTestParser(); + $this->expectException(AnnotationException::class); + $this->expectExceptionMessageMatches( + '/\[Creation Error\] An error occurred while instantiating the annotation ' + . '@NamedAnnotationWithArray declared on property SomeClassName::invalidProperty\.: ".*"\.$/' + ); + try { + $parser->parse($docblock, $context); + } catch (AnnotationException $exc) { + $this->assertInstanceOf(TypeError::class, $exc->getPrevious()); + + throw $exc; + } + } + + /** + * Override for BC with PHPUnit <8 + */ + public function expectExceptionMessageMatches(string $regularExpression): void + { + if (method_exists(get_parent_class($this), 'expectExceptionMessageMatches')) { + parent::expectExceptionMessageMatches($regularExpression); + } else { + parent::expectExceptionMessageRegExp($regularExpression); + } + } } /** @Annotation */ From 296911c4ab9e4d8254de3ce34317283637b47e22 Mon Sep 17 00:00:00 2001 From: Dorian V Date: Fri, 1 Jul 2022 17:39:35 +0200 Subject: [PATCH 2/6] Ignore PHPStan annotation @readonly see https://github.com/phpstan/phpstan-src/pull/1295 --- .../Common/Annotations/ImplicitlyIgnoredAnnotationNames.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Doctrine/Common/Annotations/ImplicitlyIgnoredAnnotationNames.php b/lib/Doctrine/Common/Annotations/ImplicitlyIgnoredAnnotationNames.php index 2efeb1d22..ab27f8a5c 100644 --- a/lib/Doctrine/Common/Annotations/ImplicitlyIgnoredAnnotationNames.php +++ b/lib/Doctrine/Common/Annotations/ImplicitlyIgnoredAnnotationNames.php @@ -147,6 +147,7 @@ final class ImplicitlyIgnoredAnnotationNames // PHPStan, Psalm 'extends' => true, 'implements' => true, + 'readonly' => true, 'template' => true, 'use' => true, From 47fb9f9abdb12067d4b661f67d26e4dcf36a3953 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Fri, 1 Jul 2022 19:24:12 +0200 Subject: [PATCH 3/6] Whitelist necessary plugin Not doing so results in the coding standard job to fail. --- composer.json | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index a29863a71..8bddde7ac 100644 --- a/composer.json +++ b/composer.json @@ -27,7 +27,10 @@ "vimeo/psalm": "^4.10" }, "config": { - "sort-packages": true + "sort-packages": true, + "allow-plugins": { + "dealerdirect/phpcodesniffer-composer-installer": true + } }, "autoload": { "psr-4": { "Doctrine\\Common\\Annotations\\": "lib/Doctrine/Common/Annotations" } From 32305b94b874665895e1114c0778c53334808947 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Sat, 2 Jul 2022 12:17:31 +0200 Subject: [PATCH 4/6] Use latest versions of shared workflows --- .github/workflows/coding-standards.yml | 2 +- .github/workflows/continuous-integration.yml | 2 +- .github/workflows/release-on-milestone-closed.yml | 3 +-- .github/workflows/static-analysis.yml | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/.github/workflows/coding-standards.yml b/.github/workflows/coding-standards.yml index 352847e3b..3ae99378a 100644 --- a/.github/workflows/coding-standards.yml +++ b/.github/workflows/coding-standards.yml @@ -12,4 +12,4 @@ on: jobs: coding-standards: name: "Coding Standards" - uses: "doctrine/.github/.github/workflows/coding-standards.yml@1.1.1" + uses: "doctrine/.github/.github/workflows/coding-standards.yml@1.5.1" diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 2720d0b2e..ac5f75bb6 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -14,6 +14,6 @@ env: jobs: phpunit: name: "PHPUnit" - uses: "doctrine/.github/.github/workflows/continuous-integration.yml@1.1.1" + uses: "doctrine/.github/.github/workflows/continuous-integration.yml@1.5.1" with: php-versions: '["7.1", "7.2", "7.3", "7.4", "8.0"]' diff --git a/.github/workflows/release-on-milestone-closed.yml b/.github/workflows/release-on-milestone-closed.yml index 563333bb4..d32d76ef7 100644 --- a/.github/workflows/release-on-milestone-closed.yml +++ b/.github/workflows/release-on-milestone-closed.yml @@ -8,9 +8,8 @@ on: jobs: release: name: "Git tag, release & create merge-up PR" - uses: "doctrine/.github/.github/workflows/release-on-milestone-closed.yml@1.1.1" + uses: "doctrine/.github/.github/workflows/release-on-milestone-closed.yml@1.5.1" secrets: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} GIT_AUTHOR_EMAIL: ${{ secrets.GIT_AUTHOR_EMAIL }} GIT_AUTHOR_NAME: ${{ secrets.GIT_AUTHOR_NAME }} ORGANIZATION_ADMIN_TOKEN: ${{ secrets.ORGANIZATION_ADMIN_TOKEN }} diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml index ddcf2ddf7..4150ff29b 100644 --- a/.github/workflows/static-analysis.yml +++ b/.github/workflows/static-analysis.yml @@ -12,4 +12,4 @@ on: jobs: static-analysis: name: "Static Analysis" - uses: "doctrine/.github/.github/workflows/static-analysis.yml@1.1.1" + uses: "doctrine/.github/.github/workflows/static-analysis.yml@1.5.1" From 06a3f83864b622ace2dc5f6d7acdc65eb86a2cc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Sat, 2 Jul 2022 12:18:57 +0200 Subject: [PATCH 5/6] Add Composer Lint workflow --- .github/workflows/composer-lint.yml | 18 ++++++++++ composer.json | 51 ++++++++++++++++++++--------- 2 files changed, 54 insertions(+), 15 deletions(-) create mode 100644 .github/workflows/composer-lint.yml diff --git a/.github/workflows/composer-lint.yml b/.github/workflows/composer-lint.yml new file mode 100644 index 000000000..ae17b7af3 --- /dev/null +++ b/.github/workflows/composer-lint.yml @@ -0,0 +1,18 @@ +name: "Composer Lint" + +on: + pull_request: + branches: + - "*.x" + paths: + - "composer.json" + push: + branches: + - "*.x" + paths: + - "composer.json" + +jobs: + composer-lint: + name: "Composer Lint" + uses: "doctrine/.github/.github/workflows/composer-lint.yml@1.5.1" diff --git a/composer.json b/composer.json index 8bddde7ac..88fbc51ec 100644 --- a/composer.json +++ b/composer.json @@ -1,17 +1,36 @@ { "name": "doctrine/annotations", - "type": "library", "description": "Docblock Annotations Parser", - "keywords": ["annotations", "docblock", "parser"], - "homepage": "https://www.doctrine-project.org/projects/annotations.html", "license": "MIT", + "type": "library", + "keywords": [ + "annotations", + "docblock", + "parser" + ], "authors": [ - {"name": "Guilherme Blanco", "email": "guilhermeblanco@gmail.com"}, - {"name": "Roman Borschel", "email": "roman@code-factory.org"}, - {"name": "Benjamin Eberlei", "email": "kontakt@beberlei.de"}, - {"name": "Jonathan Wage", "email": "jonwage@gmail.com"}, - {"name": "Johannes Schmitt", "email": "schmittjoh@gmail.com"} + { + "name": "Guilherme Blanco", + "email": "guilhermeblanco@gmail.com" + }, + { + "name": "Roman Borschel", + "email": "roman@code-factory.org" + }, + { + "name": "Benjamin Eberlei", + "email": "kontakt@beberlei.de" + }, + { + "name": "Jonathan Wage", + "email": "jonwage@gmail.com" + }, + { + "name": "Johannes Schmitt", + "email": "schmittjoh@gmail.com" + } ], + "homepage": "https://www.doctrine-project.org/projects/annotations.html", "require": { "php": "^7.1 || ^8.0", "ext-tokenizer": "*", @@ -26,14 +45,10 @@ "symfony/cache": "^4.4 || ^5.2", "vimeo/psalm": "^4.10" }, - "config": { - "sort-packages": true, - "allow-plugins": { - "dealerdirect/phpcodesniffer-composer-installer": true - } - }, "autoload": { - "psr-4": { "Doctrine\\Common\\Annotations\\": "lib/Doctrine/Common/Annotations" } + "psr-4": { + "Doctrine\\Common\\Annotations\\": "lib/Doctrine/Common/Annotations" + } }, "autoload-dev": { "psr-4": { @@ -44,5 +59,11 @@ "tests/Doctrine/Tests/Common/Annotations/Fixtures/functions.php", "tests/Doctrine/Tests/Common/Annotations/Fixtures/SingleClassLOC1000.php" ] + }, + "config": { + "allow-plugins": { + "dealerdirect/phpcodesniffer-composer-installer": true + }, + "sort-packages": true } } From 46720608c1f208d70599f186976502ec0c61da6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Sat, 2 Jul 2022 12:26:57 +0200 Subject: [PATCH 6/6] Update PHPStan --- composer.json | 2 +- phpstan.neon | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 88fbc51ec..b4dfbd390 100644 --- a/composer.json +++ b/composer.json @@ -40,7 +40,7 @@ "require-dev": { "doctrine/cache": "^1.11 || ^2.0", "doctrine/coding-standard": "^6.0 || ^8.1", - "phpstan/phpstan": "^0.12.20", + "phpstan/phpstan": "^1.4.10 || ^1.8.0", "phpunit/phpunit": "^7.5 || ^8.0 || ^9.1.5", "symfony/cache": "^4.4 || ^5.2", "vimeo/psalm": "^4.10" diff --git a/phpstan.neon b/phpstan.neon index 78663a129..4d2b2f1d3 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -3,7 +3,12 @@ parameters: paths: - lib - tests - excludes_analyse: + scanFiles: + - tests/Doctrine/Tests/Common/Annotations/Fixtures/GlobalNamespacesPerFileWithClassAsFirst.php + - tests/Doctrine/Tests/Common/Annotations/Fixtures/GlobalNamespacesPerFileWithClassAsLast.php + - tests/Doctrine/Tests/Common/Annotations/Fixtures/NonNamespacedClass.php + - tests/Doctrine/Tests/Common/Annotations/Ticket/DCOM58Entity.php + excludePaths: - tests/*/Fixtures/* - tests/Doctrine/Tests/Common/Annotations/ReservedKeywordsClasses.php - tests/Doctrine/Tests/Common/Annotations/Ticket/DCOM58Entity.php