Request cookie parser #50
Conversation
|
@mpscholten feel free to review and share your thoughts |
| * | ||
| * $userId = $this->cookieParameter('userId')->int()->defaultsTo(null) | ||
| * | ||
| * @param $name |
There was a problem hiding this comment.
Can we add a string typehint here?
| /** | ||
| * @return RequestParser | ||
| */ | ||
| public function createCookieParser(); |
There was a problem hiding this comment.
This might break existing classes implementing the interface. What do you think about creating a second interface CookieParserFactory with just the createCookieParser? Then the specific *RequestParserFactory implementations can just implements RequestParserFactory, CookieParserFactory
There was a problem hiding this comment.
Yep, true 👍 will change that
| } | ||
|
|
||
| protected function parameter($name) | ||
| protected function parameter($name, $cookie = false) |
There was a problem hiding this comment.
Can we split this into two functions (parameter and cookieParameter)? Otherwise the function is basically doing two things. Here's some interesting discussion on that topic in general: https://softwareengineering.stackexchange.com/questions/147977/is-it-wrong-to-use-a-boolean-parameter-to-determine-behavior
There was a problem hiding this comment.
I was actually wondering about it, good point, this discussion solves my conflict :-) good point
|
Hey @mpscholten, I'll pick it up from where we stopped in the coming days 🙂 thanks for the patience buddy |
No description provided.