-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: add OutgoingRequestInterface #6698
Conversation
5589339
to
d6150ff
Compare
I'm not sure if you implemented it this way because of the restrictions in our current HTTP layer, but the inheritance path for requests is confusing. What is restricting us from making Request a common base for both Incoming and Outgoing, and those not being in the same path? |
PSR-7: MessageInterface > RequestInterface (outgoing) > ServerRequestInterface (incomming)
CI4's Request is incomming request. |
Wow you are totally right! TIL an incoming request is actually an outgoing request with some extra methods. So next question... why the new class? If we already have Message > Request > IncomingRequest, what is stopping us from making Request into the equivalent of OutgoingRequest? (sorry if this is obvious, it is very hard to flip between interfaces on mobile) |
The reason for adding the new class is to make as few changes as possible.
Do you mean like this? |
I'm with you now, took me a little bit. The structure and names are confusing, but that's not your fault. I would like to look at this one more time on desktop, but generally I think it is a good direction and you should feel free to proceed with the docs. |
2404804
to
eda2615
Compare
Added changelog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're there! I would like one other set of eyes here. @paulbalandan or @samsonasik or @iRedds ?
eda2615
to
d58f681
Compare
d58f681
to
583689c
Compare
Description
See #6573, #4356
OutgoingRequest
OutgoingRequestInterface
CURLRequest
extendsOutgoingRequest
Request
extendsOutgoingRequest
RequestInterface
extendsOutgoingRequestInterface
http://www.plantuml.com/plantuml/uml/ZP1TQWCX58NVNSKX2-XJ5n38IuMKbf0kGEoTGUhFLAzGADrxnQOIbCpaRPXpplUf-yGgSdPMB4f_g9cmEyZ77Ru501ZF52Ub2S-KKadb_uykViay1-Fd4trcIjnge2yc_vwszhTsDy6Y0hHLgR5Xt6B9aUTHrze3iPb6oYWVQJsbcrpJrguWlGvkE5JRrAOFJE2m84nzl-Q0yZ2N0F52caB4q_dy-b0x1Levr-x3KTwhRTKjOock2DzEnurtwyrrCwZ3twYfE-MFs9VlxEml
Checklist: