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 exception docblock #151

Merged
merged 2 commits into from Mar 5, 2019
Merged

Remove exception docblock #151

merged 2 commits into from Mar 5, 2019

Conversation

gmponos
Copy link
Contributor

@gmponos gmponos commented Jan 21, 2019

I think according to PSR this exception should not be thrown... :/

Remove exception docblock
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

correct. looking at the psr-18 ClientInterface that we extend here, i think we should actually drop all code in here and make this

/**
 * {@inheritdoc}
 * 
 * Provide the Httplug HttpClient interface for BC. You should typehint Psr\Http\Client\ClientInterface in new code
 */
interface HttpClient extends ClientInterface
{
}

the method definition is a copy of the psr method and therefore does nothing. do you want to adjust this accordingly?

@gmponos
Copy link
Contributor Author

gmponos commented Jan 21, 2019

do you want to adjust this accordingly?

Done.. check again :)

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

@Nyholm am i missing something or is this what the interface should look like now?

@Nyholm
Copy link
Member

Nyholm commented Jan 21, 2019

Nope, We still throw HTTPlug exceptions. The exceptions however extends PSR18.

Im 👎

@gmponos
Copy link
Contributor Author

gmponos commented Jan 21, 2019

Hmm.. I missed that.. @Nyholm is correct... So shall I revert here before @dbu comment...

are you all good with this?

@Nyholm
Copy link
Member

Nyholm commented Jan 21, 2019

To remove \Exception and update the description on the other line makes sense, yes.

@dbu
Copy link
Contributor

dbu commented Jan 22, 2019

We still throw HTTPlug exceptions. The exceptions however extends PSR18.

is there any value in explicitly stating that, however? imho people should be using the PSR-18 exception interfaces, not the ones from the HTTPlug BC layer. as the exceptions extend the PSR-18 interface, the phpdoc of the base interface is not wrong.

@Nyholm
Copy link
Member

Nyholm commented Jan 25, 2019

If you are using a HTTPlug 2.0 client you may catch both HTTPlug exceptions and PSR18 exceptions.

imho people should be using the PSR-18 exception interfaces
Sure, and they may.

@dbu
Copy link
Contributor

dbu commented Jan 25, 2019

@Nyholm so you would say we should add the method signature again to add the phpdoc for the httplug exceptions?

@joelwurtz
Copy link
Member

If you are using a HTTPlug 2.0 client you may catch both HTTPlug exceptions and PSR18 exceptions.

I don't agree with that, catching an HTTPlug is now a detail of implementation in 2.0, and we should not spec this, as this will allow people to correctly depend on PSR 18 and completly remove our interface latter.

@dbu
Copy link
Contributor

dbu commented Feb 23, 2019

@Nyholm are you okay to merge with this or do you disagree with the last comments from joel and me?

@dbu dbu merged commit d7f1b18 into php-http:master Mar 5, 2019
@gmponos gmponos deleted the patch-1 branch March 5, 2019 07:41
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.

None yet

4 participants