Skip to content
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.

Sign up for GitHub

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

Suppress exception in case of empty curl response #79

Merged
merged 1 commit into from
Jun 30, 2017
Merged

Suppress exception in case of empty curl response #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 28 additions & 22 deletions lib/WebDriver/AbstractWebDriver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,38 +128,44 @@ protected function curl($requestMethod, $command, $parameters = null, $extraOpti
throw WebDriverException::factory(WebDriverException::CURL_EXEC, 'Webdriver http error: ' . $info['http_code'] . ', payload :' . substr($rawResult, 0, 1000));
}

$result = json_decode($rawResult, true);
$result = [];
$value = null;
if (!empty($rawResult)) {
$result = json_decode($rawResult, true);

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to rewrite this since we want to push a fix tonight while maintaining our coding standards (eg no nested if).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I apologise for the mis-standard! Your standard is a good one. :)

throw WebDriverException::factory(WebDriverException::CURL_EXEC,
'Payload received from webdriver is not valid json: ' . substr($rawResult, 0, 1000));
}

if (!is_array($result) || !array_key_exists('status', $result)) {
throw WebDriverException::factory(WebDriverException::CURL_EXEC, 'Payload received from webdriver is valid but unexpected json: ' . substr($rawResult, 0, 1000));
}
if (!is_array($result) || !array_key_exists('status', $result)) {
throw WebDriverException::factory(WebDriverException::CURL_EXEC,
'Payload received from webdriver is valid but unexpected json: ' . substr($rawResult, 0, 1000));
}

$value = null;
if (array_key_exists('value', $result)) {
$value = $result['value'];
}

if (array_key_exists('value', $result)) {
$value = $result['value'];
}

$message = null;
$message = null;

if (is_array($value) && array_key_exists('message', $value)) {
$message = $value['message'];
}
if (is_array($value) && array_key_exists('message', $value)) {
$message = $value['message'];
}

// if not success, throw exception
if ((int) $result['status'] !== 0) {
throw WebDriverException::factory($result['status'], $message);
// if not success, throw exception
if ((int) $result['status'] !== 0) {
throw WebDriverException::factory($result['status'], $message);
}
}

$sessionId = isset($result['sessionId'])
? $result['sessionId']
: (isset($value['webdriver.remote.sessionid'])
? $result['sessionId']
: (
isset($value['webdriver.remote.sessionid'])
? $value['webdriver.remote.sessionid']
: null);
: null
);

return array(
'value' => $value,
Expand Down
5 changes: 5 additions & 0 deletions lib/WebDriver/ServiceFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What version of php requires this? Looks like a refcount hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I understand what's going on now. I was skimming the code from my phone earlier.

}

$this->serviceClasses[$serviceName] = $className;
}
}
28 changes: 28 additions & 0 deletions test/Test/WebDriver/TestCurlService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php

namespace Test\WebDriver;

use WebDriver\Service\CurlServiceInterface;

/**
* HTTP 200 response for any request:
* - /session returns invalid json
* - any other request returns empty body
*/
class TestCurlService implements CurlServiceInterface
{
public function execute($requestMethod, $url, $parameters = null, $extraOptions = array())
{
$info = array(
'url' => $url,
'request_method' => $requestMethod,
'http_code' => 200,
);
if (preg_match('#.*session$#', $url)) {
$result = 'some invalid json';
} else {
$result = '';
}
return array($result, $info);
}
}
22 changes: 22 additions & 0 deletions test/Test/WebDriver/WebDriverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

namespace Test\WebDriver;

use WebDriver\ServiceFactory;
use WebDriver\WebDriver;

/**
Expand All @@ -42,6 +43,8 @@ class WebDriverTest extends \PHPUnit_Framework_TestCase
*/
protected function setUp()
{
require_once __DIR__ . '/TestCurlService.php';

if ($url = getenv('ROOT_URL')) {
$this->testDocumentRootUrl = $url;
}
Expand All @@ -59,6 +62,7 @@ protected function setUp()
*/
protected function tearDown()
{
ServiceFactory::getInstance()->setServiceClass('service.curl', '\\WebDriver\\Service\\CurlService');
if ($this->session) {
$this->session->close();
}
Expand Down Expand Up @@ -221,4 +225,22 @@ public function testSeleniumNoResponse()

$this->assertEquals(null, $out);
}

/**
* Assert that empty response does not trigger exception, but invalid JSON does
*/
public function testNonJsonResponse()
{
ServiceFactory::getInstance()->setServiceClass('service.curl', '\\Test\\WebDriver\\TestCurlService');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly feel this should be served by a mock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$result = $this->driver->status();
$this->assertNull($result);

// Test /session should error
$this->setExpectedException(
'WebDriver\Exception\CurlExec',
'Payload received from webdriver is not valid json: some invalid json'
);
$result = $this->driver->session();
$this->assertNull($result);
}
}