From bdc76abedd3c65796ae1580e3b4c83225abba700 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 17 Nov 2020 11:54:07 +0000 Subject: [PATCH 1/5] Apply a default timeout to Guzzle instances --- src/Client.php | 74 +++++++++++++++++++++++++++++++++++++++----- tests/ClientTest.php | 37 ++++++++++++++++++++++ 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/src/Client.php b/src/Client.php index c113eedc..73d8add0 100644 --- a/src/Client.php +++ b/src/Client.php @@ -73,6 +73,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,15 +154,68 @@ public function __construct( */ public static function makeGuzzle($base = null, array $options = []) { - $key = self::getGuzzleBaseUriOptionName(); + $options = self::resolveGuzzleOptions($base, $options); + return new GuzzleHttp\Client($options); + } + + /** + * @param string|null $base + * @param array $options + * + * @return array + */ + private static function resolveGuzzleOptions($base, array $options) + { + $key = self::getGuzzleBaseUriOptionName(); $options[$key] = $base ?: Configuration::NOTIFY_ENDPOINT; - if ($path = static::getCaBundlePath()) { + $path = static::getCaBundlePath(); + + if ($path) { $options['verify'] = $path; } - return new GuzzleHttp\Client($options); + if (self::isUsingGuzzle5()) { + if (!isset($options['defaults'])) { + $options['defaults'] = []; + } + + if (!isset($options['defaults']['timeout'])) { + $options['defaults']['timeout'] = self::DEFAULT_TIMEOUT_S; + } + + if (!isset($options['defaults']['connect_timeout'])) { + $options['defaults']['connect_timeout'] = self::DEFAULT_TIMEOUT_S; + } + + return $options; + } + + if (!isset($options['timeout'])) { + $options['timeout'] = self::DEFAULT_TIMEOUT_S; + } + + if (!isset($options['connect_timeout'])) { + $options['connect_timeout'] = self::DEFAULT_TIMEOUT_S; + } + + return $options; + } + + /** + * @return bool + */ + private static function isUsingGuzzle5() + { + if (defined(GuzzleHttp\ClientInterface::class.'::VERSION')) { + $version = constant(GuzzleHttp\ClientInterface::class.'::VERSION'); + + return version_compare($version, '5.0.0', '>=') + && version_compare($version, '6.0.0', '<'); + } + + return false; } /** @@ -163,9 +225,7 @@ public static function makeGuzzle($base = null, array $options = []) */ private static function getGuzzleBaseUriOptionName() { - return method_exists(GuzzleHttp\ClientInterface::class, 'request') - ? 'base_uri' - : 'base_url'; + return self::isUsingGuzzle5() ? 'base_url' : 'base_uri'; } /** @@ -175,7 +235,7 @@ private static function getGuzzleBaseUriOptionName() */ private function getGuzzleBaseUri(GuzzleHttp\ClientInterface $guzzle) { - return method_exists(GuzzleHttp\ClientInterface::class, 'getBaseUrl') + return self::isUsingGuzzle5() ? $guzzle->getBaseUrl() : $guzzle->getConfig(self::getGuzzleBaseUriOptionName()); } diff --git a/tests/ClientTest.php b/tests/ClientTest.php index edf7288a..99521ba4 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -1098,6 +1098,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 ($this->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 ($this->isUsingGuzzle5()) { + return $guzzle->getDefaultOption($name); + } + + return $guzzle->getConfig($name); + } + private function expectGuzzlePostWith($uri, array $options = []) { $method = self::getGuzzleMethod(); From 03ba20023b7afb78aea697b6d522a43f0215d395 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 17 Nov 2020 14:23:15 +0000 Subject: [PATCH 2/5] Add class to handle Guzzle compatibility --- src/Client.php | 73 +++++------------------------------- src/GuzzleCompat.php | 88 ++++++++++++++++++++++++++++++++++++++++++++ src/HttpClient.php | 6 +-- 3 files changed, 100 insertions(+), 67 deletions(-) create mode 100644 src/GuzzleCompat.php diff --git a/src/Client.php b/src/Client.php index 73d8add0..765636dd 100644 --- a/src/Client.php +++ b/src/Client.php @@ -167,7 +167,7 @@ public static function makeGuzzle($base = null, array $options = []) */ private static function resolveGuzzleOptions($base, array $options) { - $key = self::getGuzzleBaseUriOptionName(); + $key = GuzzleCompat::getBaseUriOptionName(); $options[$key] = $base ?: Configuration::NOTIFY_ENDPOINT; $path = static::getCaBundlePath(); @@ -176,68 +176,13 @@ private static function resolveGuzzleOptions($base, array $options) $options['verify'] = $path; } - if (self::isUsingGuzzle5()) { - if (!isset($options['defaults'])) { - $options['defaults'] = []; - } - - if (!isset($options['defaults']['timeout'])) { - $options['defaults']['timeout'] = self::DEFAULT_TIMEOUT_S; - } - - if (!isset($options['defaults']['connect_timeout'])) { - $options['defaults']['connect_timeout'] = self::DEFAULT_TIMEOUT_S; - } - - return $options; - } - - if (!isset($options['timeout'])) { - $options['timeout'] = self::DEFAULT_TIMEOUT_S; - } - - if (!isset($options['connect_timeout'])) { - $options['connect_timeout'] = self::DEFAULT_TIMEOUT_S; - } - - return $options; - } - - /** - * @return bool - */ - private static function isUsingGuzzle5() - { - if (defined(GuzzleHttp\ClientInterface::class.'::VERSION')) { - $version = constant(GuzzleHttp\ClientInterface::class.'::VERSION'); - - return version_compare($version, '5.0.0', '>=') - && version_compare($version, '6.0.0', '<'); - } - - return false; - } - - /** - * Get the base URL/URI option name, which depends on the Guzzle version. - * - * @return string - */ - private static function getGuzzleBaseUriOptionName() - { - return self::isUsingGuzzle5() ? 'base_url' : 'base_uri'; - } - - /** - * Get the base URL/URI, which depends on the Guzzle version. - * - * @return mixed - */ - private function getGuzzleBaseUri(GuzzleHttp\ClientInterface $guzzle) - { - return self::isUsingGuzzle5() - ? $guzzle->getBaseUrl() - : $guzzle->getConfig(self::getGuzzleBaseUriOptionName()); + return GuzzleCompat::applyRequestOptions( + $options, + [ + 'timeout' => self::DEFAULT_TIMEOUT_S, + 'connect_timeout' => self::DEFAULT_TIMEOUT_S, + ] + ); } /** @@ -259,7 +204,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/GuzzleCompat.php b/src/GuzzleCompat.php new file mode 100644 index 00000000..0f562a48 --- /dev/null +++ b/src/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/src/HttpClient.php b/src/HttpClient.php index 1c0ffda6..669bcc88 100644 --- a/src/HttpClient.php +++ b/src/HttpClient.php @@ -267,10 +267,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); } } From 8b2499f2b1c2be41b1b04c13e65eb112998af2eb Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 17 Nov 2020 14:24:16 +0000 Subject: [PATCH 3/5] Use GuzzleCompat in tests --- tests/ClientTest.php | 15 ++++++++------- tests/HttpClientTest.php | 3 ++- tests/TestCase.php | 20 ++------------------ 3 files changed, 12 insertions(+), 26 deletions(-) diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 99521ba4..59478d52 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -4,6 +4,7 @@ use Bugsnag\Client; use Bugsnag\Configuration; +use Bugsnag\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); @@ -1113,7 +1114,7 @@ public function testMakeGuzzleCreatesTimeoutCanBeSpecified() { $options = ['timeout' => 1, 'connect_timeout' => 2]; - if ($this->isUsingGuzzle5()) { + if (GuzzleCompat::isUsingGuzzle5()) { $options = ['defaults' => $options]; } @@ -1128,7 +1129,7 @@ public function testMakeGuzzleCreatesTimeoutCanBeSpecified() private function getGuzzleOption($guzzle, $name) { - if ($this->isUsingGuzzle5()) { + if (GuzzleCompat::isUsingGuzzle5()) { return $guzzle->getDefaultOption($name); } diff --git a/tests/HttpClientTest.php b/tests/HttpClientTest.php index 93fc8282..7b5938f3 100644 --- a/tests/HttpClientTest.php +++ b/tests/HttpClientTest.php @@ -3,6 +3,7 @@ namespace Bugsnag\Tests; use Bugsnag\Configuration; +use Bugsnag\GuzzleCompat; use Bugsnag\HttpClient; use Bugsnag\Report; use Exception; @@ -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..7d56df6a 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -2,7 +2,7 @@ namespace Bugsnag\Tests; -use GuzzleHttp\ClientInterface; +use Bugsnag\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() From c23eb36665a5e103b0d880ab05543e2e1d9bd288 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 17 Nov 2020 14:37:31 +0000 Subject: [PATCH 4/5] Move GuzzleCompat to Internal namespace --- src/Client.php | 1 + src/HttpClient.php | 1 + src/{ => Internal}/GuzzleCompat.php | 2 +- tests/ClientTest.php | 2 +- tests/HttpClientTest.php | 2 +- tests/TestCase.php | 2 +- 6 files changed, 6 insertions(+), 4 deletions(-) rename src/{ => Internal}/GuzzleCompat.php (98%) diff --git a/src/Client.php b/src/Client.php index 765636dd..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; diff --git a/src/HttpClient.php b/src/HttpClient.php index 669bcc88..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; diff --git a/src/GuzzleCompat.php b/src/Internal/GuzzleCompat.php similarity index 98% rename from src/GuzzleCompat.php rename to src/Internal/GuzzleCompat.php index 0f562a48..c6120b50 100644 --- a/src/GuzzleCompat.php +++ b/src/Internal/GuzzleCompat.php @@ -1,6 +1,6 @@ Date: Wed, 18 Nov 2020 16:31:26 +0000 Subject: [PATCH 5/5] Add changelog entry --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) 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`