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

Should PsrAdapter be splitted or not? #2

Open
remorhaz opened this issue Jun 17, 2020 · 5 comments
Open

Should PsrAdapter be splitted or not? #2

remorhaz opened this issue Jun 17, 2020 · 5 comments

Comments

@remorhaz
Copy link
Contributor

As for now, PsrAdapter class provides full set of methods to convert both Amp and PSR-7 requests and responses in both directions. There is an opinion that it could be split apart.

I think that most frequent use cases will consist of the following (maybe partially):

  • Make call to PSR-7-compliant handler from Amp-compliant handler (this case will require toPsrRequest() and fromPsrResponse() methods):
    • convert Amp request it into PSR-7 request;
    • pass it to some code and get the PSR-7 response back;
    • convert it into Amp response and pass it further.
  • Make call to Amp-compliant handler from PSR-7-compliant handler (this case will require fromPsrRequest() and toPsrResponse() methods):
    • Convert PSR-7 request into Amp request;
    • pass it to some code and get the Amp response back;
    • convert it into PSR-7 response and pass it further.

But, on the other hand, there are cases where "alien" code just alters messages and returns them back (adds headers, for example); in this case, we recieve the same class of object back, like this:

  • convert Amp request into PSR-7 request;
  • pass it to some code and get modified PSR-7 request back;
  • convert PSR-7 request into Amp request.

This case requires toPsrRequest() and fromPsrRequest() methods, so I decided that splitting the adapter for the first group of cases can harm users that implement second group of cases and vice versa.

I've looked through @trowski 's implementation of server message bridge; it just implements MessageConverter interface that declares methods convertRequest() (that works like my toPsrRequest()) and convertResponse() (that works like my fromPsrResponse()). Thus, it allows only first use case.

@kelunik, what do you think about it?

@kelunik
Copy link
Member

kelunik commented Jun 17, 2020

Depending on the use case you need every combination, yes. Main reason for splitting would be, because not every case needs the factories.

I've added aed8333, which should simplify the usage.

@kelunik
Copy link
Member

kelunik commented Jun 17, 2020

@remorhaz
Copy link
Contributor Author

remorhaz commented Jun 30, 2020

Seems to be unused currently.

It was designed as an honest bridge between Amp's and PSR's way of sequential reading the body, but it was blocked by amphp/amp#311 and amphp/amp#313; so now it looks like an overkill.

I can either remove it (and rewrite it back when it becomes possible) or use it instead of PsrAdapter::copyToPsr() private method (this option may require less work to do after wait() issues get closed). I'm totally okay with both options; which one do you prefer?

@kelunik
Copy link
Member

kelunik commented Jun 30, 2020

I prefer not using it currently, but we could also allow to configure that behavior. What do you think about that?

@remorhaz
Copy link
Contributor Author

remorhaz commented Jul 1, 2020

I think there's nothing of value to configure yet. I'll remove it for now and will get back to it when nested loops run okay.

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

No branches or pull requests

2 participants