Add specific http promise

[joelwurtz]

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	php-http/client-common#34
Documentation
License	MIT

This is not something that is meant to merge, it's to offer a point of view on this ticket php-http/client-common#34 we should accept this or not based on the final decision.

[sagikazarmark]

Looking at this PR: it actually doesn't solve the problem, because you cannot make sure that these promises are returned. It only provides type safety WHEN they are used. This means that you probably can't assume that the rejection callback will receive an Http\Client\Exception instance. Am I right?

So in this case php-http/client-common#35 is still valid?

[joelwurtz]

Yes we Will need to replace used promise in emulated client by this one

[sagikazarmark]

That's okay, but even then, we cannot make sure that clients will actually use that promise, so we have to rely on the Promise interface spec, which accepts any exceptions, meaning the rejection callbacks should too. Am I wrong?

[joelwurtz]

I see your point, however this behavior is already clearly specify in the dockblock :

@return Promise Resolves a PSR-7 Response or fails with an Http\Client\Exception.

We can add a HttpPromise (which extends Promise) to enforce this at code level, for me it's not a BC break since we have the same behavior as previously but exprimed at the code level and not on the docblock level (so this like a new feature and can be done in a 1.1.0 version IMO).

[joelwurtz]

I add the interface with change to the async client

[sagikazarmark]

This is not possible. Think about PHP 7, this line should still be public function then(callable $onFulfilled = null, callable $onRejected = null): Promise;

[joelwurtz]

So this interface is useless ? should we remove it ?

[dbu]

imho we need to solve this by documentation and avoid people using the promise stufff in HTTPlug for other things than HTTP. a promise should be allowed to throw any exception, but further promises in the chain should not be bothered with non-http exceptions.

[joelwurtz]

a promise should be allowed to throw any exception, but further promises in the chain should not be bothered with non-http exceptions.

I agree with that, but then RejectedPromise and FulfilledPromise does not respect this principle, that's why we need HttpRejectedPromise and HttpFulfilledPromise

I remove the HttpPromise interface to not having to think about a BC Break (we can do that later) and i also add tests.

[joelwurtz]

What should we do about that ?

[sagikazarmark]

This line causes all our problems IMO: https://github.com/php-http/httplug/pull/116/files#diff-0604cd13cbcbbca1c6eb9241817bb50cR34
(In the original promises). Non-HTTP exceptions can be thrown in the plugin chain, but the line above will make sure that they are not caught.

I would say let's merge this and use it in client-common.

Also, make sure we document this properly (use the above promises and/or don't throw non-http exceptions?)

My concern regarding this change: version. We will have to increase version number. How do we handle this in provides configuration in clients? Increase version number too? Or just leave 1.0 everywhere until 2.0?

[joelwurtz]

We will have to increase version number. How do we handle this in provides configuration in clients? Increase version number too? Or just leave 1.0 everywhere until 2.0?

The contract does not change so we don't need to upgrade the version of the virtual package, library like common-client would just have to require httplug with ^1.1.0

[sagikazarmark]

Okay, I vote for merging.

WDYT @joelwurtz @Nyholm @dbu ?

[Nyholm]

I've tested this and it works. Im 👍

Im trying to figure out the implications of this. It requires updates on

• PluginClient
• CachePlugin
• HttpAsyncClientEmulator
• Doc block in Plugin, right?
• Integration tests?
• Doc block in HttpAsyncClient
• In most of our test code

And our Async clients:

• cUrl client
• Guzzle6 adapter
• React adapter

Question: What would the new version of HTTPlug be?

[joelwurtz]

What would the new version of HTTPlug be?

1.1.0 It's a new feature but doesn't break BC

Yes we need to update all common-client but it's for a good reason IMO, so not so a problem

[Nyholm]

Good. =)

Yes we need to update all common-client but it's for a good reason IMO, so not so a problem

I agree.

This PR is a no brainer. 👍

[joelwurtz]

And i checked all adapters, their are good and respect the contract as far as i have seen.

[Nyholm]

Yeah, So did I. I believe there is path versions for all packages except for php-http/httplug.

[Nyholm]

I voted for merge, so did Mark and Joel. David chose to abstain. (See slack).

I will merge this PR. Thank you @joelwurtz!

[sagikazarmark]

Does this invalidate php-http/client-common#34?

[Nyholm]

Yes, that ticket is invalid. But we still need to be clear with the plugins. Ie increase the docs.

[sagikazarmark]

Okay, so based on @joelwurtz's comment, we only have to merge this, tag this and update the version number in client common (this is only a problem with plugins, right?)

[sagikazarmark]

Clients themselves are okay, right?

[Nyholm]

See my previous comment: #116 (comment)

I'll make sure to do some PRs tomorrow if nobody beats me to it

[joelwurtz]

In php-http/client-common#42

• PluginClient
• HttpAsyncClientEmulator
• Doc block in Plugin
• Most of our test code

All async client are already good

Need to do :

• CachePlugin
• Integration tests