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

Remove irrelevant @throws annotations #5777

Merged
merged 2 commits into from
Oct 19, 2022

Conversation

morozov
Copy link
Member

@morozov morozov commented Oct 19, 2022

Q A
Type improvement

At the time of its introduction in #4746, Comparator::columnsEqual() was annotated with @throws Exception because the following code flow could result in one: Comparator::columnsEqual()AbstractPlatform::columnsEqual()AbstractPlatform::getColumnDeclarationSQL()AbstractPlatform::getInlineColumnCommentSQL(). The last method in the chain would throw an exception if the platform didn't support inline column comments.

As of #5727, the NotSupported exception is declared as unchecked (extends LogicException), so this code path no longer throws a checked exception. In fact, this makes the Comparator API free of checked exceptions.

Additionally, declaring NoColumnsSpecifiedForTable as unchecked allows removing most of the @throws annotations from the Platform API. The remaining @throws is related to the type registry and is out of the scope of this change.

Note that the recent versions of PHPStan support analyzing the @throws (see Bring your exceptions under control with @throws). We may want to use them on CI in the future.

@morozov morozov added this to the 4.0.0 milestone Oct 19, 2022
@morozov morozov requested review from greg0ire and derrabus October 19, 2022 01:18
@morozov morozov merged commit 2e5aa74 into doctrine:4.0.x Oct 19, 2022
@morozov morozov deleted the irrelevant-throws-annotations branch October 19, 2022 14:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants