Suppress exception in case of empty curl response #79
Conversation
|
@robocoder if this build is successful, do you think you could please push out a 1.4.5? |
Add composer.lock to artifacts See instaclick/php-webdriver#79
|
Reviewing now... |
|
Yay thank you. :D I hope if I included good testing it would make the review decision easier. :) |
| @@ -103,6 +103,11 @@ public function setServiceClass($serviceName, $className) | |||
| $className = '\\' . $className; | |||
| } | |||
|
|
|||
| // Flush outdated service cache | |||
| if (isset($this->serviceClasses[$serviceName]) && $this->serviceClasses[$serviceName] !== $className) { | |||
| unset($this->services[$serviceName]); | |||
There was a problem hiding this comment.
What version of php requires this? Looks like a refcount hack.
There was a problem hiding this comment.
It's not PHP version specific; ServiceFactory::getService() would ignore any updated serviceClass if an outdated service exists for any prior value. This is necessary to force the new $className to trigger in any subsequent getService() call.
I would replace with setService() to allow a mock to be directly injected.
There was a problem hiding this comment.
Sorry, I understand what's going on now. I was skimming the code from my phone earlier.
| */ | ||
| public function testNonJsonResponse() | ||
| { | ||
| ServiceFactory::getInstance()->setServiceClass('service.curl', '\\Test\\WebDriver\\TestCurlService'); |
There was a problem hiding this comment.
I strongly feel this should be served by a mock.
There was a problem hiding this comment.
Yes also, but I was hesitant to make any strong changes to an unrelated API. ServiceFactory would be good if you could inject a mock directly.
| if ($result === null && json_last_error() != JSON_ERROR_NONE) { | ||
| throw WebDriverException::factory(WebDriverException::CURL_EXEC, 'Payload received from webdriver is not valid json: ' . substr($rawResult, 0, 1000)); | ||
| } | ||
| if ($result === null && json_last_error() != JSON_ERROR_NONE) { |
There was a problem hiding this comment.
I'm going to rewrite this since we want to push a fix tonight while maintaining our coding standards (eg no nested if).
There was a problem hiding this comment.
Oh, I apologise for the mis-standard! Your standard is a good one. :)
Fixes #78