Fix pipeline

.github/workflows/ci.yml

@@ -1,6 +1,6 @@
 name: CI
 
-on: [ push ]
+on: [ push, pull_request ]
 
 jobs:
   phpunit:

Classes/Helpers/DirectivesNormalizer.php

@@ -14,10 +14,10 @@
  *
  * We also cleanup of empty directives and entries here before further processing.
  */
-class DirectivesNormalizer
+final class DirectivesNormalizer
 {
     /**
-     * @param array<string, array<string|int, string|bool>> $directives
+     * @param array<string, ?array<string|int, string|bool>> $directives
      * @return string[][]
      * @throws DirectivesNormalizerException
      */
@@ -26,43 +26,50 @@ public static function normalize(array $directives, LoggerInterface $logger): ar
         $result = [];
         // directives e.g. script-src:
         foreach ($directives as $directive => $values) {
-            if(is_array($values) && sizeof($values) > 0) {
-                $normalizedValues = [];
-                $firstKeyType = null;
-                // values e.g. 'self', 'unsafe-inline' OR key-value pairs e.g. example.com: true
-                foreach ($values as $key => $value) {
-                    $keyType = gettype($key);
-                    $valueType = gettype($value);
-                    if ($firstKeyType === null) {
-                        $firstKeyType = $keyType;
-                    } else {
-                        if ($keyType !== $firstKeyType) {
-                            // we do not allow mixed key types -> this should be marked as an error in the IDE as well
-                            // as Flow should throw an exception here. But just to be sure, we add this check.
-                            throw new DirectivesNormalizerException('Directives must be defined as a list OR an object.');
-                        }
+            if (!is_array($values) || count($values) === 0) {
+                continue;
+            }
+
+            $normalizedValues = [];
+            $firstKeyType = null;
+            // values e.g. 'self', 'unsafe-inline' OR key-value pairs e.g. example.com: true
+            foreach ($values as $key => $value) {
+                if ($firstKeyType === null) {
+                    $firstKeyType = gettype($key);
+                } else {
+                    if (gettype($key) !== $firstKeyType) {
+                        // we do not allow mixed key types -> this should be marked as an error in the IDE as well
+                        // as Flow should throw an exception here. But just to be sure, we add this check.
+                        throw new DirectivesNormalizerException(
+                            'Directives must be defined as a list OR an object.'
+                        );
                     }
+                }
 
-                    if($keyType === 'integer' && $valueType === 'string' && !empty($value)) {
-                        // old configuration format using list
-                        $normalizedValues[] = $value;
-                        $logger->warning('Using list format for CSP directives is deprecated and will be removed in future versions. Please use key-value pairs with boolean values instead.');
-                    } elseif($keyType === 'string') {
-                        // new configuration format using key-value pairs
-                        if($valueType === 'boolean') {
-                            if($value === true && !empty($key)) {
-                                $normalizedValues[] = $key;
-                            }
-                        } else {
-                            // We chose a format similar to NodeType constraints yaml configuration.
-                            throw new DirectivesNormalizerException('When using keys in your yaml, the values must be boolean.');
+                if (is_int($key) && is_string($value) && trim($value) !== '') {
+                    // old configuration format using list
+                    $normalizedValues[] = $value;
+                    $logger->warning(
+                        'Using list format for CSP directives is deprecated and will be removed in future versions. Please use key-value pairs with boolean values instead.'
+                    );
+                } elseif (is_string($key)) {
+                    // new configuration format using key-value pairs
+                    if (is_bool($value)) {
+                        if ($value === true && trim($key) !== '') {
+                            $normalizedValues[] = $key;
                         }
+                        continue;
                     }
+
+                    // We chose a format similar to NodeType constraints yaml configuration.
+                    throw new DirectivesNormalizerException(
+                        'When using keys in your yaml, the values must be boolean.'
+                    );
                 }
-                if(!empty($normalizedValues)) {
-                    // we also clean up empty directives here
-                    $result[$directive] = $normalizedValues;
-                }
+            }
+            if ($normalizedValues !== []) {
+                // we also clean up empty directives here
+                $result[$directive] = $normalizedValues;
             }
         }
 

Classes/Helpers/TagHelper.php

@@ -9,7 +9,7 @@ class TagHelper
     public static function tagHasAttribute(
         string $tag,
         string $name,
-        string $value = null
+        ?string $value = null
     ): bool {
         $value = (string)$value;
         if ($value === '') {
@@ -36,10 +36,10 @@ public static function tagChangeAttributeValue(
         return preg_replace_callback(
             self::buildMatchAttributeNameWithAnyValueReqex($name),
             function ($hits) use ($newValue) {
-                return $hits["pre"].
-                    $hits["name"].
-                    $hits["glue"].
-                    $newValue.
+                return $hits["pre"] .
+                    $hits["name"] .
+                    $hits["glue"] .
+                    $newValue .
                     $hits["post"];
             },
             $tag
@@ -49,22 +49,22 @@ function ($hits) use ($newValue) {
     public static function tagAddAttribute(
         string $tag,
         string $name,
-        string $value = null
+        ?string $value = null
     ): string {
         return preg_replace_callback(
             self::buildMatchEndOfOpeningTagReqex(),
             function ($hits) use ($name, $value) {
                 if ((string)$value !== '') {
-                    return $hits["start"].
-                        ' '.
-                        $name.
-                        '="'.
-                        $value.
-                        '"'.
+                    return $hits["start"] .
+                        ' ' .
+                        $name .
+                        '="' .
+                        $value .
+                        '"' .
                         $hits["end"];
                 }
 
-                return $hits["start"].' '.$name.$hits["end"];
+                return $hits["start"] . ' ' . $name . $hits["end"];
             },
             $tag
         );
@@ -85,16 +85,16 @@ private static function buildMatchAttributeNameWithAnyValueReqex(string $name):
     {
         $nameQuoted = self::escapeReqexCharsInString($name);
 
-        return '/(?<pre><.*? )(?<name>'.
-            $nameQuoted.
+        return '/(?<pre><.*? )(?<name>' .
+            $nameQuoted .
             ')(?<glue>=")(?<value>.*?)(?<post>".*?>)/';
     }
 
     private static function buildMatchAttributeNameReqex(string $name): string
     {
         $nameQuoted = self::escapeReqexCharsInString($name);
 
-        return '/(?<pre><.*? )(?<name>'.$nameQuoted.')(?<post>.*?>)/';
+        return '/(?<pre><.*? )(?<name>' . $nameQuoted . ')(?<post>.*?>)/';
     }
 
     private static function buildMatchAttributeNameWithSpecificValueReqex(
@@ -104,10 +104,10 @@ private static function buildMatchAttributeNameWithSpecificValueReqex(
         $nameQuoted = self::escapeReqexCharsInString($name);
         $valueQuoted = self::escapeReqexCharsInString($value);
 
-        return '/(?<pre><.*? )(?<name>'.
-            $nameQuoted.
-            ')(?<glue>=")(?<value>'.
-            $valueQuoted.
+        return '/(?<pre><.*? )(?<name>' .
+            $nameQuoted .
+            ')(?<glue>=")(?<value>' .
+            $valueQuoted .
             ')(?<post>".*?>)/';
     }
 }

Tests/Unit/Factory/PolicyFactoryTest.php

@@ -6,6 +6,7 @@
 
 use Flowpack\ContentSecurityPolicy\Exceptions\InvalidDirectiveException;
 use Flowpack\ContentSecurityPolicy\Factory\PolicyFactory;
+use Flowpack\ContentSecurityPolicy\Helpers\DirectivesNormalizer;
 use Flowpack\ContentSecurityPolicy\Model\Directive;
 use Flowpack\ContentSecurityPolicy\Model\Nonce;
 use Flowpack\ContentSecurityPolicy\Model\Policy;
@@ -20,10 +21,12 @@
 #[UsesClass(Policy::class)]
 #[UsesClass(Directive::class)]
 #[UsesClass(InvalidDirectiveException::class)]
+#[UsesClass(DirectivesNormalizer::class)]
 class PolicyFactoryTest extends TestCase
 {
     private readonly LoggerInterface&MockObject $loggerMock;
     private readonly PolicyFactory $policyFactory;
+    private readonly ReflectionClass $policyFactoryReflection;
 
     protected function setUp(): void
     {
@@ -35,7 +38,10 @@ protected function setUp(): void
 
         $this->policyFactoryReflection = new ReflectionClass($this->policyFactory);
         $this->policyFactoryReflection->getProperty('logger')->setValue($this->policyFactory, $this->loggerMock);
-        $this->policyFactoryReflection->getProperty('throwInvalidDirectiveException')->setValue($this->policyFactory, true);
+        $this->policyFactoryReflection->getProperty('throwInvalidDirectiveException')->setValue(
+            $this->policyFactory,
+            true
+        );
     }
 
     public function testCreateShouldReturnPolicyAndMergeCustomWithDefaultDirective(): void
@@ -116,7 +122,10 @@ public function testCreateShouldFailWithInvalidDirective(): void
     public function testCreateShouldLogInvalidDirectiveInProduction(): void
     {
         $nonceMock = $this->createMock(Nonce::class);
-        $this->policyFactoryReflection->getProperty('throwInvalidDirectiveException')->setValue($this->policyFactory, false);
+        $this->policyFactoryReflection->getProperty('throwInvalidDirectiveException')->setValue(
+            $this->policyFactory,
+            false
+        );
 
         $defaultDirective = [
             'invalid' => [
@@ -131,7 +140,10 @@ public function testCreateShouldLogInvalidDirectiveInProduction(): void
         $this->loggerMock->expects($this->once())->method('critical');
         $this->policyFactory->create($nonceMock, $defaultDirective, $customDirective);
 
-        $this->policyFactoryReflection->getProperty('throwInvalidDirectiveException')->setValue($this->policyFactory, true);
+        $this->policyFactoryReflection->getProperty('throwInvalidDirectiveException')->setValue(
+            $this->policyFactory,
+            true
+        );
     }
 
     public function testCreateShouldReturnPolicyWithUniqueValues(): void

Tests/Unit/Helpers/DirectivesNormalizerTest.php

@@ -12,9 +12,11 @@
 use Psr\Log\LoggerInterface;
 
 #[CoversClass(DirectivesNormalizer::class)]
+#[CoversClass(DirectivesNormalizerException::class)]
 class DirectivesNormalizerTest extends TestCase
 {
     private readonly LoggerInterface&MockObject $loggerMock;
+
     protected function setUp(): void
     {
         parent::setUp();
@@ -167,6 +169,7 @@ public function testConfiguration(): void
 
         self::assertSame($expected, $actual);
 
+        // @phpstan-ignore argument.type
         $actual = DirectivesNormalizer::normalize([
             'base-uri',
             'script-src' => [],

Tests/Unit/Model/PolicyTest.php

@@ -11,7 +11,6 @@
 use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\Attributes\UsesClass;
 use PHPUnit\Framework\TestCase;
-use Psr\Log\LoggerInterface;
 use ReflectionClass;
 
 #[CoversClass(Policy::class)]
@@ -26,8 +25,6 @@ protected function setUp(): void
     {
         parent::setUp();
 
-        $this->loggerMock = $this->createMock(LoggerInterface::class);
-
         $this->policy = new Policy();
 
         $this->policyReflection = new ReflectionClass($this->policy);