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

Adapt for latest http-client #10

Merged
merged 14 commits into from
Mar 31, 2024
Merged

Adapt for latest http-client #10

merged 14 commits into from
Mar 31, 2024

Conversation

danog
Copy link
Sponsor Contributor

@danog danog commented Jul 28, 2023

No description provided.

@danog
Copy link
Sponsor Contributor Author

danog commented Nov 19, 2023

Ping @trowski @kelunik? :)

@nicolas-grekas
Copy link

Note after reviewing this PR: it would have been great to use PSR-17 instead of hardcoding against guzzle's PSR-7 implementation.

@kelunik
Copy link
Member

kelunik commented Apr 22, 2024

@nicolas-grekas PR welcome! A deeper review is anyway pending before tagging.

@trowski
Copy link
Member

trowski commented Apr 22, 2024

@nicolas-grekas Reviewing the overall API of this library is on my TODO list.

Does PsrAdapter + PsrHttpClient not use PSR-17 as you'd expect? I have yet to try actually using this library in a project, so there may be some gaps in usability that I didn't notice on my recent cleanup pass.

->followRedirects(0)
->build()
);
self::$psrAdapter ??= new PsrAdapter(new class implements RequestFactoryInterface {

Choose a reason for hiding this comment

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

this is the line that hardcodes guzzle's PSR-7

}
/** @psalm-suppress PossiblyNullReference Initialized in the constructor */
$request = self::$psrAdapter->fromPsrRequest($request);
if (isset($options[RequestOptions::TIMEOUT])) {

Choose a reason for hiding this comment

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

also this: this is not portable

@nicolas-grekas
Copy link

But maybe I misunderstood and this is just about having a guzzle adapter instead of a more generic PSR-foo adapter.

@trowski
Copy link
Member

trowski commented Apr 25, 2024

@nicolas-grekas Much of this PR was adding a Guzzle handler class, which maybe should have been done in a separate PR as it wasn't necessary for compatibility with amphp/http-client v5.x.

This library provides PsrHttpClient, which is a PSR-18 client using an instance of PsrAdaptor which uses PSR-17 factories to translate between PSR-7 requests/responses to Amp requests/responses.

examples/psr-with-amp.php

Is there something in the implementation I'm missing which makes this non-portable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants