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

Jump to bottom

Catch and process errors that occure during annotation instantiation #438

Merged
merged 1 commit into from Jun 20, 2022
Merged

Catch and process errors that occure during annotation instantiation #438

merged 1 commit into from Jun 20, 2022

Conversation

7ochem
Copy link
Contributor

@7ochem 7ochem commented May 13, 2022 •
edited

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.

Resolves: #437

stof
stof requested changes May 13, 2022
View reviewed changes
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

This should have tests

lib/Doctrine/Common/Annotations/DocParser.php Outdated Show resolved Hide resolved
lib/Doctrine/Common/Annotations/DocParser.php Outdated Show resolved Hide resolved
lib/Doctrine/Common/Annotations/DocParser.php Outdated Show resolved Hide resolved
@7ochem 7ochem mentioned this pull request May 13, 2022
Closed
@7ochem
Copy link
Contributor Author

7ochem commented May 13, 2022 •
edited

This should have tests

Thanks Christophe for the super fast feedback and valid points 🙏🏻 I'll work on it a bit more

@7ochem 7ochem force-pushed the fix-catch-and-process-instantiation-errors branch 2 times, most recently from 41bce60 to 1fb05af Compare May 17, 2022 10:24
@7ochem
Copy link
Contributor Author

7ochem commented May 17, 2022

I changed the code and made it catch all \Throwable when instantiating the annotation class. This meant that I had to update some existing tests that were testing for an \InvalidArgumentException thrown in the constructor of the annotation. The advantage is that these exceptions are now also provided with the context and target.

@7ochem 7ochem marked this pull request as ready for review May 17, 2022 10:31
@7ochem 7ochem requested a review from stof May 17, 2022 10:31
@7ochem
Catch and process errors that occure during annotation instantiation
065fad5 
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.
@7ochem 7ochem force-pushed the fix-catch-and-process-instantiation-errors branch from 1fb05af to 065fad5 Compare May 17, 2022 14:13
7ochem
7ochem commented May 17, 2022
View reviewed changes
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\(\)#'
Copy link
Contributor Author

@7ochem 7ochem May 17, 2022 •
edited

Choose a reason for hiding this comment

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

Here

annotations/tests/Doctrine/Tests/Common/Annotations/DocParserTest.php

Lines 1752 to 1756 in 065fad5

if (method_exists(get_parent_class($this), 'expectExceptionMessageMatches')) {
parent::expectExceptionMessageMatches($regularExpression);
} else {
parent::expectExceptionMessageRegExp($regularExpression);
}

Either one of those exists: expectExceptionMessageRegExp exists in PHPUnit 7, deprecated in 8 and removed in 9 (and thus PHPStan gives an error). expectExceptionMessageMatches exists in PHPUnit 8 and up

@7ochem 7ochem requested a review from stof May 17, 2022 14:21
@malarzm malarzm added this to the 1.13.3 milestone Jun 20, 2022
@malarzm malarzm merged commit 6c5e2a6 into doctrine:1.13.x Jun 20, 2022
@malarzm
Copy link
Member

malarzm commented Jun 20, 2022

Thanks @7ochem for the improvement and sorry for the long merge time!

@7ochem 7ochem deleted the fix-catch-and-process-instantiation-errors branch June 21, 2022 07:20
@7ochem
Copy link
Contributor Author

7ochem commented Jun 21, 2022

Thanks @7ochem for the improvement and sorry for the long merge time!

Thanks for merging! And no problem 👍🏻

@github-actions github-actions bot mentioned this pull request Jul 2, 2022
Merged
@franmomu franmomu mentioned this pull request Jul 17, 2022
Merged
gsteel added a commit to laminas/laminas-form that referenced this pull request Jul 25, 2022
@gsteel
Bumps minimum version of doctrine annotations to 1.13.3 to solve te…
8c7543c 
…st failures

Doctrine Annotations now converts deprecations to exceptions

ref: doctrine/annotations#438
Signed-off-by: George Steel <george@net-glue.co.uk>
@gsteel gsteel mentioned this pull request Jul 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malarzm malarzm malarzm approved these changes

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
1.13.3
Development

Successfully merging this pull request may close these issues.

Catch TypeError during annotation instantiation and throw a more meaningful AnnotationException
3 participants
@7ochem @malarzm @stof