Test QR codes logic when providing a color

config/constants.php

@@ -20,5 +20,5 @@ const DEFAULT_QR_CODE_FORMAT = 'png';
 const DEFAULT_QR_CODE_ERROR_CORRECTION = 'l';
 const DEFAULT_QR_CODE_ROUND_BLOCK_SIZE = true;
 const DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS = true;
-const DEFAULT_QR_CODE_COLOR = '#000'; // Black
+const DEFAULT_QR_CODE_COLOR = '#000000'; // Black
-const DEFAULT_QR_CODE_BG_COLOR = '#fff'; // White
+const DEFAULT_QR_CODE_BG_COLOR = '#ffffff'; // White

module/Core/src/Action/Model/QrCodeParams.php

@@ -13,8 +13,8 @@ use Endroid\QrCode\Writer\SvgWriter;
 use Endroid\QrCode\Writer\WriterInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Shlinkio\Shlink\Core\Options\QrCodeOptions;
-use Throwable;
 
+use function ctype_xdigit;
 use function hexdec;
 use function ltrim;
 use function max;
@@ -126,30 +126,23 @@ final class QrCodeParams
     private static function parseHexColor(string $hexColor, ?string $fallback): Color
     {
         $hexColor = ltrim($hexColor, '#');
-
+        if (! ctype_xdigit($hexColor) && $fallback !== null) {
-        try {
+            return self::parseHexColor($fallback, null);
-            if (strlen($hexColor) === 3) {
-                return new Color(
-                    (int) hexdec(substr($hexColor, 0, 1) . substr($hexColor, 0, 1)),
-                    (int) hexdec(substr($hexColor, 1, 1) . substr($hexColor, 1, 1)),
-                    (int) hexdec(substr($hexColor, 2, 1) . substr($hexColor, 2, 1)),
-                );
-            }
-
-            return new Color(
-                (int) hexdec(substr($hexColor, 0, 2)),
-                (int) hexdec(substr($hexColor, 2, 2)),
-                (int) hexdec(substr($hexColor, 4, 2)),
-            );
-        } catch (Throwable $e) {
-            // If a non-hex value was provided and an error occurs, fall back to the default color.
-            // Do not provide the fallback again this time, to avoid an infinite loop
-            if ($fallback !== null) {
-                return self::parseHexColor($fallback, null);
-            }
-
-            throw $e;
         }
+
+        if (strlen($hexColor) === 3) {
+            return new Color(
+                (int) hexdec(substr($hexColor, 0, 1) . substr($hexColor, 0, 1)),
+                (int) hexdec(substr($hexColor, 1, 1) . substr($hexColor, 1, 1)),
+                (int) hexdec(substr($hexColor, 2, 1) . substr($hexColor, 2, 1)),
+            );
+        }
+
+        return new Color(
+            (int) hexdec(substr($hexColor, 0, 2)),
+            (int) hexdec(substr($hexColor, 2, 2)),
+            (int) hexdec(substr($hexColor, 4, 2)),
+        );
     }
 
     private static function normalizeParam(string $param): string

module/Core/test/Action/QrCodeActionTest.php

@@ -24,9 +24,12 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier;
 use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface;
 
 use function getimagesizefromstring;
+use function hexdec;
 use function imagecolorat;
 use function imagecreatefromstring;
 
+use const Shlinkio\Shlink\DEFAULT_QR_CODE_COLOR;
+
 class QrCodeActionTest extends TestCase
 {
     private const WHITE = 0xFFFFFF;
@@ -46,10 +49,10 @@ class QrCodeActionTest extends TestCase
         $this->urlResolver->expects($this->once())->method('resolveEnabledShortUrl')->with(
             ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, ''),
         )->willThrowException(ShortUrlNotFoundException::fromNotFound(ShortUrlIdentifier::fromShortCodeAndDomain('')));
-        $delegate = $this->createMock(RequestHandlerInterface::class);
+        $handler = $this->createMock(RequestHandlerInterface::class);
-        $delegate->expects($this->once())->method('handle')->withAnyParameters()->willReturn(new Response());
+        $handler->expects($this->once())->method('handle')->withAnyParameters()->willReturn(new Response());
 
-        $this->action()->process((new ServerRequest())->withAttribute('shortCode', $shortCode), $delegate);
+        $this->action()->process((new ServerRequest())->withAttribute('shortCode', $shortCode), $handler);
     }
 
     #[Test]
@@ -59,10 +62,10 @@ class QrCodeActionTest extends TestCase
         $this->urlResolver->expects($this->once())->method('resolveEnabledShortUrl')->with(
             ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, ''),
         )->willReturn(ShortUrl::createFake());
-        $delegate = $this->createMock(RequestHandlerInterface::class);
+        $handler = $this->createMock(RequestHandlerInterface::class);
-        $delegate->expects($this->never())->method('handle');
+        $handler->expects($this->never())->method('handle');
 
-        $resp = $this->action()->process((new ServerRequest())->withAttribute('shortCode', $shortCode), $delegate);
+        $resp = $this->action()->process((new ServerRequest())->withAttribute('shortCode', $shortCode), $handler);
 
         self::assertInstanceOf(QrCodeResponse::class, $resp);
         self::assertEquals(200, $resp->getStatusCode());
@@ -78,10 +81,10 @@ class QrCodeActionTest extends TestCase
         $this->urlResolver->method('resolveEnabledShortUrl')->with(
             ShortUrlIdentifier::fromShortCodeAndDomain($code, ''),
         )->willReturn(ShortUrl::createFake());
-        $delegate = $this->createMock(RequestHandlerInterface::class);
+        $handler = $this->createMock(RequestHandlerInterface::class);
         $req = (new ServerRequest())->withAttribute('shortCode', $code)->withQueryParams($query);
 
-        $resp = $this->action(new QrCodeOptions(format: $defaultFormat))->process($req, $delegate);
+        $resp = $this->action(new QrCodeOptions(format: $defaultFormat))->process($req, $handler);
 
         self::assertEquals($expectedContentType, $resp->getHeaderLine('Content-Type'));
     }
@@ -108,9 +111,9 @@ class QrCodeActionTest extends TestCase
         $this->urlResolver->method('resolveEnabledShortUrl')->with(
             ShortUrlIdentifier::fromShortCodeAndDomain($code, ''),
         )->willReturn(ShortUrl::createFake());
-        $delegate = $this->createMock(RequestHandlerInterface::class);
+        $handler = $this->createMock(RequestHandlerInterface::class);
 
-        $resp = $this->action($defaultOptions)->process($req->withAttribute('shortCode', $code), $delegate);
+        $resp = $this->action($defaultOptions)->process($req->withAttribute('shortCode', $code), $handler);
         $result = getimagesizefromstring($resp->getBody()->__toString());
         self::assertNotFalse($result);
 
@@ -198,14 +201,14 @@ class QrCodeActionTest extends TestCase
         $this->urlResolver->method('resolveEnabledShortUrl')->with(
             ShortUrlIdentifier::fromShortCodeAndDomain($code, ''),
         )->willReturn(ShortUrl::withLongUrl('https://shlink.io'));
-        $delegate = $this->createMock(RequestHandlerInterface::class);
+        $handler = $this->createMock(RequestHandlerInterface::class);
 
-        $resp = $this->action($defaultOptions)->process($req, $delegate);
+        $resp = $this->action($defaultOptions)->process($req, $handler);
         $image = imagecreatefromstring($resp->getBody()->__toString());
         self::assertNotFalse($image);
 
         $color = imagecolorat($image, 1, 1);
-        self::assertEquals($color, $expectedColor);
+        self::assertEquals($expectedColor, $color);
     }
 
     public static function provideRoundBlockSize(): iterable
@@ -230,10 +233,47 @@ class QrCodeActionTest extends TestCase
         ];
     }
 
+    #[Test, DataProvider('provideColors')]
+    public function properColorsAreUsed(?string $queryColor, ?string $optionsColor, int $expectedColor): void
+    {
+        $code = 'abc123';
+        $req = ServerRequestFactory::fromGlobals()
+            ->withQueryParams(['color' => $queryColor])
+            ->withAttribute('shortCode', $code);
+
+        $this->urlResolver->method('resolveEnabledShortUrl')->with(
+            ShortUrlIdentifier::fromShortCodeAndDomain($code),
+        )->willReturn(ShortUrl::withLongUrl('https://shlink.io'));
+        $handler = $this->createMock(RequestHandlerInterface::class);
+
+        $resp = $this->action(
+            new QrCodeOptions(size: 250, roundBlockSize: false, color: $optionsColor ?? DEFAULT_QR_CODE_COLOR),
+        )->process($req, $handler);
+        $image = imagecreatefromstring($resp->getBody()->__toString());
+        self::assertNotFalse($image);
+
+        $resultingColor = imagecolorat($image, 1, 1);
+        self::assertEquals($expectedColor, $resultingColor);
+    }
+
+    public static function provideColors(): iterable
+    {
+        yield 'no query, no default' => [null, null, self::BLACK];
+        yield '6-char-query black' => ['000000', null, self::BLACK];
+        yield '6-char-query white' => ['ffffff', null, self::WHITE];
+        yield '6-char-query red' => ['ff0000', null, (int) hexdec('ff0000')];
+        yield '3-char-query black' => ['000', null, self::BLACK];
+        yield '3-char-query white' => ['fff', null, self::WHITE];
+        yield '3-char-query red' => ['f00', null, (int) hexdec('ff0000')];
+        yield '3-char-default red' => [null, 'f00', (int) hexdec('ff0000')];
+        yield 'invalid color in query' => ['zzzzzzzz', null, self::BLACK];
+        yield 'invalid color in query with default' => ['zzzzzzzz', 'aa88cc', self::BLACK];
+        yield 'invalid color in default' => [null, 'zzzzzzzz', self::BLACK];
+    }
+
     #[Test, DataProvider('provideEnabled')]
     public function qrCodeIsResolvedBasedOnOptions(bool $enabledForDisabledShortUrls): void
     {
-
         if ($enabledForDisabledShortUrls) {
             $this->urlResolver->expects($this->once())->method('resolvePublicShortUrl')->willThrowException(
                 ShortUrlNotFoundException::fromNotFound(ShortUrlIdentifier::fromShortCodeAndDomain('')),

phpunit.xml.dist

@@ -5,6 +5,7 @@
     bootstrap="./vendor/autoload.php"
     colors="true"
     cacheDirectory="build/.phpunit/unit-tests.cache"
+    displayDetailsOnTestsThatTriggerWarnings="true"
 >
     <testsuites>
         <testsuite name="Core">