From 5879d5cb8e36fa4fb38b979ed6af33ae14280dba Mon Sep 17 00:00:00 2001 From: HazA Date: Fri, 22 Feb 2019 14:06:26 +0100 Subject: [PATCH 1/5] ref: Make getScope private --- src/State/Hub.php | 18 +++++++++-------- src/State/HubInterface.php | 7 ------- tests/ClientTest.php | 10 ++++++++-- tests/SdkTest.php | 8 +++++++- tests/State/HubTest.php | 41 +++++++++++++++++++++++--------------- 5 files changed, 50 insertions(+), 34 deletions(-) diff --git a/src/State/Hub.php b/src/State/Hub.php index 6811b8062..a37cb5f3a 100644 --- a/src/State/Hub.php +++ b/src/State/Hub.php @@ -52,14 +52,6 @@ public function getClient(): ?ClientInterface return $this->getStackTop()->getClient(); } - /** - * {@inheritdoc} - */ - public function getScope(): Scope - { - return $this->getStackTop()->getScope(); - } - /** * {@inheritdoc} */ @@ -242,6 +234,16 @@ public function getIntegration(string $className): ?IntegrationInterface return null; } + /** + * Gets the scope bound to the top of the stack. + * + * @return Scope + */ + private function getScope(): Scope + { + return $this->getStackTop()->getScope(); + } + /** * Gets the topmost client/layer pair in the stack. * diff --git a/src/State/HubInterface.php b/src/State/HubInterface.php index d2569b0b9..af13d62e6 100644 --- a/src/State/HubInterface.php +++ b/src/State/HubInterface.php @@ -23,13 +23,6 @@ interface HubInterface */ public function getClient(): ?ClientInterface; - /** - * Gets the scope bound to the top of the stack. - * - * @return Scope - */ - public function getScope(): Scope; - /** * Gets the ID of the last captured event. * diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 87b4672f5..ff838cae4 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -274,8 +274,14 @@ public function testHandlingExceptionThrowingAnException(): void $client = new Client(new Options(), $transport, $this->createEventFactory()); - Hub::getCurrent()->bindClient($client); - $client->captureException($this->createCarelessExceptionWithStacktrace(), Hub::getCurrent()->getScope()); + $hub = Hub::getCurrent(); + $hub->bindClient($client); + + $class = new \ReflectionClass($hub); + $method = $class->getMethod('getScope'); + $method->setAccessible(true); + + $client->captureException($this->createCarelessExceptionWithStacktrace(), $method->invokeArgs($hub, [])); } /** diff --git a/tests/SdkTest.php b/tests/SdkTest.php index 9f9b0b013..9fe91f5a7 100644 --- a/tests/SdkTest.php +++ b/tests/SdkTest.php @@ -94,7 +94,13 @@ public function testAddBreadcrumb(): void addBreadcrumb($breadcrumb); - $this->assertSame([$breadcrumb], Hub::getCurrent()->getScope()->getBreadcrumbs()); + $hub = Hub::getCurrent(); + + $class = new \ReflectionClass($hub); + $method = $class->getMethod('getScope'); + $method->setAccessible(true); + + $this->assertSame([$breadcrumb], $method->invokeArgs($hub, [])->getBreadcrumbs()); } public function testWithScope(): void diff --git a/tests/State/HubTest.php b/tests/State/HubTest.php index 027f694c0..006c527d5 100644 --- a/tests/State/HubTest.php +++ b/tests/State/HubTest.php @@ -19,7 +19,7 @@ public function testConstructorCreatesScopeAutomatically(): void { $hub = new Hub(null, null); - $this->assertNotNull($hub->getScope()); + $this->assertNotNull(self::getScope($hub)); } public function testGetClient(): void @@ -36,7 +36,7 @@ public function testGetScope(): void $scope = new Scope(); $hub = new Hub($this->createMock(ClientInterface::class), $scope); - $this->assertSame($scope, $hub->getScope()); + $this->assertSame($scope, self::getScope($hub)); } public function testGetLastEventId(): void @@ -57,14 +57,14 @@ public function testPushScope(): void { $hub = new Hub($this->createMock(ClientInterface::class)); - $scope1 = $hub->getScope(); + $scope1 = self::getScope($hub); $client1 = $hub->getClient(); $scope2 = $hub->pushScope(); $client2 = $hub->getClient(); $this->assertNotSame($scope1, $scope2); - $this->assertSame($scope2, $hub->getScope()); + $this->assertSame($scope2, self::getScope($hub)); $this->assertSame($client1, $client2); $this->assertSame($client1, $hub->getClient()); } @@ -73,22 +73,22 @@ public function testPopScope(): void { $hub = new Hub($this->createMock(ClientInterface::class)); - $scope1 = $hub->getScope(); + $scope1 = self::getScope($hub); $client = $hub->getClient(); $scope2 = $hub->pushScope(); - $this->assertSame($scope2, $hub->getScope()); + $this->assertSame($scope2, self::getScope($hub)); $this->assertSame($client, $hub->getClient()); $this->assertTrue($hub->popScope()); - $this->assertSame($scope1, $hub->getScope()); + $this->assertSame($scope1, self::getScope($hub)); $this->assertSame($client, $hub->getClient()); $this->assertFalse($hub->popScope()); - $this->assertSame($scope1, $hub->getScope()); + $this->assertSame($scope1, self::getScope($hub)); $this->assertSame($client, $hub->getClient()); } @@ -97,7 +97,7 @@ public function testWithScope(): void $scope = new Scope(); $hub = new Hub($this->createMock(ClientInterface::class), $scope); - $this->assertSame($scope, $hub->getScope()); + $this->assertSame($scope, self::getScope($hub)); $callbackInvoked = false; @@ -116,7 +116,7 @@ public function testWithScope(): void } $this->assertTrue($callbackInvoked); - $this->assertSame($scope, $hub->getScope()); + $this->assertSame($scope, self::getScope($hub)); } public function testConfigureScope(): void @@ -134,7 +134,7 @@ public function testConfigureScope(): void }); $this->assertTrue($callbackInvoked); - $this->assertSame($scope, $hub->getScope()); + $this->assertSame($scope, self::getScope($hub)); } public function testBindClient(): void @@ -217,7 +217,7 @@ public function testAddBreadcrumb(): void $hub->addBreadcrumb($breadcrumb); - $this->assertSame([$breadcrumb], $hub->getScope()->getBreadcrumbs()); + $this->assertSame([$breadcrumb], self::getScope($hub)->getBreadcrumbs()); } public function testAddBreadcrumbDoesNothingIfMaxBreadcrumbsLimitIsZero(): void @@ -227,14 +227,14 @@ public function testAddBreadcrumbDoesNothingIfMaxBreadcrumbsLimitIsZero(): void $hub->addBreadcrumb(new Breadcrumb(Breadcrumb::LEVEL_ERROR, Breadcrumb::TYPE_ERROR, 'error_reporting')); - $this->assertEmpty($hub->getScope()->getBreadcrumbs()); + $this->assertEmpty(self::getScope($hub)->getBreadcrumbs()); } public function testAddBreadcrumbRespectsMaxBreadcrumbsLimit(): void { $client = ClientBuilder::create(['max_breadcrumbs' => 2])->getClient(); $hub = new Hub($client); - $scope = $hub->getScope(); + $scope = self::getScope($hub); $breadcrumb1 = new Breadcrumb(Breadcrumb::LEVEL_WARNING, Breadcrumb::TYPE_ERROR, 'error_reporting', 'foo'); $breadcrumb2 = new Breadcrumb(Breadcrumb::LEVEL_WARNING, Breadcrumb::TYPE_ERROR, 'error_reporting', 'bar'); @@ -260,7 +260,7 @@ public function testAddBreadcrumbDoesNothingWhenBeforeBreadcrumbCallbackReturnsN $hub->addBreadcrumb(new Breadcrumb(Breadcrumb::LEVEL_ERROR, Breadcrumb::TYPE_ERROR, 'error_reporting')); - $this->assertEmpty($hub->getScope()->getBreadcrumbs()); + $this->assertEmpty(self::getScope($hub)->getBreadcrumbs()); } public function testAddBreadcrumbStoresBreadcrumbReturnedByBeforeBreadcrumbCallback(): void @@ -276,7 +276,7 @@ public function testAddBreadcrumbStoresBreadcrumbReturnedByBeforeBreadcrumbCallb $hub->addBreadcrumb($breadcrumb1); - $this->assertSame([$breadcrumb2], $hub->getScope()->getBreadcrumbs()); + $this->assertSame([$breadcrumb2], self::getScope($hub)->getBreadcrumbs()); } public function testCaptureEvent(): void @@ -292,4 +292,13 @@ public function testCaptureEvent(): void $this->assertEquals('2b867534eead412cbdb882fd5d441690', $hub->captureEvent(['message' => 'test'])); } + + private static function getScope($hub): Scope + { + $class = new \ReflectionClass($hub); + $method = $class->getMethod('getScope'); + $method->setAccessible(true); + + return $method->invokeArgs($hub, []); + } } From 6ea8995f4969318802e3aafe8ed7addf01fddea4 Mon Sep 17 00:00:00 2001 From: HazA Date: Fri, 22 Feb 2019 19:26:51 +0100 Subject: [PATCH 2/5] ref: Codereview --- tests/State/HubTest.php | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/tests/State/HubTest.php b/tests/State/HubTest.php index 006c527d5..5fe11bd26 100644 --- a/tests/State/HubTest.php +++ b/tests/State/HubTest.php @@ -11,6 +11,7 @@ use Sentry\ClientInterface; use Sentry\Severity; use Sentry\State\Hub; +use Sentry\State\HubInterface; use Sentry\State\Scope; final class HubTest extends TestCase @@ -19,7 +20,7 @@ public function testConstructorCreatesScopeAutomatically(): void { $hub = new Hub(null, null); - $this->assertNotNull(self::getScope($hub)); + $this->assertNotNull($this->getScope($hub)); } public function testGetClient(): void @@ -36,7 +37,7 @@ public function testGetScope(): void $scope = new Scope(); $hub = new Hub($this->createMock(ClientInterface::class), $scope); - $this->assertSame($scope, self::getScope($hub)); + $this->assertSame($scope, $this->getScope($hub)); } public function testGetLastEventId(): void @@ -57,14 +58,14 @@ public function testPushScope(): void { $hub = new Hub($this->createMock(ClientInterface::class)); - $scope1 = self::getScope($hub); + $scope1 = $this->getScope($hub); $client1 = $hub->getClient(); $scope2 = $hub->pushScope(); $client2 = $hub->getClient(); $this->assertNotSame($scope1, $scope2); - $this->assertSame($scope2, self::getScope($hub)); + $this->assertSame($scope2, $this->getScope($hub)); $this->assertSame($client1, $client2); $this->assertSame($client1, $hub->getClient()); } @@ -73,22 +74,22 @@ public function testPopScope(): void { $hub = new Hub($this->createMock(ClientInterface::class)); - $scope1 = self::getScope($hub); + $scope1 = $this->getScope($hub); $client = $hub->getClient(); $scope2 = $hub->pushScope(); - $this->assertSame($scope2, self::getScope($hub)); + $this->assertSame($scope2, $this->getScope($hub)); $this->assertSame($client, $hub->getClient()); $this->assertTrue($hub->popScope()); - $this->assertSame($scope1, self::getScope($hub)); + $this->assertSame($scope1, $this->getScope($hub)); $this->assertSame($client, $hub->getClient()); $this->assertFalse($hub->popScope()); - $this->assertSame($scope1, self::getScope($hub)); + $this->assertSame($scope1, $this->getScope($hub)); $this->assertSame($client, $hub->getClient()); } @@ -97,7 +98,7 @@ public function testWithScope(): void $scope = new Scope(); $hub = new Hub($this->createMock(ClientInterface::class), $scope); - $this->assertSame($scope, self::getScope($hub)); + $this->assertSame($scope, $this->getScope($hub)); $callbackInvoked = false; @@ -116,7 +117,7 @@ public function testWithScope(): void } $this->assertTrue($callbackInvoked); - $this->assertSame($scope, self::getScope($hub)); + $this->assertSame($scope, $this->getScope($hub)); } public function testConfigureScope(): void @@ -134,7 +135,7 @@ public function testConfigureScope(): void }); $this->assertTrue($callbackInvoked); - $this->assertSame($scope, self::getScope($hub)); + $this->assertSame($scope, $this->getScope($hub)); } public function testBindClient(): void @@ -217,7 +218,7 @@ public function testAddBreadcrumb(): void $hub->addBreadcrumb($breadcrumb); - $this->assertSame([$breadcrumb], self::getScope($hub)->getBreadcrumbs()); + $this->assertSame([$breadcrumb], $this->getScope($hub)->getBreadcrumbs()); } public function testAddBreadcrumbDoesNothingIfMaxBreadcrumbsLimitIsZero(): void @@ -227,14 +228,14 @@ public function testAddBreadcrumbDoesNothingIfMaxBreadcrumbsLimitIsZero(): void $hub->addBreadcrumb(new Breadcrumb(Breadcrumb::LEVEL_ERROR, Breadcrumb::TYPE_ERROR, 'error_reporting')); - $this->assertEmpty(self::getScope($hub)->getBreadcrumbs()); + $this->assertEmpty($this->getScope($hub)->getBreadcrumbs()); } public function testAddBreadcrumbRespectsMaxBreadcrumbsLimit(): void { $client = ClientBuilder::create(['max_breadcrumbs' => 2])->getClient(); $hub = new Hub($client); - $scope = self::getScope($hub); + $scope = $this->getScope($hub); $breadcrumb1 = new Breadcrumb(Breadcrumb::LEVEL_WARNING, Breadcrumb::TYPE_ERROR, 'error_reporting', 'foo'); $breadcrumb2 = new Breadcrumb(Breadcrumb::LEVEL_WARNING, Breadcrumb::TYPE_ERROR, 'error_reporting', 'bar'); @@ -260,7 +261,7 @@ public function testAddBreadcrumbDoesNothingWhenBeforeBreadcrumbCallbackReturnsN $hub->addBreadcrumb(new Breadcrumb(Breadcrumb::LEVEL_ERROR, Breadcrumb::TYPE_ERROR, 'error_reporting')); - $this->assertEmpty(self::getScope($hub)->getBreadcrumbs()); + $this->assertEmpty($this->getScope($hub)->getBreadcrumbs()); } public function testAddBreadcrumbStoresBreadcrumbReturnedByBeforeBreadcrumbCallback(): void @@ -276,7 +277,7 @@ public function testAddBreadcrumbStoresBreadcrumbReturnedByBeforeBreadcrumbCallb $hub->addBreadcrumb($breadcrumb1); - $this->assertSame([$breadcrumb2], self::getScope($hub)->getBreadcrumbs()); + $this->assertSame([$breadcrumb2], $this->getScope($hub)->getBreadcrumbs()); } public function testCaptureEvent(): void @@ -293,7 +294,7 @@ public function testCaptureEvent(): void $this->assertEquals('2b867534eead412cbdb882fd5d441690', $hub->captureEvent(['message' => 'test'])); } - private static function getScope($hub): Scope + protected function getScope(HubInterface $hub): Scope { $class = new \ReflectionClass($hub); $method = $class->getMethod('getScope'); From 0534e8571b9c6935428ed360a7325d00e420e640 Mon Sep 17 00:00:00 2001 From: Alessandro Lai Date: Fri, 22 Feb 2019 22:08:43 +0100 Subject: [PATCH 3/5] Update changelog [skip CI] --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d43bb46e1..8dd862880 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,9 @@ # CHANGELOG ## Unreleased - -- ... +- Updated .gitattributes to reduce package footprint (#770) +- Use multibyte functions to handle unicode paths (#774) +- Remove `Hub::getScope()` to deny direct access to `Scope` instances (#776) ## 2.0.0-beta2 (2019-02-11) - Rename `SentryAuth` class to `SentryAuthentication` (#742) From 838d5297cafd8b80bab85828f68dbb35bb1b9f2c Mon Sep 17 00:00:00 2001 From: HazA Date: Fri, 22 Feb 2019 22:29:52 +0100 Subject: [PATCH 4/5] ref: CR --- tests/ClientTest.php | 3 +-- tests/SdkTest.php | 3 +-- tests/State/HubTest.php | 5 ++--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/ClientTest.php b/tests/ClientTest.php index ff838cae4..c812dec70 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -277,8 +277,7 @@ public function testHandlingExceptionThrowingAnException(): void $hub = Hub::getCurrent(); $hub->bindClient($client); - $class = new \ReflectionClass($hub); - $method = $class->getMethod('getScope'); + $method = new \ReflectionMethod($hub, 'getScope'); $method->setAccessible(true); $client->captureException($this->createCarelessExceptionWithStacktrace(), $method->invokeArgs($hub, [])); diff --git a/tests/SdkTest.php b/tests/SdkTest.php index 9fe91f5a7..809c510d3 100644 --- a/tests/SdkTest.php +++ b/tests/SdkTest.php @@ -96,8 +96,7 @@ public function testAddBreadcrumb(): void $hub = Hub::getCurrent(); - $class = new \ReflectionClass($hub); - $method = $class->getMethod('getScope'); + $method = new \ReflectionMethod($hub, 'getScope'); $method->setAccessible(true); $this->assertSame([$breadcrumb], $method->invokeArgs($hub, [])->getBreadcrumbs()); diff --git a/tests/State/HubTest.php b/tests/State/HubTest.php index 5fe11bd26..60eea4aa2 100644 --- a/tests/State/HubTest.php +++ b/tests/State/HubTest.php @@ -294,10 +294,9 @@ public function testCaptureEvent(): void $this->assertEquals('2b867534eead412cbdb882fd5d441690', $hub->captureEvent(['message' => 'test'])); } - protected function getScope(HubInterface $hub): Scope + private function getScope(HubInterface $hub): Scope { - $class = new \ReflectionClass($hub); - $method = $class->getMethod('getScope'); + $method = new \ReflectionMethod($hub, 'getScope'); $method->setAccessible(true); return $method->invokeArgs($hub, []); From 6d142d6c2f2da7a85bd44b2ebe47a63b30cc9468 Mon Sep 17 00:00:00 2001 From: Alessandro Lai Date: Fri, 22 Feb 2019 22:33:01 +0100 Subject: [PATCH 5/5] Simplify reflection calls --- tests/ClientTest.php | 2 +- tests/SdkTest.php | 2 +- tests/State/HubTest.php | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/ClientTest.php b/tests/ClientTest.php index c812dec70..07290a129 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -280,7 +280,7 @@ public function testHandlingExceptionThrowingAnException(): void $method = new \ReflectionMethod($hub, 'getScope'); $method->setAccessible(true); - $client->captureException($this->createCarelessExceptionWithStacktrace(), $method->invokeArgs($hub, [])); + $client->captureException($this->createCarelessExceptionWithStacktrace(), $method->invoke($hub)); } /** diff --git a/tests/SdkTest.php b/tests/SdkTest.php index 809c510d3..6d0f55e37 100644 --- a/tests/SdkTest.php +++ b/tests/SdkTest.php @@ -99,7 +99,7 @@ public function testAddBreadcrumb(): void $method = new \ReflectionMethod($hub, 'getScope'); $method->setAccessible(true); - $this->assertSame([$breadcrumb], $method->invokeArgs($hub, [])->getBreadcrumbs()); + $this->assertSame([$breadcrumb], $method->invoke($hub)->getBreadcrumbs()); } public function testWithScope(): void diff --git a/tests/State/HubTest.php b/tests/State/HubTest.php index 60eea4aa2..a95e73caf 100644 --- a/tests/State/HubTest.php +++ b/tests/State/HubTest.php @@ -299,6 +299,6 @@ private function getScope(HubInterface $hub): Scope $method = new \ReflectionMethod($hub, 'getScope'); $method->setAccessible(true); - return $method->invokeArgs($hub, []); + return $method->invoke($hub); } }