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

Add missing @throws annotation to Client::request and related methods #2152

Merged
merged 6 commits into from
Mar 24, 2023
Merged

Add missing @throws annotation to Client::request and related methods #2152

merged 6 commits into from
Mar 24, 2023

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Mar 22, 2023

When using PHPStan, it can report exceptions which are not caught if and only if the library is correctly using the @throws annotations. Same for PHPStorm.

The Client::request method is throwing ConnectionException, ClientException and ResponseException, so it should be propagated to the phpdoc of method using Client::request in order to help the developper to catch such exceptions.

@VincentLanglet VincentLanglet marked this pull request as ready for review March 22, 2023 10:46
@VincentLanglet
Copy link
Contributor Author

@ruflin Hi, I opened the PR on the 7.x branch because this is the currently stable version. Not sure if it should be done only on 8.x or if it should be merged on 7.x and then cherry-pick/merged on 8.x.

@ruflin
Copy link
Owner

ruflin commented Mar 23, 2023

I prefer to have PR's against 8.x and then backported to 7.x. The reason is that this ensures no feature is missing in 8.x. Lets say this PR goes into 8.x and for whatever reason gets stuck in 7.x, users would not have a degraded experience going to 8.x, the other way around they do.

I assume not really much code has changed around this in 8.x so I hope you can just retarget it to 8.x?

Could you also add a changelog entry?

@VincentLanglet VincentLanglet changed the base branch from 7.x to 8.x March 23, 2023 12:44
@VincentLanglet
Copy link
Contributor Author

Thanks for your quick answer @ruflin, it targets 8.x now

@ruflin
Copy link
Owner

ruflin commented Mar 24, 2023

@VincentLanglet Change LGTM. Can you add a changelog entry?

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Ruflin <[email protected]>
@VincentLanglet
Copy link
Contributor Author

Thanks for the review @ruflin, changes applied

@ruflin ruflin changed the title Add missing @throws annotation Add missing @throws annotation to Client::request and related methods Mar 24, 2023
@ruflin ruflin merged commit 87a8bf5 into ruflin:8.x Mar 24, 2023
@ruflin
Copy link
Owner

ruflin commented Mar 24, 2023

@VincentLanglet Thanks for the contribution. Please open a backport to 7.x so we have it there too.

@VincentLanglet
Copy link
Contributor Author

@VincentLanglet Thanks for the contribution. Please open a backport to 7.x so we have it there too.

Done in #2153

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.

2 participants