bug #48259 [FrameworkBundle] Allow configuring framework.exceptions with a config builder (MatTheCat)

nicolas-grekas · nicolas-grekas · commit 964bfe6720f0 · 2022-12-13T11:38:24.000+01:00
This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #48191 | License | MIT | Doc PR | symfony/symfony-docs#17455 It seems the `framework.exceptions` definition has been wrongly configured for XML. As a result it is not possible to set its value using a config builder. This pull request adds a “right” way to configure `framework.exceptions` using XML. The old one should be deprecated for `6.3` and removed for `7.0`. [The documentation](https://symfony.com/doc/current/reference/configuration/framework.html#exceptions) is also wrong and should be fixed (ideally after this pull request is merged). Guidance is requested to add a test (current failures unrelated) 🙏 Commits ------- 57e28f6bda [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder
diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php
@@ -1194,35 +1194,31 @@ private function addExceptionsSection(ArrayNodeDefinition $rootNode)
         $logLevels = (new \ReflectionClass(LogLevel::class))->getConstants();
 
         $rootNode
+            ->fixXmlConfig('exception')
             ->children()
                 ->arrayNode('exceptions')
                     ->info('Exception handling configuration')
+                    ->useAttributeAsKey('class')
                     ->beforeNormalization()
+                        // Handle legacy XML configuration
                         ->ifArray()
                         ->then(function (array $v): array {
                             if (!\array_key_exists('exception', $v)) {
                                 return $v;
                             }
 
-                            // Fix XML normalization
-                            $data = isset($v['exception'][0]) ? $v['exception'] : [$v['exception']];
-                            $exceptions = [];
-                            foreach ($data as $exception) {
-                                $config = [];
-                                if (\array_key_exists('log-level', $exception)) {
-                                    $config['log_level'] = $exception['log-level'];
-                                }
-                                if (\array_key_exists('status-code', $exception)) {
-                                    $config['status_code'] = $exception['status-code'];
-                                }
-                                $exceptions[$exception['name']] = $config;
+                            $v = $v['exception'];
+                            unset($v['exception']);
+
+                            foreach ($v as &$exception) {
+                                $exception['class'] = $exception['name'];
+                                unset($exception['name']);
                             }
 
-                            return $exceptions;
+                            return $v;
                         })
                     ->end()
                     ->prototype('array')
-                        ->fixXmlConfig('exception')
                         ->children()
                             ->scalarNode('log_level')
                                 ->info('The level of log message. Null to let Symfony decide.')
diff --git a/Resources/config/schema/symfony-1.0.xsd b/Resources/config/schema/symfony-1.0.xsd
@@ -30,6 +30,7 @@
             <xsd:element name="workflow" type="workflow" minOccurs="0" maxOccurs="unbounded" />
             <xsd:element name="php-errors" type="php-errors" minOccurs="0" maxOccurs="1" />
             <xsd:element name="exceptions" type="exceptions" minOccurs="0" maxOccurs="1" />
+            <xsd:element name="exception" type="new-exception" minOccurs="0" maxOccurs="unbounded" />
             <xsd:element name="lock" type="lock" minOccurs="0" maxOccurs="1" />
             <xsd:element name="messenger" type="messenger" minOccurs="0" maxOccurs="1" />
             <xsd:element name="http-client" type="http_client" minOccurs="0" maxOccurs="1" />
@@ -361,14 +362,29 @@
 
     <xsd:complexType name="exceptions">
         <xsd:sequence>
-            <xsd:element name="exception" type="exception" minOccurs="0" maxOccurs="unbounded" />
+            <xsd:element name="exception" type="old-exception" minOccurs="0" maxOccurs="unbounded" />
         </xsd:sequence>
     </xsd:complexType>
 
-    <xsd:complexType name="exception">
-        <xsd:attribute name="name" type="xsd:string" use="required" />
+    <xsd:complexType name="exception" abstract="true">
         <xsd:attribute name="log-level" type="xsd:string" />
-        <xsd:attribute name="status-code" type="xsd:int" />
+        <xsd:attribute name="status-code" type="xsd:integer" />
+    </xsd:complexType>
+
+    <xsd:complexType name="old-exception">
+        <xsd:complexContent>
+            <xsd:extension base="exception">
+                <xsd:attribute name="name" type="xsd:string" use="required" />
+            </xsd:extension>
+        </xsd:complexContent>
+    </xsd:complexType>
+
+    <xsd:complexType name="new-exception">
+        <xsd:complexContent>
+            <xsd:extension base="exception">
+                <xsd:attribute name="class" type="xsd:string" use="required" />
+            </xsd:extension>
+        </xsd:complexContent>
     </xsd:complexType>
 
     <xsd:complexType name="marking_store">
diff --git a/Tests/DependencyInjection/Fixtures/xml/exceptions.xml b/Tests/DependencyInjection/Fixtures/xml/exceptions.xml
@@ -6,11 +6,25 @@
         http://symfony.com/schema/dic/symfony https://symfony.com/schema/dic/symfony/symfony-1.0.xsd">
 
     <framework:config>
-        <framework:exceptions>
-            <framework:exception name="Symfony\Component\HttpKernel\Exception\BadRequestHttpException" log-level="info" status-code="422" />
-            <framework:exception name="Symfony\Component\HttpKernel\Exception\NotFoundHttpException" log-level="info" status-code="0" />
-            <framework:exception name="Symfony\Component\HttpKernel\Exception\ConflictHttpException" log-level="info" status-code="0" />
-            <framework:exception name="Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException" log-level="null" status-code="500" />
-        </framework:exceptions>
+        <framework:exception
+            class="Symfony\Component\HttpKernel\Exception\BadRequestHttpException"
+            log-level="info"
+            status-code="422"
+        />
+
+        <framework:exception
+            class="Symfony\Component\HttpKernel\Exception\NotFoundHttpException"
+            log-level="info"
+        />
+
+        <framework:exception
+            class="Symfony\Component\HttpKernel\Exception\ConflictHttpException"
+            log-level="info"
+        />
+
+        <framework:exception
+            class="Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException"
+            status-code="500"
+        />
     </framework:config>
 </container>
diff --git a/Tests/DependencyInjection/Fixtures/xml/exceptions_legacy.xml b/Tests/DependencyInjection/Fixtures/xml/exceptions_legacy.xml
@@ -0,0 +1,16 @@
+<?xml version="1.0" ?>
+<container xmlns="http://symfony.com/schema/dic/services"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xmlns:framework="http://symfony.com/schema/dic/symfony"
+    xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd
+        http://symfony.com/schema/dic/symfony https://symfony.com/schema/dic/symfony/symfony-1.0.xsd">
+
+    <framework:config>
+        <framework:exceptions>
+            <framework:exception name="Symfony\Component\HttpKernel\Exception\BadRequestHttpException" log-level="info" status-code="422" />
+            <framework:exception name="Symfony\Component\HttpKernel\Exception\NotFoundHttpException" log-level="info" status-code="0" />
+            <framework:exception name="Symfony\Component\HttpKernel\Exception\ConflictHttpException" log-level="info" status-code="0" />
+            <framework:exception name="Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException" log-level="null" status-code="500" />
+        </framework:exceptions>
+    </framework:config>
+</container>
diff --git a/Tests/DependencyInjection/FrameworkExtensionTest.php b/Tests/DependencyInjection/FrameworkExtensionTest.php
@@ -547,24 +547,34 @@ public function testExceptionsConfig()
     {
         $container = $this->createContainerFromFile('exceptions');
 
+        $configuration = $container->getDefinition('exception_listener')->getArgument(3);
+
         $this->assertSame([
-            \Symfony\Component\HttpKernel\Exception\BadRequestHttpException::class => [
-                'log_level' => 'info',
-                'status_code' => 422,
-            ],
-            \Symfony\Component\HttpKernel\Exception\NotFoundHttpException::class => [
-                'log_level' => 'info',
-                'status_code' => null,
-            ],
-            \Symfony\Component\HttpKernel\Exception\ConflictHttpException::class => [
-                'log_level' => 'info',
-                'status_code' => null,
-            ],
-            \Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException::class => [
-                'log_level' => null,
-                'status_code' => 500,
-            ],
-        ], $container->getDefinition('exception_listener')->getArgument(3));
+            \Symfony\Component\HttpKernel\Exception\BadRequestHttpException::class,
+            \Symfony\Component\HttpKernel\Exception\NotFoundHttpException::class,
+            \Symfony\Component\HttpKernel\Exception\ConflictHttpException::class,
+            \Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException::class,
+        ], array_keys($configuration));
+
+        $this->assertEqualsCanonicalizing([
+            'log_level' => 'info',
+            'status_code' => 422,
+        ], $configuration[\Symfony\Component\HttpKernel\Exception\BadRequestHttpException::class]);
+
+        $this->assertEqualsCanonicalizing([
+            'log_level' => 'info',
+            'status_code' => null,
+        ], $configuration[\Symfony\Component\HttpKernel\Exception\NotFoundHttpException::class]);
+
+        $this->assertEqualsCanonicalizing([
+            'log_level' => 'info',
+            'status_code' => null,
+        ], $configuration[\Symfony\Component\HttpKernel\Exception\ConflictHttpException::class]);
+
+        $this->assertEqualsCanonicalizing([
+            'log_level' => null,
+            'status_code' => 500,
+        ], $configuration[\Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException::class]);
     }
 
     public function testRouter()
diff --git a/Tests/DependencyInjection/XmlFrameworkExtensionTest.php b/Tests/DependencyInjection/XmlFrameworkExtensionTest.php
@@ -32,4 +32,38 @@ public function testMessengerMiddlewareFactoryErroneousFormat()
     {
         $this->markTestSkipped('XML configuration will not allow erroneous format.');
     }
+
+    public function testLegacyExceptionsConfig()
+    {
+        $container = $this->createContainerFromFile('exceptions_legacy');
+
+        $configuration = $container->getDefinition('exception_listener')->getArgument(3);
+
+        $this->assertSame([
+            \Symfony\Component\HttpKernel\Exception\BadRequestHttpException::class,
+            \Symfony\Component\HttpKernel\Exception\NotFoundHttpException::class,
+            \Symfony\Component\HttpKernel\Exception\ConflictHttpException::class,
+            \Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException::class,
+        ], array_keys($configuration));
+
+        $this->assertEqualsCanonicalizing([
+            'log_level' => 'info',
+            'status_code' => 422,
+        ], $configuration[\Symfony\Component\HttpKernel\Exception\BadRequestHttpException::class]);
+
+        $this->assertEqualsCanonicalizing([
+            'log_level' => 'info',
+            'status_code' => null,
+        ], $configuration[\Symfony\Component\HttpKernel\Exception\NotFoundHttpException::class]);
+
+        $this->assertEqualsCanonicalizing([
+            'log_level' => 'info',
+            'status_code' => null,
+        ], $configuration[\Symfony\Component\HttpKernel\Exception\ConflictHttpException::class]);
+
+        $this->assertEqualsCanonicalizing([
+            'log_level' => null,
+            'status_code' => 500,
+        ], $configuration[\Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException::class]);
+    }
 }
