From 6e1f60484e292549938686299ce0363111eac65d Mon Sep 17 00:00:00 2001 From: Alessandro Lai Date: Wed, 19 Dec 2018 09:52:57 +0100 Subject: [PATCH] Add return types to the ClientBuilder methods and make the Options class final (#728) --- phpstan.neon | 2 +- src/ClientBuilder.php | 93 +++++++++----------------------- src/ClientBuilderInterface.php | 31 ++++++----- src/Options.php | 2 +- src/Sdk.php | 3 +- tests/ClientBuilderTest.php | 98 +++++++++++----------------------- tests/ClientTest.php | 41 +++++++------- 7 files changed, 95 insertions(+), 175 deletions(-) diff --git a/phpstan.neon b/phpstan.neon index e8cd53958..44470014f 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -4,7 +4,7 @@ parameters: - src - tests ignoreErrors: - - '/Call to an undefined method Sentry\\ClientBuilder::methodThatDoesNotExists\(\)/' + - '/Method Sentry\\ClientBuilder::\w+\(\) should return \$this\(Sentry\\ClientBuilderInterface\) but returns \$this\(Sentry\\ClientBuilder\)/' - '/Argument of an invalid type object supplied for foreach, only iterables are supported/' - '/Binary operation "\*" between array and 2 results in an error\./' - '/Method Sentry\\Serializer\\RepresentationSerializer::(representationSerialize|serializeValue)\(\) should return [\w|]+ but returns [\w|]+/' diff --git a/src/ClientBuilder.php b/src/ClientBuilder.php index ce5f23e7b..63fae8a80 100644 --- a/src/ClientBuilder.php +++ b/src/ClientBuilder.php @@ -34,40 +34,6 @@ * The default implementation of {@link ClientBuilderInterface}. * * @author Stefano Arlandini - * - * @method int getSendAttempts() - * @method setSendAttempts(int $attemptsCount) - * @method string[] getPrefixes() - * @method setPrefixes(array $prefixes) - * @method float getSampleRate() - * @method setSampleRate(float $sampleRate) - * @method bool shouldAttachStacktrace() - * @method setAttachStacktrace(bool $enable) - * @method int getContextLines() - * @method setContextLines(int $contextLines) - * @method null|string getEnvironment() - * @method setEnvironment(null|string $environment) - * @method string[] getExcludedProjectPaths() - * @method setExcludedProjectPaths(string[] $paths) - * @method setExcludedLoggers(string[] $loggers) - * @method string[] getExcludedExceptions() - * @method string getProjectRoot() - * @method setProjectRoot(string $path) - * @method string getLogger() - * @method setLogger(string $logger) - * @method string getRelease() - * @method setRelease(string $release) - * @method string getDsn() - * @method string getServerName() - * @method setServerName(string $serverName) - * @method string[] getTags() - * @method setTags(string[] $tags) - * @method bool shouldSendDefaultPii() - * @method setSendDefaultPii(bool $enable) - * @method bool hasDefaultIntegrations() - * @method setDefaultIntegrations(bool $enable) - * @method callable getBeforeSendCallback() - * @method setBeforeSendCallback(callable $beforeSend) */ final class ClientBuilder implements ClientBuilderInterface { @@ -124,11 +90,11 @@ final class ClientBuilder implements ClientBuilderInterface /** * Class constructor. * - * @param array $options The client options + * @param Options|null $options The client options */ - public function __construct(array $options = []) + public function __construct(Options $options = null) { - $this->options = new Options($options); + $this->options = $options ?? new Options(); if ($this->options->hasDefaultIntegrations()) { $this->options->setIntegrations(\array_merge([ @@ -141,15 +107,23 @@ public function __construct(array $options = []) /** * {@inheritdoc} */ - public static function create(array $options = []): self + public static function create(array $options = []): ClientBuilderInterface { - return new static($options); + return new static(new Options($options)); } /** * {@inheritdoc} */ - public function setUriFactory(UriFactory $uriFactory): self + public function getOptions(): Options + { + return $this->options; + } + + /** + * {@inheritdoc} + */ + public function setUriFactory(UriFactory $uriFactory): ClientBuilderInterface { $this->uriFactory = $uriFactory; @@ -159,7 +133,7 @@ public function setUriFactory(UriFactory $uriFactory): self /** * {@inheritdoc} */ - public function setMessageFactory(MessageFactory $messageFactory): self + public function setMessageFactory(MessageFactory $messageFactory): ClientBuilderInterface { $this->messageFactory = $messageFactory; @@ -169,7 +143,7 @@ public function setMessageFactory(MessageFactory $messageFactory): self /** * {@inheritdoc} */ - public function setTransport(TransportInterface $transport): self + public function setTransport(TransportInterface $transport): ClientBuilderInterface { $this->transport = $transport; @@ -179,7 +153,7 @@ public function setTransport(TransportInterface $transport): self /** * {@inheritdoc} */ - public function setHttpClient(HttpAsyncClient $httpClient): self + public function setHttpClient(HttpAsyncClient $httpClient): ClientBuilderInterface { $this->httpClient = $httpClient; @@ -189,7 +163,7 @@ public function setHttpClient(HttpAsyncClient $httpClient): self /** * {@inheritdoc} */ - public function addHttpClientPlugin(Plugin $plugin): self + public function addHttpClientPlugin(Plugin $plugin): ClientBuilderInterface { $this->httpClientPlugins[] = $plugin; @@ -199,7 +173,7 @@ public function addHttpClientPlugin(Plugin $plugin): self /** * {@inheritdoc} */ - public function removeHttpClientPlugin(string $className): self + public function removeHttpClientPlugin(string $className): ClientBuilderInterface { foreach ($this->httpClientPlugins as $index => $httpClientPlugin) { if (!$httpClientPlugin instanceof $className) { @@ -215,7 +189,7 @@ public function removeHttpClientPlugin(string $className): self /** * {@inheritdoc} */ - public function setSerializer(SerializerInterface $serializer): self + public function setSerializer(SerializerInterface $serializer): ClientBuilderInterface { $this->serializer = $serializer; @@ -225,7 +199,7 @@ public function setSerializer(SerializerInterface $serializer): self /** * {@inheritdoc} */ - public function setRepresentationSerializer(RepresentationSerializerInterface $representationSerializer): self + public function setRepresentationSerializer(RepresentationSerializerInterface $representationSerializer): ClientBuilderInterface { $this->representationSerializer = $representationSerializer; @@ -235,7 +209,7 @@ public function setRepresentationSerializer(RepresentationSerializerInterface $r /** * {@inheritdoc} */ - public function setSdkIdentifier(string $sdkIdentifier): self + public function setSdkIdentifier(string $sdkIdentifier): ClientBuilderInterface { $this->sdkIdentifier = $sdkIdentifier; @@ -259,7 +233,7 @@ private function getSdkVersion(): string /** * {@inheritdoc} */ - public function setSdkVersion(string $sdkVersion): self + public function setSdkVersion(string $sdkVersion): ClientBuilderInterface { $this->sdkVersion = $sdkVersion; @@ -273,7 +247,7 @@ public function setSdkVersion(string $sdkVersion): self * * @return $this */ - public function setSdkVersionByPackageName(string $packageName): self + public function setSdkVersionByPackageName(string $packageName): ClientBuilderInterface { $this->sdkVersion = PrettyVersions::getVersion($packageName)->getPrettyVersion(); @@ -293,25 +267,6 @@ public function getClient(): ClientInterface return new Client($this->options, $this->transport, $this->createEventFactory()); } - /** - * This method forwards all methods calls to the options object. - * - * @param string $name The name of the method being called - * @param array $arguments Parameters passed to the $name'ed method - * - * @return $this - * - * @throws \BadMethodCallException If the called method does not exists - */ - public function __call($name, $arguments) - { - if (!method_exists($this->options, $name)) { - throw new \BadMethodCallException(sprintf('The method named "%s" does not exists.', $name)); - } - - return $this->options->$name(...$arguments); - } - /** * Creates a new instance of the HTTP client. * diff --git a/src/ClientBuilderInterface.php b/src/ClientBuilderInterface.php index eecbfbc03..6bb57b454 100644 --- a/src/ClientBuilderInterface.php +++ b/src/ClientBuilderInterface.php @@ -22,11 +22,18 @@ interface ClientBuilderInterface /** * Creates a new instance of this builder. * - * @param array $options The client options + * @param array $options The client options, in naked array form * * @return static */ - public static function create(array $options = []); + public static function create(array $options = []): self; + + /** + * The options that will be used to create the {@see Client}. + * + * @return Options + */ + public function getOptions(): Options; /** * Sets the factory to use to create URIs. @@ -35,7 +42,7 @@ public static function create(array $options = []); * * @return $this */ - public function setUriFactory(UriFactory $uriFactory); + public function setUriFactory(UriFactory $uriFactory): self; /** * Sets the factory to use to create PSR-7 messages. @@ -44,7 +51,7 @@ public function setUriFactory(UriFactory $uriFactory); * * @return $this */ - public function setMessageFactory(MessageFactory $messageFactory); + public function setMessageFactory(MessageFactory $messageFactory): self; /** * Sets the transport that will be used to send events. @@ -53,7 +60,7 @@ public function setMessageFactory(MessageFactory $messageFactory); * * @return $this */ - public function setTransport(TransportInterface $transport); + public function setTransport(TransportInterface $transport): self; /** * Sets the HTTP client. @@ -62,7 +69,7 @@ public function setTransport(TransportInterface $transport); * * @return $this */ - public function setHttpClient(HttpAsyncClient $httpClient); + public function setHttpClient(HttpAsyncClient $httpClient): self; /** * Adds a new HTTP client plugin to the end of the plugins chain. @@ -71,7 +78,7 @@ public function setHttpClient(HttpAsyncClient $httpClient); * * @return $this */ - public function addHttpClientPlugin(Plugin $plugin); + public function addHttpClientPlugin(Plugin $plugin): self; /** * Removes a HTTP client plugin by its fully qualified class name (FQCN). @@ -80,7 +87,7 @@ public function addHttpClientPlugin(Plugin $plugin); * * @return $this */ - public function removeHttpClientPlugin(string $className); + public function removeHttpClientPlugin(string $className): self; /** * Gets the instance of the client built using the configured options. @@ -96,7 +103,7 @@ public function getClient(): ClientInterface; * * @return $this */ - public function setSerializer(SerializerInterface $serializer); + public function setSerializer(SerializerInterface $serializer): self; /** * Sets a representation serializer instance to be injected as a dependency of the client. @@ -107,7 +114,7 @@ public function setSerializer(SerializerInterface $serializer); * * @return $this */ - public function setRepresentationSerializer(RepresentationSerializerInterface $representationSerializer); + public function setRepresentationSerializer(RepresentationSerializerInterface $representationSerializer): self; /** * Sets the SDK identifier to be passed onto {@see Event} and HTTP User-Agent header. @@ -118,7 +125,7 @@ public function setRepresentationSerializer(RepresentationSerializerInterface $r * * @internal */ - public function setSdkIdentifier(string $sdkIdentifier); + public function setSdkIdentifier(string $sdkIdentifier): self; /** * Sets the SDK version to be passed onto {@see Event} and HTTP User-Agent header. @@ -129,5 +136,5 @@ public function setSdkIdentifier(string $sdkIdentifier); * * @internal */ - public function setSdkVersion(string $sdkVersion); + public function setSdkVersion(string $sdkVersion): self; } diff --git a/src/Options.php b/src/Options.php index db8234af8..615df288b 100644 --- a/src/Options.php +++ b/src/Options.php @@ -13,7 +13,7 @@ * * @author Stefano Arlandini */ -class Options +final class Options { /** * The default maximum number of breadcrumbs that will be sent with an event. diff --git a/src/Sdk.php b/src/Sdk.php index 8153bc4b0..d7efcb260 100644 --- a/src/Sdk.php +++ b/src/Sdk.php @@ -13,7 +13,8 @@ */ function init(array $options = []): void { - Hub::setCurrent(new Hub(ClientBuilder::create($options)->getClient())); + $client = ClientBuilder::create($options)->getClient(); + Hub::setCurrent(new Hub($client)); } /** diff --git a/tests/ClientBuilderTest.php b/tests/ClientBuilderTest.php index a72db219f..4e38a1834 100644 --- a/tests/ClientBuilderTest.php +++ b/tests/ClientBuilderTest.php @@ -35,7 +35,7 @@ public function testCreate(): void public function testHttpTransportIsUsedWhenServeIsConfigured(): void { - $clientBuilder = new ClientBuilder(['dsn' => 'http://public:secret@example.com/sentry/1']); + $clientBuilder = ClientBuilder::create(['dsn' => 'http://public:secret@example.com/sentry/1']); $transport = $this->getObjectAttribute($clientBuilder->getClient(), 'transport'); @@ -56,7 +56,7 @@ public function testSetUriFactory(): void /** @var UriFactory|MockObject $uriFactory */ $uriFactory = $this->createMock(UriFactory::class); - $clientBuilder = new ClientBuilder(['dsn' => 'http://public:secret@example.com/sentry/1']); + $clientBuilder = ClientBuilder::create(['dsn' => 'http://public:secret@example.com/sentry/1']); $clientBuilder->setUriFactory($uriFactory); $this->assertAttributeSame($uriFactory, 'uriFactory', $clientBuilder); @@ -67,7 +67,7 @@ public function testSetMessageFactory(): void /** @var MessageFactory|MockObject $messageFactory */ $messageFactory = $this->createMock(MessageFactory::class); - $clientBuilder = new ClientBuilder(['dsn' => 'http://public:secret@example.com/sentry/1']); + $clientBuilder = ClientBuilder::create(['dsn' => 'http://public:secret@example.com/sentry/1']); $clientBuilder->setMessageFactory($messageFactory); $this->assertAttributeSame($messageFactory, 'messageFactory', $clientBuilder); @@ -82,7 +82,7 @@ public function testSetTransport(): void /** @var TransportInterface|MockObject $transport */ $transport = $this->createMock(TransportInterface::class); - $clientBuilder = new ClientBuilder(['dsn' => 'http://public:secret@example.com/sentry/1']); + $clientBuilder = ClientBuilder::create(['dsn' => 'http://public:secret@example.com/sentry/1']); $clientBuilder->setTransport($transport); $this->assertAttributeSame($transport, 'transport', $clientBuilder); @@ -94,7 +94,7 @@ public function testSetHttpClient(): void /** @var HttpAsyncClient|MockObject $httpClient */ $httpClient = $this->createMock(HttpAsyncClient::class); - $clientBuilder = new ClientBuilder(['dsn' => 'http://public:secret@example.com/sentry/1']); + $clientBuilder = ClientBuilder::create(['dsn' => 'http://public:secret@example.com/sentry/1']); $clientBuilder->setHttpClient($httpClient); $this->assertAttributeSame($httpClient, 'httpClient', $clientBuilder); @@ -140,7 +140,7 @@ public function testRemoveHttpClientPlugin(): void public function testGetClient(): void { - $clientBuilder = new ClientBuilder(['dsn' => 'http://public:secret@example.com/sentry/1']); + $clientBuilder = ClientBuilder::create(['dsn' => 'http://public:secret@example.com/sentry/1']); $client = $clientBuilder->getClient(); $this->assertInstanceOf(Client::class, $client); @@ -161,13 +161,14 @@ public function testGetClient(): void */ public function testIntegrationsAreAddedToClientCorrectly(bool $defaultIntegrations, array $integrations, array $expectedIntegrations): void { - $clientBuilder = new ClientBuilder([ - 'default_integrations' => $defaultIntegrations, - 'integrations' => $integrations, - ]); + $options = new Options(); + $options->setDefaultIntegrations($defaultIntegrations); + $options->setIntegrations($integrations); + $clientBuilder = new ClientBuilder($options); $client = $clientBuilder->getClient(); - $actualIntegrationsClassNames = array_map('get_class', $client->getOptions()->getIntegrations()); + + $actualIntegrationsClassNames = array_map('\get_class', $client->getOptions()->getIntegrations()); $this->assertEquals($expectedIntegrations, $actualIntegrationsClassNames, '', 0, 10, true); } @@ -198,63 +199,13 @@ public function integrationsAreAddedToClientCorrectlyDataProvider(): array ]; } - /** - * @expectedException \BadMethodCallException - * @expectedExceptionMessage The method named "methodThatDoesNotExists" does not exists. - */ - public function testCallInvalidMethodThrowsException(): void - { - $clientBuilder = new ClientBuilder(); - $clientBuilder->methodThatDoesNotExists(); - } - - /** - * @dataProvider optionsDataProvider - */ - public function testCallExistingMethodForwardsCallToConfiguration(string $setterMethod, $value): void - { - $options = $this->createMock(Options::class); - $options->expects($this->once()) - ->method($setterMethod) - ->with($this->equalTo($value)); - - $clientBuilder = new ClientBuilder(); - - $reflectionProperty = new \ReflectionProperty(ClientBuilder::class, 'options'); - $reflectionProperty->setAccessible(true); - $reflectionProperty->setValue($clientBuilder, $options); - $reflectionProperty->setAccessible(false); - - $clientBuilder->$setterMethod($value); - } - - public function optionsDataProvider(): array - { - return [ - ['setPrefixes', ['foo', 'bar']], - ['setSampleRate', 0.5], - ['setAttachStacktrace', true], - ['setContextLines', 0], - ['setEnableCompression', false], - ['setEnvironment', 'test'], - ['setExcludedProjectPaths', ['foo', 'bar']], - ['setExcludedExceptions', ['foo', 'bar']], - ['setProjectRoot', 'foo'], - ['setLogger', 'bar'], - ['setRelease', 'dev'], - ['setServerName', 'example.com'], - ['setTags', ['foo', 'bar']], - ['setErrorTypes', 0], - ]; - } - public function testClientBuilderFallbacksToDefaultSdkIdentifierAndVersion(): void { $callbackCalled = false; $expectedVersion = PrettyVersions::getVersion('sentry/sentry')->getPrettyVersion(); - $clientBuilder = new ClientBuilder(); - $clientBuilder->setBeforeSendCallback(function (Event $event) use ($expectedVersion, &$callbackCalled) { + $options = new Options(); + $options->setBeforeSendCallback(function (Event $event) use ($expectedVersion, &$callbackCalled) { $callbackCalled = true; $this->assertSame(Client::SDK_IDENTIFIER, $event->getSdkIdentifier()); @@ -263,7 +214,7 @@ public function testClientBuilderFallbacksToDefaultSdkIdentifierAndVersion(): vo return null; }); - $clientBuilder->getClient()->captureMessage('test'); + (new ClientBuilder($options))->getClient()->captureMessage('test'); $this->assertTrue($callbackCalled, 'Callback not invoked, no assertions performed'); } @@ -272,8 +223,8 @@ public function testClientBuilderSetsSdkIdentifierAndVersion(): void { $callbackCalled = false; - $clientBuilder = new ClientBuilder(); - $clientBuilder->setBeforeSendCallback(function (Event $event) use (&$callbackCalled) { + $options = new Options(); + $options->setBeforeSendCallback(function (Event $event) use (&$callbackCalled) { $callbackCalled = true; $this->assertSame('sentry.test', $event->getSdkIdentifier()); @@ -282,7 +233,8 @@ public function testClientBuilderSetsSdkIdentifierAndVersion(): void return null; }); - $clientBuilder->setSdkIdentifier('sentry.test') + (new ClientBuilder($options)) + ->setSdkIdentifier('sentry.test') ->setSdkVersion('1.2.3-test') ->getClient() ->captureMessage('test'); @@ -295,7 +247,9 @@ public function testClientBuilderSetsSdkIdentifierAndVersion(): void */ public function testGetClientTogglesCompressionPluginInHttpClient(bool $enabled): void { - $builder = ClientBuilder::create(['enable_compression' => $enabled, 'dsn' => 'http://public:secret@example.com/sentry/1']); + $options = new Options(['dsn' => 'http://public:secret@example.com/sentry/1']); + $options->setEnableCompression($enabled); + $builder = new ClientBuilder($options); $builder->getClient(); $decoderPluginFound = false; @@ -318,6 +272,14 @@ public function getClientTogglesCompressionPluginInHttpClientDataProvider(): arr [false], ]; } + + public function testCreateWithNoOptionsIsTheSameAsDefaultOptions(): void + { + $this->assertEquals( + new ClientBuilder(new Options()), + ClientBuilder::create([]) + ); + } } final class StubIntegration implements IntegrationInterface diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 037f56a98..2f696ac63 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -202,14 +202,16 @@ public function testSendChecksBeforeSendOption(): void $transport->expects($this->never()) ->method('send'); - $client = ClientBuilder::create([ - 'dsn' => 'http://public:secret@example.com/1', - 'before_send' => function () use (&$beforeSendCalled) { - $beforeSendCalled = true; + $options = new Options(['dsn' => 'http://public:secret@example.com/1']); + $options->setBeforeSendCallback(function () use (&$beforeSendCalled) { + $beforeSendCalled = true; - return null; - }, - ])->setTransport($transport)->getClient(); + return null; + }); + + $client = (new ClientBuilder($options)) + ->setTransport($transport) + ->getClient(); $client->captureEvent([]); @@ -219,11 +221,14 @@ public function testSendChecksBeforeSendOption(): void /** * @dataProvider sampleRateAbsoluteDataProvider */ - public function testSampleRateAbsolute($options): void + public function testSampleRateAbsolute(float $sampleRate): void { $httpClient = new MockClient(); - $client = ClientBuilder::create($options) + $options = new Options(['dsn' => 'http://public:secret@example.com/1']); + $options->setSampleRate($sampleRate); + + $client = (new ClientBuilder($options)) ->setHttpClient($httpClient) ->getClient(); @@ -231,7 +236,7 @@ public function testSampleRateAbsolute($options): void $client->captureMessage('foobar'); } - switch ($options['sample_rate']) { + switch ($sampleRate) { case 0: $this->assertEmpty($httpClient->getRequests()); break; @@ -244,18 +249,8 @@ public function testSampleRateAbsolute($options): void public function sampleRateAbsoluteDataProvider(): array { return [ - [ - [ - 'dsn' => 'http://public:secret@example.com/1', - 'sample_rate' => 0, - ], - ], - [ - [ - 'dsn' => 'http://public:secret@example.com/1', - 'sample_rate' => 1, - ], - ], + 'sample rate 0' => [0], + 'sample rate 1' => [1], ]; } @@ -479,7 +474,7 @@ private function createEventFactory(): EventFactory return new EventFactory( $this->createMock(SerializerInterface::class), $this->createMock(RepresentationSerializerInterface::class), - $this->createMock(Options::class), + new Options(), 'sentry.sdk.identifier', '1.2.3' );