Remove PSR-7 dependency

[sagikazarmark]

Although this is a BC breaking change in an RC state, I think it is not a big problem.

The idea behind separating promise was that at some point we deprecate it and rely on a PSR. That PSR will unlikely rely on PSR7, so I would say get rid of PSR-7 dependency now.

[ddeboer]

👍

[dbu]

i think this makes sense, yes.

but you missed that the Promise interface has @return ResponseInterface (and a use statement)

[sagikazarmark]

Oops, good catch @dbu

[joelwurtz]

Don't mind to have this but we must add sanity check everywhere we use the result or error from a promise to ensure that we have a ResponseInterface or Exception (plugins / client common for the most part)

[sagikazarmark]

Well, at this point we either drop our plan to replace Promise with a PSR or get used to this.

[joelwurtz]

My mind was more, when the Promise PSR is out, have some sort of decorator or a callback which does this check (for type of result / failure) and ensure that existings plugin / implementation are consistent, that's why i didn't make something really generic.

[sagikazarmark]

decorator is probably not a good idea, because Promises are immutable and created internally, so you would have to create a huge object graph to decorate all promises.

What kind of callback do you think?

We should probably take a look at how guzzle does this.

[joelwurtz]

Like adding a then callable which throws an Http\Client\TransferException if value or exception is not acceptable

[joelwurtz]

The other solution is we can say we don't care and it's an user / implementation problem to have consistence as anyway it will failed hard if it's not correct.

[sagikazarmark]

Is this a blocker for release?

[joelwurtz]

If we merge this and say we don't care -> not a blocker

If we don't merge this and approve it later by adding checks everywhere -> also not a blocker (as there is just more features but no BC break)

if we merge this and need to add checks -> not a blocker for promise / httplug, but a blocker for plugins / common-client and maybe extra stuffs

[dbu]

i would say merge and dont care. before, it was only a phpdoc, not any hard check. except for FullfilledPromise, but that is an edge case.

[sagikazarmark]

Agree with @dbu

[sagikazarmark]

Ready to tag promise then?

[joelwurtz]

i would say merge and dont care. before, it was only a phpdoc, not any hard check. except for FullfilledPromise, but that is an edge case.

Don't agree, before user was aware that we needed this type to work, so if it's incorrect it's his problem.

Now we say we accept everything but in the reality it will fail

[sagikazarmark]

No, we say in theory we accept everything, but inside http client, we accept ResponseInterface.

Again, Promises are created internally, not injected anywhere.

[joelwurtz]

If someone create it's own HttpAsyncClient this will not be created internally and the same for someone creating a custom plugin whichs depends on the PromiseInterface it may break other plugins.

[sagikazarmark]

By internally, I mean HTTP context. As @dbu said this limitation was phpdoc only (except the FullfilledPromise, but that's another story), so in theory, nothing kept you from returning a non-Response result.

[joelwurtz]

Agreed, but we must say somewhere in HttpAsyncClient and Plugins documentation that the usage of Promise here require result to be a ResponseInterface or error to be a Http\Client\Exception so user is aware of this.

I don't want to add checks i just want to be sure that our documentation or php docs clearly show our intent on how we use promise for HttpAsyncClient

[sagikazarmark]

Of course, "don't care" didn't mean "don't document" 😄

[sagikazarmark]

php-http/documentation#79