From 689343d1c9e54c0a46fc47bb5dade59a436d17a9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 18 Feb 2024 21:02:35 +0100 Subject: [PATCH] Test QR codes logic when providing a color --- config/constants.php | 4 +- module/Core/src/Action/Model/QrCodeParams.php | 41 +++++------ module/Core/test/Action/QrCodeActionTest.php | 68 +++++++++++++++---- phpunit.xml.dist | 1 + 4 files changed, 74 insertions(+), 40 deletions(-) diff --git a/config/constants.php b/config/constants.php index 8d2e55cb..51ee0476 100644 --- a/config/constants.php +++ b/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_BG_COLOR = '#fff'; // White +const DEFAULT_QR_CODE_COLOR = '#000000'; // Black +const DEFAULT_QR_CODE_BG_COLOR = '#ffffff'; // White diff --git a/module/Core/src/Action/Model/QrCodeParams.php b/module/Core/src/Action/Model/QrCodeParams.php index 0fc1ebc6..2a3907cc 100644 --- a/module/Core/src/Action/Model/QrCodeParams.php +++ b/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, '#'); - - try { - 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 (! ctype_xdigit($hexColor) && $fallback !== null) { + 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)), + ); } private static function normalizeParam(string $param): string diff --git a/module/Core/test/Action/QrCodeActionTest.php b/module/Core/test/Action/QrCodeActionTest.php index 98e1e375..63f26b3f 100644 --- a/module/Core/test/Action/QrCodeActionTest.php +++ b/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); - $delegate->expects($this->once())->method('handle')->withAnyParameters()->willReturn(new Response()); + $handler = $this->createMock(RequestHandlerInterface::class); + $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); - $delegate->expects($this->never())->method('handle'); + $handler = $this->createMock(RequestHandlerInterface::class); + $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('')), diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 5abec3eb..9c85d2c4 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -5,6 +5,7 @@ bootstrap="./vendor/autoload.php" colors="true" cacheDirectory="build/.phpunit/unit-tests.cache" + displayDetailsOnTestsThatTriggerWarnings="true" >