Add dedicated with* options methods in favour over withOptions #155
Conversation
There was a problem hiding this comment.
@WyriHaximus Thank you for taking a stab at this! I like the direction this is heading and I think this is a good starting point to discuss the best API for these options. I've left a number of remarks below, any input is welcome 👍
src/Browser.php
Outdated
| return $browser; | ||
| } | ||
|
|
||
| public function withoutTimeout() |
There was a problem hiding this comment.
withTimeout() LGTM, but I'm unsure about withoutTImeout().
This currently sets the underlying open to null which returns to the default timeout value (60) as per the documentation. It does not send requests "without" a timeout.
I'm undecided, do we need dedicated methods or should we accept different values like the underlying option?
There was a problem hiding this comment.
Only added it because you mentioned in our Gitter communication. Just dropped it in the latest push.
| return $browser; | ||
| } | ||
|
|
||
| public function withFollowRedirects($followRedirects) |
There was a problem hiding this comment.
withFollowRedirects() LGTM, but not sure about withMaxRedirects().
Perhaps it makes sense to merge both into a single method and trigger this logic based on the value given (bool|int)?
There was a problem hiding this comment.
Doesn't feel right to do that tbh. They are both closely related but distinctly different and I prefer less magic over more magic
There was a problem hiding this comment.
I'm undecided. Can you give a use case to explicitly control both parameters individually?
It's currently my understanding this could be better covered by a single method accepting different parameter types instead.
There was a problem hiding this comment.
What about dropping withFollowRedirects altogether, both here and deeper inside the package? withMaxRedirects already implies we'll follow redirects, setting that to 0 will be the same as withFollowRedirects(false) anyway.
There was a problem hiding this comment.
I guess we need some better documentation :-)
They're somewhat similar, but they actually have distinct behavior: setting to followRedirects=false means we do not follow redirects, i.e. the request may be fulfilled with a response with status code 3xx (and maxRedirects has no effect here). Setting maxRedirects=0 means we do not accept redirects, i.e. the request will be rejected when a redirect is encountered (only if followRedirects=true is set).
I agree that this behavior is confusing, hence my suggestion to merge both into a single method 👍
There was a problem hiding this comment.
To be honest that would suggest they should be two different methods rather than combining them into one to me ;).
src/Browser.php
Outdated
|
|
||
| public function withTimeout($timeout) | ||
| { | ||
| $browser = clone $this; |
There was a problem hiding this comment.
No need to clone here, you can rely on the withOptions().
There was a problem hiding this comment.
Feels a bit meh given you want to remove it, but it works for now and keeps the code simpler
src/Browser.php
Outdated
| return $browser; | ||
| } | ||
|
|
||
| public function withStreaming($streaming) |
There was a problem hiding this comment.
Makes sense to have this method from a consistency perspective, but I'd rather get rid of the underlying option. I'm currently working on a dedicated PR to add a new request() and requestStreaming() method which I believe to be a cleaner solution to this 👍
There was a problem hiding this comment.
As discussed on Gitter I'll be dropping this assuming your PR doing to refactor gets merged first keeping everything working
There was a problem hiding this comment.
Removed via #170, currently taking a look at rebasing your work, adding documentation and improving API consistency 👍
There was a problem hiding this comment.
@clue go for it 👍
97378c0 to
3d65b77
Compare
| return $browser; | ||
| } | ||
|
|
||
| public function withFollowRedirects($followRedirects) |
There was a problem hiding this comment.
I'm undecided. Can you give a use case to explicitly control both parameters individually?
It's currently my understanding this could be better covered by a single method accepting different parameter types instead.
| @@ -335,4 +337,32 @@ public function withOptions(array $options) | |||
|
|
|||
| return $browser; | |||
| } | |||
|
|
|||
| public function withTimeout($timeout) | |||
There was a problem hiding this comment.
Can you add docblocks with parameter type hints? In particular, should we keep the negative timeout and the null timeout value? See also other methods.
There was a problem hiding this comment.
Don't think so to be honest, only allowing int in the type hint
| )); | ||
| } | ||
|
|
||
| public function withObeySuccessCode($obeySuccessCode) |
There was a problem hiding this comment.
Docblock please? This would probably accept a bool, does withObeySuccessCode(false) make sense semantically?
There was a problem hiding this comment.
To be fair, does that feature make sense it all? Should this HTTP client care about the response that much that it turns anything about 400? In my opinion, it's like cache, any result from the cache engine is valid, and only communication errors between the cache and the adapter should yield exceptions.
There was a problem hiding this comment.
I think you're raising a valid point that's worth discussing, but I see this is still supported in other projects like https://www.php.net/manual/de/context.http.php#context.http.ignore-errors and http://docs.guzzlephp.org/en/stable/request-options.html#http-errors.
I would suggest adding this as part of this PR so we can deprecate the withOptions() method and retain all functionality. If you feel it's worth it, we can still discuss this in a separate ticket for a possible v3 👍
There was a problem hiding this comment.
Wasn't intending on removing it in this PR, already added the docblock for it, but I do feel it's worth removing from v3.
There was a problem hiding this comment.
Keeping it makes sense, since it allows cleaner code when using the library and the handling code for all HTTP response code is the same. This would allow to handle only "real errors" in a promise rejection handler, while being able to handle completed HTTP requests (regardless of their status code) in one handler, without boilerplate code.
3d65b77 to
ddbd4be
Compare
ddbd4be to
bdecaa6
Compare
|
@clue updated it to the latest comments. |
Implements / Closes #154