From d232b4f89ea29bcf138b359f34c1ae9d9ba84057 Mon Sep 17 00:00:00 2001 From: Olivier Lechevalier Date: Sat, 20 Jul 2019 06:39:17 +0900 Subject: [PATCH 1/2] Extract the logic to create the transport from the client builder into a factory --- .appveyor.yml | 1 + composer.json | 2 +- phpstan.neon | 16 +- src/ClientBuilder.php | 142 ++++++++---------- src/ClientBuilderInterface.php | 15 ++ .../Authentication/SentryAuthentication.php | 1 - src/HttpClient/HttpClientFactory.php | 138 +++++++++++++++++ src/HttpClient/HttpClientFactoryInterface.php | 24 +++ src/HttpClient/PluggableHttpClientFactory.php | 52 +++++++ src/Transport/DefaultTransportFactory.php | 50 ++++++ src/Transport/TransportFactoryInterface.php | 23 +++ tests/ClientBuilderTest.php | 83 +++------- tests/ClientTest.php | 81 +++++----- .../SentryAuthenticationTest.php | 13 +- tests/HttpClient/HttpClientFactoryTest.php | 102 +++++++++++++ .../PluggableHttpClientFactoryTest.php | 54 +++++++ .../Transport/DefaultTransportFactoryTest.php | 44 ++++++ 17 files changed, 650 insertions(+), 191 deletions(-) create mode 100644 src/HttpClient/HttpClientFactory.php create mode 100644 src/HttpClient/HttpClientFactoryInterface.php create mode 100644 src/HttpClient/PluggableHttpClientFactory.php create mode 100644 src/Transport/DefaultTransportFactory.php create mode 100644 src/Transport/TransportFactoryInterface.php create mode 100644 tests/HttpClient/HttpClientFactoryTest.php create mode 100644 tests/HttpClient/PluggableHttpClientFactoryTest.php create mode 100644 tests/Transport/DefaultTransportFactoryTest.php diff --git a/.appveyor.yml b/.appveyor.yml index e652b52c6..1a3dbbccd 100644 --- a/.appveyor.yml +++ b/.appveyor.yml @@ -7,6 +7,7 @@ branches: only: - master - develop + - /^release\/.+$/ environment: matrix: diff --git a/composer.json b/composer.json index 837fc3ac6..fb7e18c00 100644 --- a/composer.json +++ b/composer.json @@ -30,7 +30,7 @@ "require-dev": { "friendsofphp/php-cs-fixer": "^2.13", "monolog/monolog": "^1.3|^2.0", - "php-http/mock-client": "^1.0", + "php-http/mock-client": "^1.3", "phpstan/extension-installer": "^1.0", "phpstan/phpstan": "^0.11", "phpstan/phpstan-phpunit": "^0.11", diff --git a/phpstan.neon b/phpstan.neon index 83e9202e4..ed6830c86 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -7,7 +7,6 @@ parameters: ignoreErrors: - '/Argument of an invalid type object supplied for foreach, only iterables are supported/' - '/Binary operation "\*" between array and 2 results in an error\./' - - '/Http\\Client\\Curl\\Client/' - '/^Parameter #1 \$object of method ReflectionProperty::setValue\(\) expects object, null given\.$/' # https://github.com/phpstan/phpstan/pull/2340 - message: /^Cannot assign offset 'os' to array\|string\.$/ @@ -15,15 +14,24 @@ parameters: - message: "/^Parameter #1 \\$function of function register_shutdown_function expects callable\\(\\): void, 'register_shutdown…' given\\.$/" path: src/Transport/HttpTransport.php + - + message: '/^Class Http\\Client\\Curl\\Client not found\.$/' + path: src/HttpClient/HttpClientFactory.php - message: '/^Class Http\\Adapter\\Guzzle6\\Client not found\.$/' - path: src/ClientBuilder.php + path: src/HttpClient/HttpClientFactory.php + - + message: '/^Instantiated class Http\\Client\\Curl\\Client not found\.$/' + path: src/HttpClient/HttpClientFactory.php - message: '/^Call to static method createWithConfig\(\) on an unknown class Http\\Adapter\\Guzzle6\\Client\.$/' - path: src/ClientBuilder.php + path: src/HttpClient/HttpClientFactory.php - message: '/^Access to constant PROXY on an unknown class GuzzleHttp\\RequestOptions\.$/' - path: src/ClientBuilder.php + path: src/HttpClient/HttpClientFactory.php + - + message: '/^Property Sentry\\HttpClient\\HttpClientFactory::\$httpClient \(Http\\Client\\HttpAsyncClient\|null\) does not accept Http\\Client\\Curl\\Client.$/' + path: src/HttpClient/HttpClientFactory.php excludes_analyse: - tests/resources - tests/Fixtures diff --git a/src/ClientBuilder.php b/src/ClientBuilder.php index 3e110b358..57525173b 100644 --- a/src/ClientBuilder.php +++ b/src/ClientBuilder.php @@ -4,20 +4,8 @@ namespace Sentry; -use GuzzleHttp\RequestOptions as GuzzleHttpClientOptions; -use Http\Adapter\Guzzle6\Client as GuzzleHttpClient; use Http\Client\Common\Plugin as PluginInterface; -use Http\Client\Common\Plugin\AuthenticationPlugin; -use Http\Client\Common\Plugin\BaseUriPlugin; -use Http\Client\Common\Plugin\DecoderPlugin; -use Http\Client\Common\Plugin\ErrorPlugin; -use Http\Client\Common\Plugin\HeaderSetPlugin; -use Http\Client\Common\Plugin\RetryPlugin; -use Http\Client\Common\PluginClient; -use Http\Client\Curl\Client as CurlHttpClient; use Http\Client\HttpAsyncClient; -use Http\Discovery\ClassDiscovery; -use Http\Discovery\HttpAsyncClientDiscovery; use Http\Discovery\MessageFactoryDiscovery; use Http\Discovery\StreamFactoryDiscovery; use Http\Discovery\UriFactoryDiscovery; @@ -25,8 +13,8 @@ use Http\Message\StreamFactory as StreamFactoryInterface; use Http\Message\UriFactory as UriFactoryInterface; use Jean85\PrettyVersions; -use Sentry\HttpClient\Authentication\SentryAuthentication; -use Sentry\HttpClient\Plugin\GzipEncoderPlugin; +use Sentry\HttpClient\HttpClientFactory; +use Sentry\HttpClient\PluggableHttpClientFactory; use Sentry\Integration\ErrorListenerIntegration; use Sentry\Integration\ExceptionListenerIntegration; use Sentry\Integration\FatalErrorListenerIntegration; @@ -36,8 +24,8 @@ use Sentry\Serializer\RepresentationSerializerInterface; use Sentry\Serializer\Serializer; use Sentry\Serializer\SerializerInterface; -use Sentry\Transport\HttpTransport; -use Sentry\Transport\NullTransport; +use Sentry\Transport\DefaultTransportFactory; +use Sentry\Transport\TransportFactoryInterface; use Sentry\Transport\TransportInterface; /** @@ -67,6 +55,11 @@ final class ClientBuilder implements ClientBuilderInterface */ private $messageFactory; + /** + * @var TransportFactoryInterface|null The transport factory + */ + private $transportFactory; + /** * @var TransportInterface|null The transport */ @@ -144,6 +137,8 @@ public function getOptions(): Options */ public function setUriFactory(UriFactoryInterface $uriFactory): ClientBuilderInterface { + @trigger_error(sprintf('Method %s() is deprecated since version 2.3 and will be removed in 3.0.', __METHOD__), E_USER_DEPRECATED); + $this->uriFactory = $uriFactory; return $this; @@ -154,6 +149,8 @@ public function setUriFactory(UriFactoryInterface $uriFactory): ClientBuilderInt */ public function setMessageFactory(MessageFactoryInterface $messageFactory): ClientBuilderInterface { + @trigger_error(sprintf('Method %s() is deprecated since version 2.3 and will be removed in 3.0.', __METHOD__), E_USER_DEPRECATED); + $this->messageFactory = $messageFactory; return $this; @@ -164,6 +161,8 @@ public function setMessageFactory(MessageFactoryInterface $messageFactory): Clie */ public function setTransport(TransportInterface $transport): ClientBuilderInterface { + @trigger_error(sprintf('Method %s() is deprecated since version 2.3 and will be removed in 3.0. Use the setTransportFactory() method instead.', __METHOD__), E_USER_DEPRECATED); + $this->transport = $transport; return $this; @@ -174,6 +173,8 @@ public function setTransport(TransportInterface $transport): ClientBuilderInterf */ public function setHttpClient(HttpAsyncClient $httpClient): ClientBuilderInterface { + @trigger_error(sprintf('Method %s() is deprecated since version 2.3 and will be removed in 3.0.', __METHOD__), E_USER_DEPRECATED); + $this->httpClient = $httpClient; return $this; @@ -184,6 +185,8 @@ public function setHttpClient(HttpAsyncClient $httpClient): ClientBuilderInterfa */ public function addHttpClientPlugin(PluginInterface $plugin): ClientBuilderInterface { + @trigger_error(sprintf('Method %s() is deprecated since version 2.3 and will be removed in 3.0.', __METHOD__), E_USER_DEPRECATED); + $this->httpClientPlugins[] = $plugin; return $this; @@ -194,6 +197,8 @@ public function addHttpClientPlugin(PluginInterface $plugin): ClientBuilderInter */ public function removeHttpClientPlugin(string $className): ClientBuilderInterface { + @trigger_error(sprintf('Method %s() is deprecated since version 2.3 and will be removed in 3.0.', __METHOD__), E_USER_DEPRECATED); + foreach ($this->httpClientPlugins as $index => $httpClientPlugin) { if (!$httpClientPlugin instanceof $className) { continue; @@ -274,40 +279,17 @@ public function getClient(): ClientInterface } /** - * Creates a new instance of the HTTP client. + * Sets the transport factory {@see TransportFactoryInterface}. * - * @return PluginClient + * @param TransportFactoryInterface $transportFactory The transport factory + * + * @return $this */ - private function createHttpClientInstance(): PluginClient + public function setTransportFactory(TransportFactoryInterface $transportFactory): ClientBuilderInterface { - if (null === $this->uriFactory) { - throw new \RuntimeException('The PSR-7 URI factory must be set.'); - } + $this->transportFactory = $transportFactory; - if (null === $this->streamFactory) { - throw new \RuntimeException('The PSR-17 stream factory must be set.'); - } - - if (null === $this->httpClient) { - throw new \RuntimeException('The PSR-18 HTTP client must be set.'); - } - - if (null !== $this->options->getDsn()) { - $this->addHttpClientPlugin(new BaseUriPlugin($this->uriFactory->createUri($this->options->getDsn()))); - } - - $this->addHttpClientPlugin(new HeaderSetPlugin(['User-Agent' => $this->sdkIdentifier . '/' . $this->sdkVersion])); - $this->addHttpClientPlugin(new AuthenticationPlugin(new SentryAuthentication($this->options, $this->sdkIdentifier, $this->sdkVersion))); - - if ($this->options->isCompressionEnabled()) { - $this->addHttpClientPlugin(new GzipEncoderPlugin($this->streamFactory)); - $this->addHttpClientPlugin(new DecoderPlugin()); - } - - $this->addHttpClientPlugin(new RetryPlugin(['retries' => $this->options->getSendAttempts()])); - $this->addHttpClientPlugin(new ErrorPlugin()); - - return new PluginClient($this->httpClient, $this->httpClientPlugins); + return $this; } /** @@ -321,38 +303,9 @@ private function createTransportInstance(): TransportInterface return $this->transport; } - if (null === $this->options->getDsn()) { - return new NullTransport(); - } + $transportFactory = $this->transportFactory ?? $this->createDefaultTransportFactory(); - $this->messageFactory = $this->messageFactory ?? MessageFactoryDiscovery::find(); - $this->streamFactory = $this->streamFactory ?? StreamFactoryDiscovery::find(); - $this->uriFactory = $this->uriFactory ?? UriFactoryDiscovery::find(); - - if (null !== $this->options->getHttpProxy()) { - if (null !== $this->httpClient) { - throw new \RuntimeException('The `http_proxy` option does not work together with a custom client.'); - } - - if (ClassDiscovery::safeClassExists(GuzzleHttpClient::class)) { - /** @psalm-suppress InvalidPropertyAssignmentValue */ - $this->httpClient = GuzzleHttpClient::createWithConfig([ - GuzzleHttpClientOptions::PROXY => $this->options->getHttpProxy(), - ]); - } elseif (ClassDiscovery::safeClassExists(CurlHttpClient::class)) { - /** @psalm-suppress InvalidPropertyAssignmentValue */ - $this->httpClient = new CurlHttpClient(null, null, [ - CURLOPT_PROXY => $this->options->getHttpProxy(), - ]); - } - - throw new \RuntimeException('The `http_proxy` option requires either the `php-http/curl-client` or the `php-http/guzzle6-adapter` package to be installed.'); - } - - /** @psalm-suppress PossiblyInvalidPropertyAssignmentValue */ - $this->httpClient = $this->httpClient ?? HttpAsyncClientDiscovery::find(); - - return new HttpTransport($this->options, $this->createHttpClientInstance(), $this->messageFactory); + return $transportFactory->create($this->options); } /** @@ -365,6 +318,39 @@ private function createEventFactory(): EventFactoryInterface $this->serializer = $this->serializer ?? new Serializer($this->options); $this->representationSerializer = $this->representationSerializer ?? new RepresentationSerializer($this->options); - return new EventFactory($this->serializer, $this->representationSerializer, $this->options, $this->sdkIdentifier, $this->sdkVersion); + return new EventFactory( + $this->serializer, + $this->representationSerializer, + $this->options, + $this->sdkIdentifier, + $this->sdkVersion + ); + } + + /** + * Creates a new instance of the {@see DefaultTransportFactory} factory. + * + * @return DefaultTransportFactory + */ + private function createDefaultTransportFactory(): DefaultTransportFactory + { + $this->messageFactory = $this->messageFactory ?? MessageFactoryDiscovery::find(); + $this->uriFactory = $this->uriFactory ?? UriFactoryDiscovery::find(); + $this->streamFactory = $this->streamFactory ?? StreamFactoryDiscovery::find(); + + $httpClientFactory = new HttpClientFactory( + $this->uriFactory, + $this->messageFactory, + $this->streamFactory, + $this->httpClient, + $this->sdkIdentifier, + $this->sdkVersion + ); + + if (!empty($this->httpClientPlugins)) { + $httpClientFactory = new PluggableHttpClientFactory($httpClientFactory, $this->httpClientPlugins); + } + + return new DefaultTransportFactory($this->messageFactory, $httpClientFactory); } } diff --git a/src/ClientBuilderInterface.php b/src/ClientBuilderInterface.php index 5a97eb466..6cf380642 100644 --- a/src/ClientBuilderInterface.php +++ b/src/ClientBuilderInterface.php @@ -10,12 +10,15 @@ use Http\Message\UriFactory as UriFactoryInterface; use Sentry\Serializer\RepresentationSerializerInterface; use Sentry\Serializer\SerializerInterface; +use Sentry\Transport\TransportFactoryInterface; use Sentry\Transport\TransportInterface; /** * A configurable builder for Client objects. * * @author Stefano Arlandini + * + * @method self setTransportFactory(TransportFactoryInterface $transportFactory) */ interface ClientBuilderInterface { @@ -41,6 +44,8 @@ public function getOptions(): Options; * @param UriFactoryInterface $uriFactory The factory * * @return $this + * + * @deprecated Since version 2.3, to be removed in 3.0 */ public function setUriFactory(UriFactoryInterface $uriFactory): self; @@ -50,6 +55,8 @@ public function setUriFactory(UriFactoryInterface $uriFactory): self; * @param MessageFactoryInterface $messageFactory The factory * * @return $this + * + * @deprecated Since version 2.3, to be removed in 3.0 */ public function setMessageFactory(MessageFactoryInterface $messageFactory): self; @@ -59,6 +66,8 @@ public function setMessageFactory(MessageFactoryInterface $messageFactory): self * @param TransportInterface $transport The transport * * @return $this + * + * @deprecated Since version 2.3, to be removed in 3.0 */ public function setTransport(TransportInterface $transport): self; @@ -68,6 +77,8 @@ public function setTransport(TransportInterface $transport): self; * @param HttpAsyncClient $httpClient The HTTP client * * @return $this + * + * @deprecated Since version 2.3, to be removed in 3.0 */ public function setHttpClient(HttpAsyncClient $httpClient): self; @@ -77,6 +88,8 @@ public function setHttpClient(HttpAsyncClient $httpClient): self; * @param PluginInterface $plugin The plugin instance * * @return $this + * + * @deprecated Since version 2.3, to be removed in 3.0 */ public function addHttpClientPlugin(PluginInterface $plugin): self; @@ -86,6 +99,8 @@ public function addHttpClientPlugin(PluginInterface $plugin): self; * @param string $className The class name * * @return $this + * + * @deprecated Since version 2.3, to be removed in 3.0 */ public function removeHttpClientPlugin(string $className): self; diff --git a/src/HttpClient/Authentication/SentryAuthentication.php b/src/HttpClient/Authentication/SentryAuthentication.php index 20a60809c..d28124c8c 100644 --- a/src/HttpClient/Authentication/SentryAuthentication.php +++ b/src/HttpClient/Authentication/SentryAuthentication.php @@ -64,7 +64,6 @@ public function authenticate(RequestInterface $request): RequestInterface $data = [ 'sentry_version' => Client::PROTOCOL_VERSION, 'sentry_client' => $this->sdkIdentifier . '/' . $this->sdkVersion, - 'sentry_timestamp' => sprintf('%F', microtime(true)), 'sentry_key' => $publicKey, ]; diff --git a/src/HttpClient/HttpClientFactory.php b/src/HttpClient/HttpClientFactory.php new file mode 100644 index 000000000..898bec81d --- /dev/null +++ b/src/HttpClient/HttpClientFactory.php @@ -0,0 +1,138 @@ +uriFactory = $uriFactory; + $this->responseFactory = $responseFactory; + $this->streamFactory = $streamFactory; + $this->httpClient = $httpClient; + $this->sdkIdentifier = $sdkIdentifier; + $this->sdkVersion = $sdkVersion; + } + + /** + * {@inheritdoc} + */ + public function create(Options $options): HttpAsyncClientInterface + { + if (null === $options->getDsn()) { + throw new \RuntimeException('Cannot create an HTTP client without the Sentry DSN set in the options.'); + } + + $httpClient = $this->httpClient; + + if (null !== $httpClient && null !== $options->getHttpProxy()) { + throw new \RuntimeException('The "http_proxy" option does not work together with a custom HTTP client.'); + } + + if (null === $httpClient && null !== $options->getHttpProxy()) { + if (class_exists(GuzzleHttpClient::class)) { + /** @psalm-suppress InvalidPropertyAssignmentValue */ + $this->httpClient = GuzzleHttpClient::createWithConfig([ + GuzzleHttpClientOptions::PROXY => $options->getHttpProxy(), + ]); + } elseif (class_exists(CurlHttpClient::class)) { + /** @psalm-suppress InvalidPropertyAssignmentValue */ + $this->httpClient = new CurlHttpClient($this->responseFactory, $this->streamFactory, [ + CURLOPT_PROXY => $options->getHttpProxy(), + ]); + } + + throw new \RuntimeException('The "http_proxy" option requires either the "php-http/curl-client" or the "php-http/guzzle6-adapter" package to be installed.'); + } + + if (null === $httpClient) { + $httpClient = HttpAsyncClientDiscovery::find(); + } + + $httpClientPlugins = [ + new BaseUriPlugin($this->uriFactory->createUri($options->getDsn())), + new HeaderSetPlugin(['User-Agent' => $this->sdkIdentifier . '/' . $this->sdkVersion]), + new AuthenticationPlugin(new SentryAuthentication($options, $this->sdkIdentifier, $this->sdkVersion)), + new RetryPlugin(['retries' => $options->getSendAttempts()]), + new ErrorPlugin(), + ]; + + if ($options->isCompressionEnabled()) { + $httpClientPlugins[] = new GzipEncoderPlugin($this->streamFactory); + $httpClientPlugins[] = new DecoderPlugin(); + } + + return new PluginClient($httpClient, $httpClientPlugins); + } +} diff --git a/src/HttpClient/HttpClientFactoryInterface.php b/src/HttpClient/HttpClientFactoryInterface.php new file mode 100644 index 000000000..9a6ea7eef --- /dev/null +++ b/src/HttpClient/HttpClientFactoryInterface.php @@ -0,0 +1,24 @@ + + * + * @deprecated since version 2.3, to be removed in 3.0 + */ +final class PluggableHttpClientFactory implements HttpClientFactoryInterface +{ + /** + * @var HttpClientFactoryInterface The HTTP factory being decorated + */ + private $decoratedHttpClientFactory; + + /** + * @var HttpClientPluginInterface[] The list of plugins to add to the HTTP client + */ + private $httpClientPlugins; + + /** + * Constructor. + * + * @param HttpClientFactoryInterface $decoratedHttpClientFactory The HTTP factory being decorated + * @param HttpClientPluginInterface[] $httpClientPlugins The list of plugins to add to the HTTP client + */ + public function __construct(HttpClientFactoryInterface $decoratedHttpClientFactory, array $httpClientPlugins) + { + $this->decoratedHttpClientFactory = $decoratedHttpClientFactory; + $this->httpClientPlugins = $httpClientPlugins; + } + + /** + * {@inheritdoc} + */ + public function create(Options $options): HttpAsyncClientInterface + { + $httpClient = $this->decoratedHttpClientFactory->create($options); + + return new PluginClient($httpClient, $this->httpClientPlugins); + } +} diff --git a/src/Transport/DefaultTransportFactory.php b/src/Transport/DefaultTransportFactory.php new file mode 100644 index 000000000..3a1df0676 --- /dev/null +++ b/src/Transport/DefaultTransportFactory.php @@ -0,0 +1,50 @@ +messageFactory = $messageFactory; + $this->httpClientFactory = $httpClientFactory; + } + + /** + * {@inheritdoc} + */ + public function create(Options $options): TransportInterface + { + if (null === $options->getDsn()) { + return new NullTransport(); + } + + return new HttpTransport($options, $this->httpClientFactory->create($options), $this->messageFactory, false); + } +} diff --git a/src/Transport/TransportFactoryInterface.php b/src/Transport/TransportFactoryInterface.php new file mode 100644 index 000000000..b3b6454db --- /dev/null +++ b/src/Transport/TransportFactoryInterface.php @@ -0,0 +1,23 @@ +assertInstanceOf(NullTransport::class, $transport); } + /** + * @group legacy + * + * @expectedDeprecation Method Sentry\ClientBuilder::setUriFactory() is deprecated since version 2.3 and will be removed in 3.0. + */ public function testSetUriFactory(): void { /** @var UriFactory|MockObject $uriFactory */ @@ -86,6 +89,11 @@ public function testSetMessageFactory(): void $this->assertAttributeSame($messageFactory, 'requestFactory', $transport); } + /** + * @group legacy + * + * @expectedDeprecation Method Sentry\ClientBuilder::setTransport() is deprecated since version 2.3 and will be removed in 3.0. Use the setTransportFactory() method instead. + */ public function testSetTransport(): void { /** @var TransportInterface|MockObject $transport */ @@ -118,6 +126,11 @@ public function testSetHttpClient(): void $this->assertAttributeSame($httpClient, 'client', $this->getObjectAttribute($transport, 'httpClient')); } + /** + * @group legacy + * + * @expectedDeprecationMessage Method Sentry\ClientBuilder::addHttpClientPlugin() is deprecated since version 2.3 and will be removed in 3.0. + */ public function testAddHttpClientPlugin(): void { /** @var Plugin|MockObject $plugin */ @@ -132,6 +145,12 @@ public function testAddHttpClientPlugin(): void $this->assertSame($plugin, $plugins[0]); } + /** + * @group legacy + * + * @expectedDeprecation Method Sentry\ClientBuilder::addHttpClientPlugin() is deprecated since version 2.3 and will be removed in 3.0. + * @expectedDeprecation Method Sentry\ClientBuilder::removeHttpClientPlugin() is deprecated since version 2.3 and will be removed in 3.0. + */ public function testRemoveHttpClientPlugin(): void { $plugin = new PluginStub1(); @@ -169,10 +188,6 @@ public function testGetClient(): void $this->assertAttributeSame($this->getObjectAttribute($clientBuilder, 'messageFactory'), 'requestFactory', $transport); $this->assertAttributeInstanceOf(PluginClient::class, 'httpClient', $transport); - - $httpClientPlugin = $this->getObjectAttribute($transport, 'httpClient'); - - $this->assertAttributeSame($this->getObjectAttribute($clientBuilder, 'httpClient'), 'client', $httpClientPlugin); } /** @@ -274,46 +289,6 @@ public function testClientBuilderSetsSdkIdentifierAndVersion(): void $this->assertTrue($callbackCalled, 'Callback not invoked, no assertions performed'); } - /** - * @group legacy - * - * @dataProvider getClientTogglesCompressionPluginInHttpClientDataProvider - * - * @expectedDeprecationMessage Delaying the sending of the events using the "Sentry\Transport\HttpTransport" class is deprecated since version 2.2 and will not work in 3.0. - */ - public function testGetClientTogglesCompressionPluginInHttpClient(bool $enabled): void - { - $options = new Options([ - 'dsn' => 'http://public:secret@example.com/sentry/1', - 'enable_compression' => $enabled, - ]); - - $builder = new ClientBuilder($options); - $builder->getClient(); - - $encoderPluginFound = false; - $decoderPluginFound = false; - - foreach ($this->getObjectAttribute($builder, 'httpClientPlugins') as $plugin) { - if ($plugin instanceof GzipEncoderPlugin) { - $encoderPluginFound = true; - } elseif ($plugin instanceof DecoderPlugin) { - $decoderPluginFound = true; - } - } - - $this->assertSame($enabled, $encoderPluginFound); - $this->assertSame($enabled, $decoderPluginFound); - } - - public function getClientTogglesCompressionPluginInHttpClientDataProvider(): array - { - return [ - [true], - [false], - ]; - } - public function testCreateWithNoOptionsIsTheSameAsDefaultOptions(): void { $this->assertEquals( @@ -321,22 +296,6 @@ public function testCreateWithNoOptionsIsTheSameAsDefaultOptions(): void ClientBuilder::create([]) ); } - - public function testCreateWithHttpProxyAndCustomTransportThrowsException(): void - { - $options = new Options([ - 'dsn' => 'http://public:secret@example.com/sentry/1', - 'http_proxy' => 'some-proxy', - ]); - - $clientBuilder = new ClientBuilder($options); - $clientBuilder->setHttpClient($this->createMock(HttpAsyncClient::class)); - - $this->expectException(\RuntimeException::class); - $this->expectExceptionMessage('The `http_proxy` option does not work together with a custom client.'); - - $clientBuilder->getClient(); - } } final class StubIntegration implements IntegrationInterface diff --git a/tests/ClientTest.php b/tests/ClientTest.php index e12e90c9c..31090b739 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -10,21 +10,19 @@ use PHPUnit\Framework\TestCase; use Sentry\ClientBuilder; use Sentry\Event; -use Sentry\EventFactory; use Sentry\Options; -use Sentry\Serializer\RepresentationSerializerInterface; use Sentry\Serializer\Serializer; -use Sentry\Serializer\SerializerInterface; use Sentry\Severity; use Sentry\Stacktrace; use Sentry\Transport\HttpTransport; +use Sentry\Transport\TransportFactoryInterface; use Sentry\Transport\TransportInterface; class ClientTest extends TestCase { public function testCaptureMessage(): void { - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->once()) ->method('send') @@ -36,7 +34,7 @@ public function testCaptureMessage(): void })); $client = ClientBuilder::create() - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); $client->captureMessage('foo', Severity::fatal()); @@ -46,7 +44,7 @@ public function testCaptureException(): void { $exception = new \Exception('Some foo error'); - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->once()) ->method('send') @@ -62,7 +60,7 @@ public function testCaptureException(): void })); $client = ClientBuilder::create() - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); $client->captureException($exception); @@ -74,6 +72,7 @@ public function testCaptureException(): void public function testCaptureExceptionDoesNothingIfExcludedExceptionsOptionMatches(bool $shouldCapture, string $excluded, \Throwable $thrown): void { $transport = $this->createMock(TransportInterface::class); + $transportFactory = $this->createTransportFactory($transport); $transport->expects($shouldCapture ? $this->once() : $this->never()) ->method('send') @@ -84,7 +83,7 @@ public function testCaptureExceptionDoesNothingIfExcludedExceptionsOptionMatches })); $client = ClientBuilder::create(['excluded_exceptions' => [$excluded]]) - ->setTransport($transport) + ->setTransportFactory($transportFactory) ->getClient(); $client->captureException($thrown); @@ -113,14 +112,14 @@ public function captureExceptionDoesNothingIfExcludedExceptionsOptionMatchesData public function testCapture(): void { - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->once()) ->method('send') ->willReturn('500a339f3ab2450b96dee542adf36ba7'); $client = ClientBuilder::create() - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); $inputData = [ @@ -137,7 +136,7 @@ public function testCapture(): void public function testCaptureLastError(): void { - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->once()) ->method('send') @@ -151,7 +150,7 @@ public function testCaptureLastError(): void })); $client = ClientBuilder::create(['dsn' => 'http://public:secret@example.com/1']) - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); @trigger_error('foo', E_USER_NOTICE); @@ -163,13 +162,13 @@ public function testCaptureLastError(): void public function testCaptureLastErrorDoesNothingWhenThereIsNoError(): void { - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->never()) ->method('send'); $client = ClientBuilder::create(['dsn' => 'http://public:secret@example.com/1']) - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); $this->clearLastError(); @@ -202,7 +201,7 @@ public function testSendChecksBeforeSendOption(): void { $beforeSendCalled = false; - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->never()) ->method('send'); @@ -215,7 +214,7 @@ public function testSendChecksBeforeSendOption(): void }); $client = (new ClientBuilder($options)) - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); $client->captureEvent([]); @@ -231,9 +230,10 @@ public function testSampleRateAbsolute(float $sampleRate): void $httpClient = new MockClient(); $options = new Options(['dsn' => 'http://public:secret@example.com/1']); $options->setSampleRate($sampleRate); + $transportFactory = $this->createTransportFactory(new HttpTransport($options, $httpClient, MessageFactoryDiscovery::find(), false)); $client = (new ClientBuilder($options)) - ->setTransport(new HttpTransport($options, $httpClient, MessageFactoryDiscovery::find(), false)) + ->setTransportFactory($transportFactory) ->getClient(); for ($i = 0; $i < 10; ++$i) { @@ -263,7 +263,7 @@ public function sampleRateAbsoluteDataProvider(): array */ public function testConvertException(\Exception $exception, array $expectedResult): void { - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->once()) ->method('send') @@ -281,7 +281,7 @@ public function testConvertException(\Exception $exception, array $expectedResul })); $client = ClientBuilder::create() - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); $client->captureException($exception); @@ -323,7 +323,7 @@ public function convertExceptionDataProvider(): array public function testConvertExceptionThrownInLatin1File(): void { - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->once()) ->method('send') @@ -358,7 +358,7 @@ public function testConvertExceptionThrownInLatin1File(): void $serializer->setMbDetectOrder('ISO-8859-1, ASCII, UTF-8'); $client = ClientBuilder::create() - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->setSerializer($serializer) ->getClient(); @@ -367,7 +367,7 @@ public function testConvertExceptionThrownInLatin1File(): void public function testAttachStacktrace(): void { - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->once()) ->method('send') @@ -386,7 +386,7 @@ public function testAttachStacktrace(): void })); $client = ClientBuilder::create(['attach_stacktrace' => true]) - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); $client->captureMessage('test'); @@ -394,7 +394,7 @@ public function testAttachStacktrace(): void public function testAttachStacktraceShouldNotWorkWithCaptureEvent(): void { - /** @var TransportInterface|MockObject $transport */ + /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->once()) ->method('send') @@ -405,7 +405,7 @@ public function testAttachStacktraceShouldNotWorkWithCaptureEvent(): void })); $client = ClientBuilder::create(['attach_stacktrace' => true]) - ->setTransport($transport) + ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); $client->captureEvent([]); @@ -416,23 +416,32 @@ public function testAttachStacktraceShouldNotWorkWithCaptureEvent(): void */ private function clearLastError(): void { - $handler = static function () { + set_error_handler(static function (): bool { return false; - }; + }); - set_error_handler($handler); @trigger_error(''); + restore_error_handler(); } - private function createEventFactory(): EventFactory + private function createTransportFactory(TransportInterface $transport): TransportFactoryInterface { - return new EventFactory( - $this->createMock(SerializerInterface::class), - $this->createMock(RepresentationSerializerInterface::class), - new Options(), - 'sentry.sdk.identifier', - '1.2.3' - ); + return new class($transport) implements TransportFactoryInterface { + /** + * @var TransportInterface + */ + private $transport; + + public function __construct(TransportInterface $transport) + { + $this->transport = $transport; + } + + public function create(Options $options): TransportInterface + { + return $this->transport; + } + }; } } diff --git a/tests/HttpClient/Authentication/SentryAuthenticationTest.php b/tests/HttpClient/Authentication/SentryAuthenticationTest.php index b99af9395..153577adb 100644 --- a/tests/HttpClient/Authentication/SentryAuthenticationTest.php +++ b/tests/HttpClient/Authentication/SentryAuthenticationTest.php @@ -12,9 +12,6 @@ use Sentry\HttpClient\Authentication\SentryAuthentication; use Sentry\Options; -/** - * @group time-sensitive - */ final class SentryAuthenticationTest extends TestCase { public function testAuthenticate(): void @@ -31,10 +28,9 @@ public function testAuthenticate(): void ->getMock(); $headerValue = sprintf( - 'Sentry sentry_version=%s, sentry_client=%s, sentry_timestamp=%F, sentry_key=public, sentry_secret=secret', + 'Sentry sentry_version=%s, sentry_client=%s, sentry_key=public, sentry_secret=secret', Client::PROTOCOL_VERSION, - 'sentry.php.test/1.2.3', - microtime(true) + 'sentry.php.test/1.2.3' ); $request->expects($this->once()) @@ -59,10 +55,9 @@ public function testAuthenticateWithNoSecretKey(): void ->getMock(); $headerValue = sprintf( - 'Sentry sentry_version=%s, sentry_client=%s, sentry_timestamp=%F, sentry_key=public', + 'Sentry sentry_version=%s, sentry_client=%s, sentry_key=public', Client::PROTOCOL_VERSION, - 'sentry.php.test/2.0.0', - microtime(true) + 'sentry.php.test/2.0.0' ); $request->expects($this->once()) diff --git a/tests/HttpClient/HttpClientFactoryTest.php b/tests/HttpClient/HttpClientFactoryTest.php new file mode 100644 index 000000000..f261efd2d --- /dev/null +++ b/tests/HttpClient/HttpClientFactoryTest.php @@ -0,0 +1,102 @@ +create(new Options([ + 'dsn' => 'http://public@example.com/sentry/1', + 'default_integrations' => false, + 'enable_compression' => $isCompressionEnabled, + ])); + + $httpClient->sendAsyncRequest(MessageFactoryDiscovery::find()->createRequest('POST', '/foo', [], 'foo bar')); + + /** @var RequestInterface|bool $httpRequest */ + $httpRequest = $mockHttpClient->getLastRequest(); + + $this->assertInstanceOf(RequestInterface::class, $httpRequest); + $this->assertSame('http://example.com/sentry/foo', (string) $httpRequest->getUri()); + $this->assertSame('sentry.php.test/1.2.3', (string) $httpRequest->getHeaderLine('User-Agent')); + $this->assertSame('Sentry sentry_version=7, sentry_client=sentry.php.test/1.2.3, sentry_key=public', (string) $httpRequest->getHeaderLine('X-Sentry-Auth')); + $this->assertSame($expectedRequestBody, (string) $httpRequest->getBody()); + } + + public function createDataProvider(): \Generator + { + yield [ + false, + 'foo bar', + ]; + + yield [ + true, + gzcompress('foo bar', -1, ZLIB_ENCODING_GZIP), + ]; + } + + public function testCreateThrowsIfDsnOptionIsNotConfigured(): void + { + $httpClientFactory = new HttpClientFactory( + UriFactoryDiscovery::find(), + MessageFactoryDiscovery::find(), + StreamFactoryDiscovery::find(), + null, + 'sentry.php.test', + '1.2.3' + ); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Cannot create an HTTP client without the Sentry DSN set in the options.'); + + $httpClientFactory->create(new Options(['default_integrations' => false])); + } + + public function testCreateThrowsIfHttpProxyOptionIsUsedWithCustomHttpClient(): void + { + $httpClientFactory = new HttpClientFactory( + UriFactoryDiscovery::find(), + MessageFactoryDiscovery::find(), + StreamFactoryDiscovery::find(), + $this->createMock(HttpAsyncClientInterface::class), + 'sentry.php.test', + '1.2.3' + ); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('The "http_proxy" option does not work together with a custom HTTP client.'); + + $httpClientFactory->create(new Options([ + 'dsn' => 'http://public@example.com/sentry/1', + 'default_integrations' => false, + 'http_proxy' => 'http://example.com', + ])); + } +} diff --git a/tests/HttpClient/PluggableHttpClientFactoryTest.php b/tests/HttpClient/PluggableHttpClientFactoryTest.php new file mode 100644 index 000000000..797850d52 --- /dev/null +++ b/tests/HttpClient/PluggableHttpClientFactoryTest.php @@ -0,0 +1,54 @@ +createMock(HttpAsyncClientInterface::class); + $wrappedHttpClient->expects($this->once()) + ->method('sendAsyncRequest') + ->willReturn($this->createMock(PromiseInterface::class)); + + /** @var HttpClientPluginInterface&MockObject $httpClientPlugin */ + $httpClientPlugin = $this->createMock(HttpClientPluginInterface::class); + $httpClientPlugin->expects($this->once()) + ->method('handleRequest') + ->willReturnCallback(static function (RequestInterface $request, callable $next): PromiseInterface { + return $next($request); + }); + + $httpClientFactory = new class($wrappedHttpClient) implements HttpClientFactoryInterface { + private $httpClient; + + public function __construct(HttpAsyncClientInterface $httpClient) + { + $this->httpClient = $httpClient; + } + + public function create(Options $options): HttpAsyncClientInterface + { + return $this->httpClient; + } + }; + + $httpClientFactory = new PluggableHttpClientFactory($httpClientFactory, [$httpClientPlugin]); + $httpClient = $httpClientFactory->create(new Options(['default_integrations' => false])); + + $httpClient->sendAsyncRequest($this->createMock(RequestInterface::class)); + } +} diff --git a/tests/Transport/DefaultTransportFactoryTest.php b/tests/Transport/DefaultTransportFactoryTest.php new file mode 100644 index 000000000..f3ceee135 --- /dev/null +++ b/tests/Transport/DefaultTransportFactoryTest.php @@ -0,0 +1,44 @@ +createMock(HttpClientFactoryInterface::class) + ); + + $this->assertInstanceOf(NullTransport::class, $factory->create(new Options())); + } + + public function testCreateReturnsHttpTransportWhenDsnOptionIsConfigured(): void + { + $options = new Options(['dsn' => 'http://public@example.com/sentry/1']); + + /** @var HttpClientFactoryInterface&MockObject $clientFactory */ + $clientFactory = $this->createMock(HttpClientFactoryInterface::class); + $clientFactory->expects($this->once()) + ->method('create') + ->with($options) + ->willReturn($this->createMock(HttpAsyncClientInterface::class)); + + $factory = new DefaultTransportFactory(MessageFactoryDiscovery::find(), $clientFactory); + + $this->assertInstanceOf(HttpTransport::class, $factory->create($options)); + } +} From 4afa495661250be42702b67f98969e38c83c04ea Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Mon, 7 Oct 2019 10:29:41 +0200 Subject: [PATCH 2/2] Fix CR issues --- src/Breadcrumb.php | 4 ++++ src/ClientBuilder.php | 10 ++-------- src/Transport/DefaultTransportFactory.php | 9 +++++++-- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/Breadcrumb.php b/src/Breadcrumb.php index 292e3b7d1..57ff4889e 100644 --- a/src/Breadcrumb.php +++ b/src/Breadcrumb.php @@ -135,9 +135,13 @@ public function __construct(string $level, string $type, string $category, ?stri * @param \ErrorException $exception The exception * * @return string + * + * @deprecated since version 2.3, to be removed in 3.0 */ public static function levelFromErrorException(\ErrorException $exception): string { + @trigger_error(sprintf('Method %s() is deprecated since version 2.3 and will be removed in 3.0.', __METHOD__), E_USER_DEPRECATED); + switch ($exception->getSeverity()) { case E_DEPRECATED: case E_USER_DEPRECATED: diff --git a/src/ClientBuilder.php b/src/ClientBuilder.php index 57525173b..05dd349af 100644 --- a/src/ClientBuilder.php +++ b/src/ClientBuilder.php @@ -279,7 +279,7 @@ public function getClient(): ClientInterface } /** - * Sets the transport factory {@see TransportFactoryInterface}. + * Sets the transport factory. * * @param TransportFactoryInterface $transportFactory The transport factory * @@ -318,13 +318,7 @@ private function createEventFactory(): EventFactoryInterface $this->serializer = $this->serializer ?? new Serializer($this->options); $this->representationSerializer = $this->representationSerializer ?? new RepresentationSerializer($this->options); - return new EventFactory( - $this->serializer, - $this->representationSerializer, - $this->options, - $this->sdkIdentifier, - $this->sdkVersion - ); + return new EventFactory($this->serializer, $this->representationSerializer, $this->options, $this->sdkIdentifier, $this->sdkVersion); } /** diff --git a/src/Transport/DefaultTransportFactory.php b/src/Transport/DefaultTransportFactory.php index 3a1df0676..3c5108e74 100644 --- a/src/Transport/DefaultTransportFactory.php +++ b/src/Transport/DefaultTransportFactory.php @@ -25,7 +25,7 @@ final class DefaultTransportFactory implements TransportFactoryInterface private $httpClientFactory; /** - * DefaultTransportFactory constructor. + * Constructor. * * @param MessageFactoryInterface $messageFactory The PSR-7 message factory * @param HttpClientFactoryInterface $httpClientFactory The HTTP client factory @@ -45,6 +45,11 @@ public function create(Options $options): TransportInterface return new NullTransport(); } - return new HttpTransport($options, $this->httpClientFactory->create($options), $this->messageFactory, false); + return new HttpTransport( + $options, + $this->httpClientFactory->create($options), + $this->messageFactory, + false + ); } }