Fix issue with deprecated option values not handled correctly; migrat…

…e services configuration to non-deprecated values
getsentry · Aug 24, 2017 · 5d961ce · 5d961ce
1 parent b80a9e7
commit 5d961ce
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,8 +7,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 
 ## 0.8.6 - 2017-08-24
+### Changed
+ - Migrate service definitons to non-deprecated option configuration values
 ### Fixed
  - Fix expected type of the `options.error_types` config value (scalar instead of array, discovered in #72)
+ - Fix handling of deprecated options value
 
 ## 0.8.5 - 2017-08-22
 ### Fixed

diff --git a/src/Sentry/SentryBundle/DependencyInjection/SentryExtension.php b/src/Sentry/SentryBundle/DependencyInjection/SentryExtension.php
@@ -61,16 +61,7 @@ private function checkConfigurationOnForInvalidSettings(array $config, Container
                 @trigger_error($deprecationMessage, E_USER_DEPRECATED);
             }
 
-            if ($config[$option] !== $default && $config['options'][$option] === $default) {
-                $container->setParameter('sentry.options.' . $option, $config[$option]);
-            }
-
-            // new option is used
-            if ($config[$option] === $default && $config['options'][$option] !== $default) {
-                $container->setParameter('sentry.' . $option, $config[$option]);
-            }
-
-            // both are used
+            // both are used, check if there are issues
             if (
                 $config[$option] !== $default
                 && $config['options'][$option] !== $default
@@ -83,6 +74,14 @@ private function checkConfigurationOnForInvalidSettings(array $config, Container
                 );
                 throw new InvalidConfigurationException($message);
             }
+
+            // new option is used, overrides old one
+            if ($config[$option] === $default && $config['options'][$option] !== $default) {
+                $config[$option] = $config['options'][$option];
+            }
+
+            $container->setParameter('sentry.' . $option, $config[$option]);
+            $container->setParameter('sentry.options.' . $option, $config[$option]);
         }
     }
 
@@ -96,6 +95,7 @@ private function getDeprecatedOptionsWithDefaults()
             'app_path' => '%kernel.root_dir%/..',
             'release' => null,
             'prefixes' => array('%kernel.root_dir%/..'),
+            'error_types' => null,
             'excluded_app_paths' => array(
                 '%kernel.root_dir%/../vendor',
                 '%kernel.root_dir%/../app/cache',

diff --git a/src/Sentry/SentryBundle/Resources/config/services.yml b/src/Sentry/SentryBundle/Resources/config/services.yml
@@ -1,13 +1,8 @@
 services:
     sentry.client:
         class: '%sentry.client%'
-        arguments: ['%sentry.dsn%', '%sentry.options%', '%sentry.error_types%']
+        arguments: ['%sentry.dsn%', '%sentry.options%', '%sentry.options.error_types%']
         calls:
-            - [setRelease, ['%sentry.release%']]
-            - [setEnvironment, ['%sentry.environment%']]
-            - [setAppPath, ['%sentry.app_path%']]
-            - [setExcludedAppPaths, ['%sentry.excluded_app_paths%']]
-            - [setPrefixes, ['%sentry.prefixes%']]
             - [install, []]
 
     sentry.exception_listener:

diff --git a/test/DependencyInjection/SentryExtensionTest.php b/test/DependencyInjection/SentryExtensionTest.php
@@ -70,6 +70,57 @@ public function test_that_it_uses_both_new_and_deprecated_values()
         );
     }
 
+    public function test_that_using_only_deprecated_values_doesnt_trigger_exception()
+    {
+        $container = $this->getContainer(
+            array(
+                static::CONFIG_ROOT => array(
+                    'app_path' => 'sentry/app/path',
+                    'error_types' => 'some-value',
+                ),
+            )
+        );
+
+        $this->assertSame('sentry/app/path', $container->getParameter('sentry.app_path'));
+        $this->assertSame('some-value', $container->getParameter('sentry.error_types'));
+    }
+
+    public function test_that_using_deprecated_values_works_on_both_options()
+    {
+        $container = $this->getContainer(
+            array(
+                static::CONFIG_ROOT => array(
+                    'app_path' => 'sentry/app/path',
+                    'error_types' => 'some-value',
+                ),
+            )
+        );
+
+        $this->assertSame('sentry/app/path', $container->getParameter('sentry.app_path'));
+        $this->assertSame('sentry/app/path', $container->getParameter('sentry.options.app_path'));
+        $this->assertSame('some-value', $container->getParameter('sentry.error_types'));
+        $this->assertSame('some-value', $container->getParameter('sentry.options.error_types'));
+    }
+
+    public function test_that_using_new_values_works_on_both_options()
+    {
+        $container = $this->getContainer(
+            array(
+                static::CONFIG_ROOT => array(
+                    'options' => array(
+                        'app_path' => 'sentry/app/path',
+                        'error_types' => 'some-value',
+                    ),
+                ),
+            )
+        );
+
+        $this->assertSame('sentry/app/path', $container->getParameter('sentry.app_path'));
+        $this->assertSame('sentry/app/path', $container->getParameter('sentry.options.app_path'));
+        $this->assertSame('some-value', $container->getParameter('sentry.error_types'));
+        $this->assertSame('some-value', $container->getParameter('sentry.options.error_types'));
+    }
+
     public function test_that_throws_exception_if_new_and_deprecated_values_dont_match()
     {
         $this->setExpectedException('Symfony\Component\Config\Definition\Exception\InvalidConfigurationException');