From ded0ceb1f441bb589c30014ae651b045ff9cbae6 Mon Sep 17 00:00:00 2001 From: Chris Date: Thu, 7 Mar 2019 21:09:50 -0500 Subject: [PATCH 1/8] Fixing config syntax Current syntax isn't compatible with Laravel config functions. --- config/sentry.php | 2 +- src/Sentry/Laravel/EventHandler.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/sentry.php b/config/sentry.php index ad01f9bb..61db0d81 100644 --- a/config/sentry.php +++ b/config/sentry.php @@ -7,5 +7,5 @@ // 'release' => trim(exec('git --git-dir ' . base_path('.git') . ' log --pretty="%h" -n1 HEAD')), // Capture bindings on SQL queries - 'breadcrumbs.sql_bindings' => true, + 'breadcrumbs' => array('sql_bindings' => true), ); diff --git a/src/Sentry/Laravel/EventHandler.php b/src/Sentry/Laravel/EventHandler.php index 06d2714c..11675879 100644 --- a/src/Sentry/Laravel/EventHandler.php +++ b/src/Sentry/Laravel/EventHandler.php @@ -65,7 +65,7 @@ class EventHandler */ public function __construct(array $config) { - $this->sqlBindings = isset($config['breadcrumbs.sql_bindings']) ? $config['breadcrumbs.sql_bindings'] === true : true; + $this->sqlBindings = isset($config['breadcrumbs']['sql_bindings']) ? $config['breadcrumbs']['sql_bindings'] === true : true; } /** From 621f49ca39773e6210359fa0b1cd251df3d0f684 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Mon, 11 Mar 2019 10:41:20 +0100 Subject: [PATCH 2/8] Fix test scaffolding with autoloading --- composer.json | 5 +++++ test/Sentry/EventHandlerTest.php | 2 ++ test/Sentry/ServiceProviderTest.php | 2 ++ test/Sentry/ServiceProviderWithoutDsnTest.php | 2 ++ test/bootstrap.php | 2 ++ 5 files changed, 13 insertions(+) diff --git a/composer.json b/composer.json index 7ce0a9ae..b56c1414 100644 --- a/composer.json +++ b/composer.json @@ -27,6 +27,11 @@ "Sentry\\Laravel\\": "src/" } }, + "autoload-dev": { + "psr-4": { + "Sentry\\Laravel\\Tests\\": "test/Sentry/" + } + }, "scripts": { "tests": [ "vendor/bin/phpunit --verbose" diff --git a/test/Sentry/EventHandlerTest.php b/test/Sentry/EventHandlerTest.php index add556aa..2e6db05f 100644 --- a/test/Sentry/EventHandlerTest.php +++ b/test/Sentry/EventHandlerTest.php @@ -1,5 +1,7 @@ Date: Mon, 11 Mar 2019 10:42:11 +0100 Subject: [PATCH 3/8] Update configuration code style --- config/sentry.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/config/sentry.php b/config/sentry.php index 61db0d81..c80e174b 100644 --- a/config/sentry.php +++ b/config/sentry.php @@ -6,6 +6,10 @@ // capture release as git sha // 'release' => trim(exec('git --git-dir ' . base_path('.git') . ' log --pretty="%h" -n1 HEAD')), - // Capture bindings on SQL queries - 'breadcrumbs' => array('sql_bindings' => true), + 'breadcrumbs' => [ + + // Capture bindings on SQL queries logged in breadcrumbs + 'sql_bindings' => true, + + ], ); From 19a227418395c24ccac3afcfbcde6cf3404dcf34 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Mon, 11 Mar 2019 10:42:40 +0100 Subject: [PATCH 4/8] Allow both new and old syntax for sql bindings config --- src/Sentry/Laravel/EventHandler.php | 8 ++++---- src/Sentry/Laravel/ServiceProvider.php | 6 +++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Sentry/Laravel/EventHandler.php b/src/Sentry/Laravel/EventHandler.php index 11675879..9aba7356 100644 --- a/src/Sentry/Laravel/EventHandler.php +++ b/src/Sentry/Laravel/EventHandler.php @@ -56,7 +56,7 @@ class EventHandler * * @var bool */ - private $sqlBindings; + private $recordSqlBindings; /** * EventHandler constructor. @@ -65,7 +65,7 @@ class EventHandler */ public function __construct(array $config) { - $this->sqlBindings = isset($config['breadcrumbs']['sql_bindings']) ? $config['breadcrumbs']['sql_bindings'] === true : true; + $this->recordSqlBindings = ($config['breadcrumbs']['sql_bindings'] ?? $config['breadcrumbs.sql_bindings'] ?? false) === true; } /** @@ -161,7 +161,7 @@ protected function queryHandler($query, $bindings, $time, $connectionName) { $data = array('connectionName' => $connectionName); - if ($this->sqlBindings) { + if ($this->recordSqlBindings) { $data['bindings'] = $bindings; } @@ -183,7 +183,7 @@ protected function queryExecutedHandler(QueryExecuted $query) { $data = array('connectionName' => $query->connectionName); - if ($this->sqlBindings) { + if ($this->recordSqlBindings) { $data['bindings'] = $query->bindings; } diff --git a/src/Sentry/Laravel/ServiceProvider.php b/src/Sentry/Laravel/ServiceProvider.php index 9a80b383..c137e73d 100644 --- a/src/Sentry/Laravel/ServiceProvider.php +++ b/src/Sentry/Laravel/ServiceProvider.php @@ -96,7 +96,11 @@ protected function configureAndRegisterClient(): void $userConfig = $this->app['config'][static::$abstract]; // We do not want this setting to hit our main client because it's Laravel specific - unset($userConfig['breadcrumbs.sql_bindings']); + unset( + $userConfig['breadcrumbs'], + // this is kept for backwards compatibilty and can be dropped in a breaking release + $userConfig['breadcrumbs.sql_bindings'] + ); $options = \array_merge( [ From c899071ba30a558440c7851c5ed061ab8a085509 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Mon, 11 Mar 2019 10:50:05 +0100 Subject: [PATCH 5/8] Add tests for the sql binding config key --- test/Sentry/SentryLaravelTestCase.php | 46 ++++++++++++ test/Sentry/SqlBindingsInBreadcrumbsTest.php | 72 +++++++++++++++++++ ...dingsInBreadcrumbsWithOldConfigKeyTest.php | 52 ++++++++++++++ 3 files changed, 170 insertions(+) create mode 100644 test/Sentry/SentryLaravelTestCase.php create mode 100644 test/Sentry/SqlBindingsInBreadcrumbsTest.php create mode 100644 test/Sentry/SqlBindingsInBreadcrumbsWithOldConfigKeyTest.php diff --git a/test/Sentry/SentryLaravelTestCase.php b/test/Sentry/SentryLaravelTestCase.php new file mode 100644 index 00000000..c15efa7b --- /dev/null +++ b/test/Sentry/SentryLaravelTestCase.php @@ -0,0 +1,46 @@ +resetApplicationWithConfig([ /* config */ ]);` helper method + ]; + + protected function getEnvironmentSetUp($app) + { + foreach ($this->setupConfig as $key => $value) { + $app['config']->set($key, $value); + } + } + + protected function getPackageProviders($app) + { + return [ + ServiceProvider::class, + ]; + } + + protected function resetApplicationWithConfig(array $config) + { + $this->setupConfig = $config; + + $this->refreshApplication(); + } + + protected function getScope(HubInterface $hub): Scope + { + $method = new \ReflectionMethod($hub, 'getScope'); + + $method->setAccessible(true); + + return $method->invoke($hub); + } +} diff --git a/test/Sentry/SqlBindingsInBreadcrumbsTest.php b/test/Sentry/SqlBindingsInBreadcrumbsTest.php new file mode 100644 index 00000000..af7624e5 --- /dev/null +++ b/test/Sentry/SqlBindingsInBreadcrumbsTest.php @@ -0,0 +1,72 @@ +set('sentry.dsn', 'http://publickey:secretkey@sentry.dev/123'); + + parent::getEnvironmentSetUp($app); + } + + public function testIsBound() + { + $this->assertTrue(app()->bound('sentry')); + $this->assertInstanceOf(Hub::class, app('sentry')); + } + + /** + * @depends testIsBound + */ + public function testSqlBindingsAreRecordedWhenEnabled() + { + $this->resetApplicationWithConfig([ + 'sentry.breadcrumbs.sql_bindings' => true, + ]); + + $this->assertTrue($this->app['config']->get('sentry.breadcrumbs.sql_bindings')); + + $this->app['events']->dispatch('illuminate.query', [ + $query = 'SELECT * FROM breadcrumbs WHERE bindings = ?;', + $bindings = ['1'], + 10, + 'test', + ]); + + /** @var \Sentry\Breadcrumb $lastBreadcrumb */ + $lastBreadcrumb = Arr::last($this->getScope(Hub::getCurrent())->getBreadcrumbs()); + + $this->assertEquals($query, $lastBreadcrumb->getMessage()); + $this->assertEquals($bindings, $lastBreadcrumb->getMetadata()['bindings']); + } + + /** + * @depends testIsBound + */ + public function testSqlBindingsAreRecordedWhenDisabled() + { + $this->resetApplicationWithConfig([ + 'sentry.breadcrumbs.sql_bindings' => false, + ]); + + $this->assertFalse($this->app['config']->get('sentry.breadcrumbs.sql_bindings')); + + $this->app['events']->dispatch('illuminate.query', [ + $query = 'SELECT * FROM breadcrumbs WHERE bindings <> ?;', + $bindings = ['1'], + 10, + 'test', + ]); + + /** @var \Sentry\Breadcrumb $lastBreadcrumb */ + $lastBreadcrumb = Arr::last($this->getScope(Hub::getCurrent())->getBreadcrumbs()); + + $this->assertEquals($query, $lastBreadcrumb->getMessage()); + $this->assertFalse(isset($lastBreadcrumb->getMetadata()['bindings'])); + } +} diff --git a/test/Sentry/SqlBindingsInBreadcrumbsWithOldConfigKeyTest.php b/test/Sentry/SqlBindingsInBreadcrumbsWithOldConfigKeyTest.php new file mode 100644 index 00000000..86790008 --- /dev/null +++ b/test/Sentry/SqlBindingsInBreadcrumbsWithOldConfigKeyTest.php @@ -0,0 +1,52 @@ +set('sentry.dsn', 'http://publickey:secretkey@sentry.dev/123'); + + $config = $app['config']->all(); + + $config['sentry']['breadcrumbs.sql_bindings'] = true; + + unset($config['sentry']['breadcrumbs']); + + $app['config'] = new Repository($config); + } + + public function testIsBound() + { + $this->assertTrue(app()->bound('sentry')); + $this->assertInstanceOf(Hub::class, app('sentry')); + } + + /** + * @depends testIsBound + */ + public function testSqlBindingsAreRecordedWhenEnabledByOldConfigKey() + { + $this->assertTrue($this->app['config']->get('sentry')['breadcrumbs.sql_bindings']); + + $this->app['events']->dispatch('illuminate.query', [ + $query = 'SELECT * FROM breadcrumbs WHERE bindings = ?;', + $bindings = ['1'], + 10, + 'test', + ]); + + /** @var \Sentry\Breadcrumb $lastBreadcrumb */ + $lastBreadcrumb = Arr::last($this->getScope(Hub::getCurrent())->getBreadcrumbs()); + + $this->assertEquals($query, $lastBreadcrumb->getMessage()); + $this->assertEquals($bindings, $lastBreadcrumb->getMetadata()['bindings']); + } +} From 36674d667c555dd4c0a3b8c67e167182f42fedec Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Mon, 11 Mar 2019 10:53:19 +0100 Subject: [PATCH 6/8] Update changelog --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 963f987e..78dfc007 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Fix the configuration syntax for the sql bindings in breadcrumbs configuration option to be compaitable with Laravel (#207) + ## 1.0.0 - This version requires `sentry/sentry` `>= 2.0` and also PHP `>= 7.1` @@ -8,10 +12,6 @@ Please see [Docs](https://docs.sentry.io/platforms/php/laravel/) for detailed usage. -## 0.12.0 (unreleased) - -- ... - ## 0.11.0 - Correctly merge the user config with the default configuration file (#163) From 1c4162afa76776e18a0cb04b7d7f6451587410c9 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Mon, 11 Mar 2019 11:58:12 +0100 Subject: [PATCH 7/8] Fix the tests --- .travis.yml | 2 +- src/Sentry/Laravel/ServiceProvider.php | 4 +++- test/Sentry/SentryLaravelTestCase.php | 10 ++++++++++ test/Sentry/SqlBindingsInBreadcrumbsTest.php | 13 ++++++++----- ...SqlBindingsInBreadcrumbsWithOldConfigKeyTest.php | 7 ++++--- 5 files changed, 26 insertions(+), 10 deletions(-) diff --git a/.travis.yml b/.travis.yml index 2d1bcdd1..80a473b5 100644 --- a/.travis.yml +++ b/.travis.yml @@ -48,7 +48,7 @@ jobs: before_install: - if [ "$USE_COMPOSER_JSON" != "1" ]; then composer remove friendsofphp/php-cs-fixer --dev --no-update; fi; - - if [ "$USE_COMPOSER_JSON" != "1" ]; then composer require laravel/framework:$LARAVEL illuminate/support:$LARAVEL orchestra/testbench:$TESTBENCH phpunit/phpunit:$PHPUNIT php-http/curl-client guzzlehttp/psr7 --no-update --no-interaction --dev; fi; + - if [ "$USE_COMPOSER_JSON" != "1" ]; then composer require laravel/framework:$LARAVEL illuminate/support:$LARAVEL orchestra/testbench:$TESTBENCH phpunit/phpunit:$PHPUNIT --no-update --no-interaction --dev; fi; install: - travis_retry composer install --no-suggest --no-interaction --prefer-dist --no-progress diff --git a/src/Sentry/Laravel/ServiceProvider.php b/src/Sentry/Laravel/ServiceProvider.php index c137e73d..9e144ce6 100644 --- a/src/Sentry/Laravel/ServiceProvider.php +++ b/src/Sentry/Laravel/ServiceProvider.php @@ -130,7 +130,9 @@ protected function configureAndRegisterClient(): void */ protected function hasDsnSet(): bool { - return !empty($this->app['config'][static::$abstract]['dsn'] ?? null); + $config = $this->app['config'][static::$abstract]; + + return !empty($config['dsn']); } /** diff --git a/test/Sentry/SentryLaravelTestCase.php b/test/Sentry/SentryLaravelTestCase.php index c15efa7b..8322d316 100644 --- a/test/Sentry/SentryLaravelTestCase.php +++ b/test/Sentry/SentryLaravelTestCase.php @@ -35,6 +35,16 @@ protected function resetApplicationWithConfig(array $config) $this->refreshApplication(); } + protected function dispatchLaravelEvent($event, array $payload = []) + { + $dispatcher = $this->app['events']; + + // Laravel 5.4+ uses the dispatch method to dispatch/fire events + return method_exists($dispatcher, 'dispatch') + ? $dispatcher->dispatch($event, $payload) + : $dispatcher->fire($event, $payload); + } + protected function getScope(HubInterface $hub): Scope { $method = new \ReflectionMethod($hub, 'getScope'); diff --git a/test/Sentry/SqlBindingsInBreadcrumbsTest.php b/test/Sentry/SqlBindingsInBreadcrumbsTest.php index af7624e5..1f5cc032 100644 --- a/test/Sentry/SqlBindingsInBreadcrumbsTest.php +++ b/test/Sentry/SqlBindingsInBreadcrumbsTest.php @@ -3,7 +3,6 @@ namespace Sentry\Laravel\Tests; use Sentry\State\Hub; -use Illuminate\Support\Arr; class SqlBindingsInBreadcrumbsTest extends SentryLaravelTestCase { @@ -31,15 +30,17 @@ public function testSqlBindingsAreRecordedWhenEnabled() $this->assertTrue($this->app['config']->get('sentry.breadcrumbs.sql_bindings')); - $this->app['events']->dispatch('illuminate.query', [ + $this->dispatchLaravelEvent('illuminate.query', [ $query = 'SELECT * FROM breadcrumbs WHERE bindings = ?;', $bindings = ['1'], 10, 'test', ]); + $breadcrumbs = $this->getScope(Hub::getCurrent())->getBreadcrumbs(); + /** @var \Sentry\Breadcrumb $lastBreadcrumb */ - $lastBreadcrumb = Arr::last($this->getScope(Hub::getCurrent())->getBreadcrumbs()); + $lastBreadcrumb = end($breadcrumbs); $this->assertEquals($query, $lastBreadcrumb->getMessage()); $this->assertEquals($bindings, $lastBreadcrumb->getMetadata()['bindings']); @@ -56,15 +57,17 @@ public function testSqlBindingsAreRecordedWhenDisabled() $this->assertFalse($this->app['config']->get('sentry.breadcrumbs.sql_bindings')); - $this->app['events']->dispatch('illuminate.query', [ + $this->dispatchLaravelEvent('illuminate.query', [ $query = 'SELECT * FROM breadcrumbs WHERE bindings <> ?;', $bindings = ['1'], 10, 'test', ]); + $breadcrumbs = $this->getScope(Hub::getCurrent())->getBreadcrumbs(); + /** @var \Sentry\Breadcrumb $lastBreadcrumb */ - $lastBreadcrumb = Arr::last($this->getScope(Hub::getCurrent())->getBreadcrumbs()); + $lastBreadcrumb = end($breadcrumbs); $this->assertEquals($query, $lastBreadcrumb->getMessage()); $this->assertFalse(isset($lastBreadcrumb->getMetadata()['bindings'])); diff --git a/test/Sentry/SqlBindingsInBreadcrumbsWithOldConfigKeyTest.php b/test/Sentry/SqlBindingsInBreadcrumbsWithOldConfigKeyTest.php index 86790008..fc48cf6f 100644 --- a/test/Sentry/SqlBindingsInBreadcrumbsWithOldConfigKeyTest.php +++ b/test/Sentry/SqlBindingsInBreadcrumbsWithOldConfigKeyTest.php @@ -3,7 +3,6 @@ namespace Sentry\Laravel\Tests; use Sentry\State\Hub; -use Illuminate\Support\Arr; use Illuminate\Config\Repository; class SqlBindingsInBreadcrumbsWithOldConfigKeyTest extends SentryLaravelTestCase @@ -36,15 +35,17 @@ public function testSqlBindingsAreRecordedWhenEnabledByOldConfigKey() { $this->assertTrue($this->app['config']->get('sentry')['breadcrumbs.sql_bindings']); - $this->app['events']->dispatch('illuminate.query', [ + $this->dispatchLaravelEvent('illuminate.query', [ $query = 'SELECT * FROM breadcrumbs WHERE bindings = ?;', $bindings = ['1'], 10, 'test', ]); + $breadcrumbs = $this->getScope(Hub::getCurrent())->getBreadcrumbs(); + /** @var \Sentry\Breadcrumb $lastBreadcrumb */ - $lastBreadcrumb = Arr::last($this->getScope(Hub::getCurrent())->getBreadcrumbs()); + $lastBreadcrumb = end($breadcrumbs); $this->assertEquals($query, $lastBreadcrumb->getMessage()); $this->assertEquals($bindings, $lastBreadcrumb->getMetadata()['bindings']); From a235b84e8769a856ab7f35ee3688d13ce4e3b3ae Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Fri, 15 Mar 2019 11:00:10 +0100 Subject: [PATCH 8/8] Fix typo --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78dfc007..8f931596 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- Fix the configuration syntax for the sql bindings in breadcrumbs configuration option to be compaitable with Laravel (#207) +- Fix the configuration syntax for the sql bindings in breadcrumbs configuration option to be compatible with Laravel (#207) ## 1.0.0