Add PSR-7 request adapter (#266) #270

remorhaz · 2020-05-11T17:09:44Z

This PR shows partial job, just to sync expectations. It only implements converting requests between Amp and PSR-7 and vice versa.

nicolas-grekas · 2020-05-11T17:37:38Z

composer.json

@@ -34,7 +34,8 @@
        "amphp/socket": "^1",
        "amphp/sync": "^1.3",
        "league/uri": "^6",
-        "psr/http-message": "^1"
+        "psr/http-message": "^1",
+        "laminas/laminas-diactoros": "^2.3"


Please move this back to require-dev, it should remain an optional feature.

Now that's impossible because adapter uses Request object from Laminas library. I've tried to find League implementation but they use Laminas in their examples; so Laminas is not optional anymore.

We can avoid requiring concrete PSR-7 implementation, though, if we accept RequestInterface in corresponding method (and then call with* methods). But the problem is body - it's mutable in PSR-7.

None of the added classes are mandatory to use the http-client. If one doesn't use any of them (I don't), one shouldn't be forced to install a dead-code dependency. Moving it back to require-dev fixes the issue.

About diactoros, could it be possible to be compatible with nyholm/psr7? That's the lib we should recommend IMHO (I don't know about the policy of the amp project here, I'm just talking for Symfony-related apps.)

Wait, I'll try one idea...

I've got rid of requiring Laminas with injecting PSR-7 RequestFactoryInterface. Maybe it would be more convenient to inject it in constructor - but from* methods will never use it, so in this case we'll have to split adapter into independent from* and to* parts.

On the other hand, most use cases will require using either "fromPsrRequest/toPsrResponse" or "toPsrRequest/fromPsrResponse" pair of methods. One of them will require RequestFactoryInterface, so I'll better inject it in constructor.

nicolas-grekas · 2020-05-11T19:48:26Z

composer.json

@@ -34,7 +34,8 @@
        "amphp/socket": "^1",
        "amphp/sync": "^1.3",
        "league/uri": "^6",
-        "psr/http-message": "^1"
+        "psr/http-message": "^1",
+        "psr/http-factory": "^1"


now you can also remove this one as it's not a dependency at all!
(it's only a dependency of the external part that instantiates the factory)

It's a dependency as much as psr/http-message is a dependency for UriInterface.

Look at the code: UriInterface is part of the public signature of Request::getUri().

RequestFactoryInterface on the contrary is only part of the constructor of PsrAdapter. This means you can load the class definition of PsrAdapter without any issue. (And you can't for Request).

The thing that instantiates PsrAdapter must have psr/http-factory as a dependency to use the class, but the class and the package themself do not dependent on psr/http-factory.

I agree that psr/http-factory is much lesser evil than Laminas - it's just interfaces. In case it's unacceptable - the only escape is to extract adapters to separate library.

But it is not dependency of this package... This should be moved to require-dev as that's the place where it belongs.

Let's say we remove this line. What did we lose in the process? Did we break anything? What does this line provide in the first place?

@remorhaz: "conflict" is very much an option, and probably the way to go.

As most users won't use the PSR converter, I'm leaning towards a separate package, as breaking changes seem more likely than for the rest of the code in this package.

Let's say we remove this line. What did we lose in the process? Did we break anything?

Of course. If someone uses factory v1 in his code and then upgrades to factory v2 that breaks BC (by renaming factory method, for example), he gets his service broken without any chance of being preserved from it by Composer. That's how the dependency works; just ignoring the dependency doesn't remove it.

"Conflicting" with factory v2+ is pretty the same as requiring v1 - it will not allow to install factory v2 with our library, even if user doesn't use PSR adapters. It's just using wrong mechanism instead of right one, I think; it solves no problems. I hope @kelunik will decide to create a separate package - that's the best way to keep original client's dependencies untouched.

Software design, package principles here, should prepare us for events that are possible. Here the v1/v2 example is unrealistic IMHO when talking about psr/http-factory. It's not a good enough reason to force it as a dependency to everyone "just in case". Even adding a "conflict" looks like useless precaution to me.

Anyway, we don't have to agree on this as a separate package might provide a path forward for both our povs.

Thanks for the talk.

@nicolas-grekas In the case of psr/http-factory the BC break is indeed unlikely, as PSRs don't currently evolve and probably never will, because releasing a v2 on the same package would be a huge pain.

"conflict" seems to be the right choice currently for we're compatible with this API, but don't want an installation dependency.

Indeed, a separate package solves the "problem" entirely. :)

kelunik · 2020-05-11T19:39:18Z

src/Body/PsrStreamBody.php

@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);


We don't use strict_types in any @amphp project, so we shouldn't use it here.

kelunik · 2020-05-11T19:47:09Z

src/Internal/PsrRequestStream.php

+
+    private function readFromStream(): string
+    {
+        $data = wait(timeout($this->getOpenStream()->read(), $this->timeout));


We can't use wait, because it has issues with nesting, see amphp/amp#311 and amphp/amp#313. This means we need to buffer the entire body for these.

Hmm, that's really bad: the body can be very big in some cases. Is there any workaround that will allow as to read the body by parts? Maybe some non-promise generator?

The only one would be writing huge responses to files, I guess.

Is there any chance of solving these issues in the future? In this case I can buffer the entire body for now, but keep the infrastructure to restore current behavior in future.

Is there any (relatively) simple way to reproduce the issues using my code? Maybe I'll think something out.

Yes, it's very easy to reproduce, just call that code inside Loop::run() and it'll stop the outer Loop::run(), too.

enumag · 2020-05-11T20:11:10Z

src/Body/PsrStreamBody.php

+
+final class PsrStreamBody implements RequestBody
+{
+


Suggested change

remorhaz · 2020-05-12T14:19:39Z

I've made some changes:

Added response adapters;
Adapter returns promises instead of "direct" objects;
Message bodies are buffered entirely before resolving the message promise (escaping wait() issues).
Minor codestyle fixes, tests added, etc.

trowski · 2020-05-12T14:29:28Z

I agree with @kelunik, this really should be a separate package.

See the PSR adaptor I've been working on for http-server: https://github.com/trowski/psr-http-bridge

remorhaz · 2020-05-12T14:41:09Z

So, should I move my code to separate repository?

kelunik · 2020-05-12T19:48:27Z

@remorhaz Do you want to maintain it yourself or should I create a new repository in the @amphp namespace? I'd be fine with both.

remorhaz · 2020-05-12T21:28:20Z

I guess it's better to keep it in amphp space.

remorhaz · 2020-05-17T11:18:37Z

@kelunik, so what about a new repository?

kelunik · 2020-05-17T19:05:32Z

@remorhaz I've just created https://github.com/amphp/http-client-psr7, so you can PR against that. :-)

enumag · 2020-05-25T07:03:39Z

ping @remorhaz

remorhaz · 2020-05-25T09:23:25Z

Yep, @enumag, I remember; just havin' some hot time on main job. I'll make a PR in a day or two.

Co-authored-by: Niklas Keller <me@kelunik.com> Co-authored-by: Jáchym Toušek <enumag@gmail.com>

kelunik · 2020-06-16T20:31:40Z

Closing, because of amphp/http-client-psr7@487af25.

Co-authored-by: Niklas Keller <me@kelunik.com> Co-authored-by: Jáchym Toušek <enumag@gmail.com>

nicolas-grekas reviewed May 11, 2020

View reviewed changes

Add PSR-7 request adapter (#266)

49491c7

kelunik reviewed May 11, 2020

View reviewed changes

enumag reviewed May 11, 2020

View reviewed changes

src/Body/PsrStreamBody.php Outdated

final class PsrStreamBody implements RequestBody

{

Copy link

Contributor

enumag May 11, 2020

Choose a reason for hiding this comment

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

Suggested change

Added PSR-7 response adapters (#266)

99c1855

Moved psr/http-factory to conflict section

9b2f4b7

remorhaz mentioned this pull request May 30, 2020

Code moved from amp/http-client PR #270 amphp/http-client-psr7#1

Closed

kelunik added a commit to amphp/http-client-psr7 that referenced this pull request Jun 16, 2020

Add adapters based on amphp/http-client#270

487af25

Co-authored-by: Niklas Keller <me@kelunik.com> Co-authored-by: Jáchym Toušek <enumag@gmail.com>

kelunik closed this Jun 16, 2020

trowski pushed a commit to amphp/http-client-guzzle-adapter that referenced this pull request Aug 31, 2024

Add adapters based on amphp/http-client#270

0c72889

Co-authored-by: Niklas Keller <me@kelunik.com> Co-authored-by: Jáchym Toušek <enumag@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PSR-7 request adapter (#266) #270

Add PSR-7 request adapter (#266) #270

remorhaz commented May 11, 2020

nicolas-grekas May 11, 2020

remorhaz May 11, 2020

remorhaz May 11, 2020

nicolas-grekas May 11, 2020 •

edited

Loading

remorhaz May 11, 2020

remorhaz May 11, 2020

remorhaz May 11, 2020

nicolas-grekas May 11, 2020 •

edited

Loading

kelunik May 11, 2020

nicolas-grekas May 11, 2020 •

edited

Loading

remorhaz May 11, 2020

nicolas-grekas May 11, 2020 •

edited

Loading

nicolas-grekas May 11, 2020

kelunik May 12, 2020

remorhaz May 12, 2020

nicolas-grekas May 12, 2020 •

edited

Loading

kelunik May 12, 2020

kelunik May 11, 2020

kelunik May 11, 2020

remorhaz May 11, 2020

kelunik May 11, 2020

remorhaz May 11, 2020

kelunik May 12, 2020

enumag May 11, 2020

remorhaz commented May 12, 2020

trowski commented May 12, 2020

remorhaz commented May 12, 2020

kelunik commented May 12, 2020

remorhaz commented May 12, 2020

remorhaz commented May 17, 2020

kelunik commented May 17, 2020

enumag commented May 25, 2020

remorhaz commented May 25, 2020

kelunik commented Jun 16, 2020

		@@ -0,0 +1,41 @@
		<?php

		declare(strict_types=1);

Add PSR-7 request adapter (#266) #270

Add PSR-7 request adapter (#266) #270

Conversation

remorhaz commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remorhaz commented May 12, 2020

trowski commented May 12, 2020

remorhaz commented May 12, 2020

kelunik commented May 12, 2020

remorhaz commented May 12, 2020

remorhaz commented May 17, 2020

kelunik commented May 17, 2020

enumag commented May 25, 2020

remorhaz commented May 25, 2020

kelunik commented Jun 16, 2020

nicolas-grekas May 11, 2020 •

edited

Loading

nicolas-grekas May 11, 2020 •

edited

Loading

nicolas-grekas May 11, 2020 •

edited

Loading

nicolas-grekas May 11, 2020 •

edited

Loading

nicolas-grekas May 12, 2020 •

edited

Loading