search

ruflin / Elastica

Elastica is a PHP client for elasticsearch
http://elastica.io/
MIT License
2.26k stars 736 forks source link

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

Closed VincentLanglet closed 1 year ago

VincentLanglet commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

ruflin commented 1 year ago

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

VincentLanglet commented 1 year ago

Thanks for the review @ruflin, changes applied

ruflin commented 1 year ago

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

VincentLanglet commented 1 year ago

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

Done in https://github.com/ruflin/Elastica/pull/2153