Add http client router feature #13
Conversation
| * | ||
| * @author Joel Wurtz <[email protected]> | ||
| */ | ||
| interface RequestMatcher |
There was a problem hiding this comment.
did we not already add a RequestMatcher somewhere?
There was a problem hiding this comment.
We did, message. See #14 (comment)
sagikazarmark
force-pushed
the
feature/http-client-router
branch
from
c05df7b to
f55b3dd
Compare
| */ | ||
| public function addClient($client, RequestMatcher $requestMatcher) | ||
| { | ||
| $this->clients[] = [ |
There was a problem hiding this comment.
Do you think we could find a better way to store these. Although it's an internal detail, it feels a little but weird.
There was a problem hiding this comment.
having a member client and a member matcher would make more sense.
re-reading this i am confused about this code. we have a bunch of matcher, but only one single client? if none of the matchers match, we throw an exception, if any matches we always use the same client. that is not what i would expect from a router. i would have expected this to be an array of matcher, client and if a matcher matches, that client is used...
There was a problem hiding this comment.
the code also indicates that this was originally the idea.
There was a problem hiding this comment.
oh sorry. i misread the code. it does what i would expect. i don't see how we could store these differently. its a list of pairs [matcher, client] - i would not know how to store that differently. you could try some spl hashmap that can use objects as key if that exists, but i think its quite straightforward.
There was a problem hiding this comment.
i would have expected this to be an array of matcher, client and if a matcher matches, that client is used...
I think this is what's happening right now. Isn't it?
That one client is only used as a decorator in order to correctly emulate a client if necessary.
|
Did some improvments. Anyone review? |
|
looks good to me. can we squash the commits a bit? also, according to the description we have no documentation yet and we should update the changelog. |
|
Will do. |
sagikazarmark
force-pushed
the
feature/http-client-router
branch
from
1ad7353 to
be5d829
Compare
sagikazarmark
force-pushed
the
feature/http-client-router
branch
from
be5d829 to
dacaf68
Compare
|
Changelog updated, doc PR ready. |
sagikazarmark
changed the title
What's in this PR?
Explained in #12
Checklist