diff --git a/CHANGELOG.md b/CHANGELOG.md index a8bbce5f..a954e862 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ Changelog ========= +## TBD + +### Enhancements + +* Add a default timeout & connect_timeout to Guzzle instances created by bugsnag-php. This does not apply if you are providing a custom Guzzle instance. + [#616](https://github.com/bugsnag/bugsnag-php/pull/616) + ## 3.24.0 (2020-10-27) This release changes how Bugsnag detects the error suppression operator in combination with the `errorReportingLevel` configuration option, for PHP 8 compatibility. Bugsnag's `errorReportingLevel` must now be a subset of `error_reporting` — i.e. every error level in `errorReportingLevel` must also be in `error_reporting` diff --git a/src/Client.php b/src/Client.php index c113eedc..5d7be7de 100644 --- a/src/Client.php +++ b/src/Client.php @@ -9,6 +9,7 @@ use Bugsnag\Callbacks\RequestMetaData; use Bugsnag\Callbacks\RequestSession; use Bugsnag\Callbacks\RequestUser; +use Bugsnag\Internal\GuzzleCompat; use Bugsnag\Middleware\BreadcrumbData; use Bugsnag\Middleware\CallbackBridge; use Bugsnag\Middleware\NotificationSkipper; @@ -73,6 +74,15 @@ class Client */ protected $sessionTracker; + /** + * Default HTTP timeout, in seconds. + * + * @internal + * + * @var float + */ + const DEFAULT_TIMEOUT_S = 15.0; + /** * Make a new client instance. * @@ -145,39 +155,35 @@ public function __construct( */ public static function makeGuzzle($base = null, array $options = []) { - $key = self::getGuzzleBaseUriOptionName(); - - $options[$key] = $base ?: Configuration::NOTIFY_ENDPOINT; - - if ($path = static::getCaBundlePath()) { - $options['verify'] = $path; - } + $options = self::resolveGuzzleOptions($base, $options); return new GuzzleHttp\Client($options); } /** - * Get the base URL/URI option name, which depends on the Guzzle version. + * @param string|null $base + * @param array $options * - * @return string + * @return array */ - private static function getGuzzleBaseUriOptionName() + private static function resolveGuzzleOptions($base, array $options) { - return method_exists(GuzzleHttp\ClientInterface::class, 'request') - ? 'base_uri' - : 'base_url'; - } + $key = GuzzleCompat::getBaseUriOptionName(); + $options[$key] = $base ?: Configuration::NOTIFY_ENDPOINT; - /** - * Get the base URL/URI, which depends on the Guzzle version. - * - * @return mixed - */ - private function getGuzzleBaseUri(GuzzleHttp\ClientInterface $guzzle) - { - return method_exists(GuzzleHttp\ClientInterface::class, 'getBaseUrl') - ? $guzzle->getBaseUrl() - : $guzzle->getConfig(self::getGuzzleBaseUriOptionName()); + $path = static::getCaBundlePath(); + + if ($path) { + $options['verify'] = $path; + } + + return GuzzleCompat::applyRequestOptions( + $options, + [ + 'timeout' => self::DEFAULT_TIMEOUT_S, + 'connect_timeout' => self::DEFAULT_TIMEOUT_S, + ] + ); } /** @@ -199,7 +205,7 @@ private function syncNotifyEndpointWithGuzzleBaseUri( return; } - $base = $this->getGuzzleBaseUri($guzzle); + $base = GuzzleCompat::getBaseUri($guzzle); if (is_string($base) || (is_object($base) && method_exists($base, '__toString'))) { $configuration->setNotifyEndpoint((string) $base); diff --git a/src/HttpClient.php b/src/HttpClient.php index 1c0ffda6..b6ee032a 100644 --- a/src/HttpClient.php +++ b/src/HttpClient.php @@ -3,6 +3,7 @@ namespace Bugsnag; use Bugsnag\DateTime\Date; +use Bugsnag\Internal\GuzzleCompat; use Exception; use GuzzleHttp\ClientInterface; use RuntimeException; @@ -267,10 +268,10 @@ protected function getHeaders($version = self::NOTIFY_PAYLOAD_VERSION) */ protected function post($uri, array $options = []) { - if (method_exists(ClientInterface::class, 'request')) { - $this->guzzle->request('POST', $uri, $options); - } else { + if (GuzzleCompat::isUsingGuzzle5()) { $this->guzzle->post($uri, $options); + } else { + $this->guzzle->request('POST', $uri, $options); } } diff --git a/src/Internal/GuzzleCompat.php b/src/Internal/GuzzleCompat.php new file mode 100644 index 00000000..c6120b50 --- /dev/null +++ b/src/Internal/GuzzleCompat.php @@ -0,0 +1,88 @@ +=') + && version_compare($version, '6.0.0', '<'); + } + + return false; + } + + /** + * Get the base URL/URI option name, which depends on the Guzzle version. + * + * @return string + */ + public static function getBaseUriOptionName() + { + return self::isUsingGuzzle5() ? 'base_url' : 'base_uri'; + } + + /** + * Get the base URL/URI, which depends on the Guzzle version. + * + * @param GuzzleHttp\ClientInterface $guzzle + * + * @return mixed + */ + public static function getBaseUri(GuzzleHttp\ClientInterface $guzzle) + { + return self::isUsingGuzzle5() + ? $guzzle->getBaseUrl() + : $guzzle->getConfig(self::getBaseUriOptionName()); + } + + /** + * Apply the given $requestOptions to the Guzzle $options array, if they are + * not already set. + * + * The layout of request options differs in Guzzle 5 to 6/7; in Guzzle 5 + * request options live in a 'defaults' array, but in 6/7 they are in the + * top level + * + * @param array $options + * @param array $requestOptions + * + * @return array + */ + public static function applyRequestOptions(array $options, array $requestOptions) + { + if (self::isUsingGuzzle5()) { + if (!isset($options['defaults'])) { + $options['defaults'] = []; + } + + foreach ($requestOptions as $key => $value) { + if (!isset($options['defaults'][$key])) { + $options['defaults'][$key] = $value; + } + } + + return $options; + } + + foreach ($requestOptions as $key => $value) { + if (!isset($options[$key])) { + $options[$key] = $value; + } + } + + return $options; + } +} diff --git a/tests/ClientTest.php b/tests/ClientTest.php index edf7288a..0df26ae7 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -4,6 +4,7 @@ use Bugsnag\Client; use Bugsnag\Configuration; +use Bugsnag\Internal\GuzzleCompat; use Bugsnag\Report; use Bugsnag\Tests\Fakes\FakeShutdownStrategy; use Exception; @@ -149,7 +150,7 @@ public function testTheApiKeyAndNotifyEndpointCanBeSetViaEnvSuperglobal() public function testTheNotifyEndpointCanBeSetBySettingItOnAGuzzleInstance() { $guzzle = new Guzzle([ - $this->getGuzzleBaseOptionName() => 'https://example.com', + GuzzleCompat::getBaseUriOptionName() => 'https://example.com', ]); $client = new Client(new Configuration('abc'), null, $guzzle); @@ -173,7 +174,7 @@ public function testTheNotifyEndpointWontBeOverwrittenByGivenGuzzleInstanceWhenO $config->setNotifyEndpoint('https://foo.com'); $guzzle = new Guzzle([ - $this->getGuzzleBaseOptionName() => 'https://example.com', + GuzzleCompat::getBaseUriOptionName() => 'https://example.com', ]); $client = new Client($config, null, $guzzle); @@ -195,14 +196,14 @@ public function testTheNotifyEndpointWontBeOverwrittenByMakeGuzzleWhenOneIsAlrea public function testTheNotifyEndpointCanBeSetBySettingItOnAGuzzleInstanceWithAnArray() { - if (!$this->isUsingGuzzle5()) { + if (!GuzzleCompat::isUsingGuzzle5()) { $this->markTestSkipped( 'This test is not relevant on Guzzle >= 6 as arrays are not allowed' ); } $guzzle = new Guzzle([ - $this->getGuzzleBaseOptionName() => [ + GuzzleCompat::getBaseUriOptionName() => [ 'https://example.com/{version}', ['version' => '1.2'], ], ]); @@ -215,7 +216,7 @@ public function testTheNotifyEndpointCanBeSetBySettingItOnAGuzzleInstanceWithAnA public function testTheNotifyEndpointCanBeSetBySettingItOnAGuzzleInstanceWithAUriInstance() { $guzzle = new Guzzle([ - $this->getGuzzleBaseOptionName() => new Uri('https://example.com:8080/hello/world'), + GuzzleCompat::getBaseUriOptionName() => new Uri('https://example.com:8080/hello/world'), ]); $client = new Client(new Configuration('abc'), null, $guzzle); @@ -1098,6 +1099,43 @@ public function testShutdownStrategyIsCalledWithinConstructor() $this->assertTrue($shutdownStrategy->wasRegistered()); } + public function testMakeGuzzleCreatesAGuzzleInstanceWithATimeout() + { + $guzzle = Client::makeGuzzle(); + + $timeout = $this->getGuzzleOption($guzzle, 'timeout'); + $connectTimeout = $this->getGuzzleOption($guzzle, 'connect_timeout'); + + $this->assertSame(Client::DEFAULT_TIMEOUT_S, $timeout); + $this->assertSame(Client::DEFAULT_TIMEOUT_S, $connectTimeout); + } + + public function testMakeGuzzleCreatesTimeoutCanBeSpecified() + { + $options = ['timeout' => 1, 'connect_timeout' => 2]; + + if (GuzzleCompat::isUsingGuzzle5()) { + $options = ['defaults' => $options]; + } + + $guzzle = Client::makeGuzzle(null, $options); + + $timeout = $this->getGuzzleOption($guzzle, 'timeout'); + $connectTimeout = $this->getGuzzleOption($guzzle, 'connect_timeout'); + + $this->assertSame(1, $timeout); + $this->assertSame(2, $connectTimeout); + } + + private function getGuzzleOption($guzzle, $name) + { + if (GuzzleCompat::isUsingGuzzle5()) { + return $guzzle->getDefaultOption($name); + } + + return $guzzle->getConfig($name); + } + private function expectGuzzlePostWith($uri, array $options = []) { $method = self::getGuzzleMethod(); diff --git a/tests/HttpClientTest.php b/tests/HttpClientTest.php index 93fc8282..2763ac8b 100644 --- a/tests/HttpClientTest.php +++ b/tests/HttpClientTest.php @@ -4,6 +4,7 @@ use Bugsnag\Configuration; use Bugsnag\HttpClient; +use Bugsnag\Internal\GuzzleCompat; use Bugsnag\Report; use Exception; use GuzzleHttp\Client; @@ -41,7 +42,7 @@ protected function beforeEach() private function setExpectedGuzzleParameters($expectation, $callback) { - if ($this->isUsingGuzzle5()) { + if (GuzzleCompat::isUsingGuzzle5()) { $expectation->with( $this->config->getNotifyEndpoint(), $this->callback($callback) diff --git a/tests/TestCase.php b/tests/TestCase.php index 04c134a7..439200f0 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -2,7 +2,7 @@ namespace Bugsnag\Tests; -use GuzzleHttp\ClientInterface; +use Bugsnag\Internal\GuzzleCompat; use phpmock\phpunit\PHPMock; use PHPUnit\Framework\TestCase as BaseTestCase; use PHPUnit\Runner\Version as PhpUnitVersion; @@ -43,23 +43,7 @@ protected function isPhpUnit4() */ protected static function getGuzzleMethod() { - return method_exists(ClientInterface::class, 'request') ? 'request' : 'post'; - } - - /** - * @return string - */ - protected function getGuzzleBaseOptionName() - { - return $this->isUsingGuzzle5() ? 'base_url' : 'base_uri'; - } - - /** - * @return bool - */ - protected function isUsingGuzzle5() - { - return method_exists(ClientInterface::class, 'getBaseUrl'); + return GuzzleCompat::isUsingGuzzle5() ? 'post' : 'request'; } private function phpUnitVersion()