From 11a4702b10f4301e9e53b59e2aa00230037d9586 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 8 Oct 2024 08:57:51 +0200 Subject: [PATCH 01/11] Promote installer config options as env vars explicitly --- composer.json | 2 +- config/config.php | 5 ----- config/container.php | 13 ++++++++----- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/composer.json b/composer.json index 77fc84a8..5f7078df 100644 --- a/composer.json +++ b/composer.json @@ -45,7 +45,7 @@ "ramsey/uuid": "^4.7", "shlinkio/doctrine-specification": "^2.1.1", "shlinkio/shlink-common": "^6.3", - "shlinkio/shlink-config": "^3.0", + "shlinkio/shlink-config": "dev-main#76a96ee as 3.1", "shlinkio/shlink-event-dispatcher": "^4.1", "shlinkio/shlink-importer": "^5.3.2", "shlinkio/shlink-installer": "^9.2", diff --git a/config/config.php b/config/config.php index 78fc542a..40f88ea3 100644 --- a/config/config.php +++ b/config/config.php @@ -8,18 +8,13 @@ use Laminas\ConfigAggregator; use Laminas\Diactoros; use Mezzio; use Mezzio\ProblemDetails; -use Shlinkio\Shlink\Config\ConfigAggregator\EnvVarLoaderProvider; use function Shlinkio\Shlink\Config\env; -use function Shlinkio\Shlink\Core\enumValues; $isTestEnv = env('APP_ENV') === 'test'; return (new ConfigAggregator\ConfigAggregator( providers: [ - ! $isTestEnv - ? new EnvVarLoaderProvider('config/params/generated_config.php', enumValues(Core\Config\EnvVars::class)) - : new ConfigAggregator\ArrayProvider([]), Mezzio\ConfigProvider::class, Mezzio\Router\ConfigProvider::class, Mezzio\Router\FastRouteRouter\ConfigProvider::class, diff --git a/config/container.php b/config/container.php index 6e5172a4..13d64353 100644 --- a/config/container.php +++ b/config/container.php @@ -6,13 +6,20 @@ use Laminas\ServiceManager\ServiceManager; use Shlinkio\Shlink\Core\Config\EnvVars; use Symfony\Component\Lock; +use function Shlinkio\Shlink\Config\loadEnvVarsFromConfig; +use function Shlinkio\Shlink\Core\enumValues; + use const Shlinkio\Shlink\LOCAL_LOCK_FACTORY; chdir(dirname(__DIR__)); require 'vendor/autoload.php'; -// This is one of the first files loaded. Configure the timezone here +// Promote env vars from installer config +loadEnvVarsFromConfig('config/params/generated_config.php', enumValues(EnvVars::class)); + +// This is one of the first files loaded. Configure the timezone and memory limit here +ini_set('memory_limit', EnvVars::MEMORY_LIMIT->loadFromEnv('512M')); date_default_timezone_set(EnvVars::TIMEZONE->loadFromEnv(date_default_timezone_get())); // This class alias tricks the ConfigAbstractFactory to return Lock\Factory instances even with a different service name @@ -23,10 +30,6 @@ if (! class_exists(LOCAL_LOCK_FACTORY)) { return (static function (): ServiceManager { $config = require __DIR__ . '/config.php'; - - // Set memory limit right after loading config, to ensure installer config has been promoted as env vars - ini_set('memory_limit', EnvVars::MEMORY_LIMIT->loadFromEnv('512M')); - $container = new ServiceManager($config['dependencies']); $container->setService('config', $config); From 15b53ef43c92378fe67a94527b1b67d9ae35e679 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 8 Oct 2024 09:04:30 +0200 Subject: [PATCH 02/11] Update changelog --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0696a389..a350897f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,23 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). +## [Unreleased] +### Added +* *Nothing* + +### Changed +* [#2208](https://github.com/shlinkio/shlink/issues/2208) Explicitly promote installer config options as env vars, instead of as a side effect of loading the app config. + +### Deprecated +* *Nothing* + +### Removed +* *Nothing* + +### Fixed +* *Nothing* + + ## [4.2.1] - 2024-10-04 ### Added * [#2183](https://github.com/shlinkio/shlink/issues/2183) Redis database index to be used can now be specified in the connection URI path, and Shlink will honor it. From 1773e6ecae8283d6b2577b0039253739f7a08673 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 10 Oct 2024 11:28:31 +0200 Subject: [PATCH 03/11] Ensure query parameters are preserved verbatim when forwarded to long URL --- CHANGELOG.md | 2 +- config/autoload/rabbit.local.php.dist | 2 +- .../Helper/ShortUrlRedirectionBuilder.php | 6 ++- module/Core/test-api/Action/RedirectTest.php | 18 ++++++++ .../Helper/ShortUrlRedirectionBuilderTest.php | 41 +++++++++++-------- 5 files changed, 50 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a350897f..129e7a16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * *Nothing* ### Fixed -* *Nothing* +* [#2213](https://github.com/shlinkio/shlink/issues/2213) Fix spaces being replaced with underscores in query parameter names, when forwarded from short URL to long URL. ## [4.2.1] - 2024-10-04 diff --git a/config/autoload/rabbit.local.php.dist b/config/autoload/rabbit.local.php.dist index d19f82c6..37faf181 100644 --- a/config/autoload/rabbit.local.php.dist +++ b/config/autoload/rabbit.local.php.dist @@ -7,7 +7,7 @@ return [ 'rabbitmq' => [ 'enabled' => true, 'host' => 'shlink_rabbitmq', - 'port' => '5672', + 'port' => 5672, 'user' => 'rabbit', 'password' => 'rabbit', ], diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php index 768fb60e..4d801c6f 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php @@ -28,11 +28,15 @@ readonly class ShortUrlRedirectionBuilder implements ShortUrlRedirectionBuilderI ?string $extraPath = null, ): string { $uri = new Uri($this->redirectionResolver->resolveLongUrl($shortUrl, $request)); - $currentQuery = $request->getQueryParams(); $shouldForwardQuery = $shortUrl->forwardQuery(); $baseQueryString = $uri->getQuery(); $basePath = $uri->getPath(); + // Get current query by manually parsing query string, instead of using $request->getQueryParams(). + // That prevents some weird PHP logic in which some characters in param names are converted to ensure resulting + // names are valid variable names. + $currentQuery = Query::parse($request->getUri()->getQuery()); + return $uri ->withQuery($shouldForwardQuery ? $this->resolveQuery($baseQueryString, $currentQuery) : $baseQueryString) ->withPath($this->resolvePath($basePath, $extraPath)) diff --git a/module/Core/test-api/Action/RedirectTest.php b/module/Core/test-api/Action/RedirectTest.php index dc6ca174..36031da8 100644 --- a/module/Core/test-api/Action/RedirectTest.php +++ b/module/Core/test-api/Action/RedirectTest.php @@ -122,4 +122,22 @@ class RedirectTest extends ApiTestCase self::assertEquals(302, $response->getStatusCode()); self::assertEquals($longUrl, $response->getHeaderLine('Location')); } + + #[Test] + public function queryParametersAreProperlyForwarded(): void + { + $slug = 'forward-query-params'; + $this->callApiWithKey('POST', '/short-urls', [ + RequestOptions::JSON => [ + 'longUrl' => 'https://example.com', + 'customSlug' => $slug, + 'forwardQuery' => true, + ], + ]); + + $response = $this->callShortUrl($slug, [RequestOptions::QUERY => ['foo bar' => '123']]); + + self::assertEquals(302, $response->getStatusCode()); + self::assertEquals('https://example.com?foo%20bar=123', $response->getHeaderLine('Location')); + } } diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php index f1ffc012..afe83c8d 100644 --- a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php +++ b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\ShortUrl\Helper; use Laminas\Diactoros\ServerRequestFactory; +use Laminas\Diactoros\Uri; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\TestWith; @@ -53,62 +54,70 @@ class ShortUrlRedirectionBuilderTest extends TestCase public static function provideData(): iterable { - $request = static fn (array $query = []) => ServerRequestFactory::fromGlobals()->withQueryParams($query); + $request = static fn (string $query = '') => ServerRequestFactory::fromGlobals()->withUri( + (new Uri())->withQuery($query), + ); yield ['https://example.com/foo/bar?some=thing', $request(), null, true]; yield ['https://example.com/foo/bar?some=thing', $request(), null, null]; yield ['https://example.com/foo/bar?some=thing', $request(), null, false]; - yield ['https://example.com/foo/bar?some=thing&else', $request(['else' => null]), null, true]; - yield ['https://example.com/foo/bar?some=thing&foo=bar', $request(['foo' => 'bar']), null, true]; - yield ['https://example.com/foo/bar?some=thing&foo=bar', $request(['foo' => 'bar']), null, null]; - yield ['https://example.com/foo/bar?some=thing', $request(['foo' => 'bar']), null, false]; - yield ['https://example.com/foo/bar?some=thing&123=foo', $request(['123' => 'foo']), null, true]; - yield ['https://example.com/foo/bar?some=thing&456=foo', $request([456 => 'foo']), null, true]; - yield ['https://example.com/foo/bar?some=thing&456=foo', $request([456 => 'foo']), null, null]; - yield ['https://example.com/foo/bar?some=thing', $request([456 => 'foo']), null, false]; + yield ['https://example.com/foo/bar?some=thing&else', $request('else'), null, true]; + yield ['https://example.com/foo/bar?some=thing&foo=bar', $request('foo=bar'), null, true]; + yield ['https://example.com/foo/bar?some=thing&foo=bar', $request('foo=bar'), null, null]; + yield ['https://example.com/foo/bar?some=thing', $request('foo=bar'), null, false]; + yield ['https://example.com/foo/bar?some=thing&123=foo', $request('123=foo'), null, true]; + yield ['https://example.com/foo/bar?some=thing&456=foo', $request('456=foo'), null, true]; + yield ['https://example.com/foo/bar?some=thing&456=foo', $request('456=foo'), null, null]; + yield ['https://example.com/foo/bar?some=thing', $request('456=foo'), null, false]; yield [ 'https://example.com/foo/bar?some=overwritten&foo=bar', - $request(['foo' => 'bar', 'some' => 'overwritten']), + $request('foo=bar&some=overwritten'), null, true, ]; yield [ 'https://example.com/foo/bar?some=overwritten', - $request(['foobar' => 'notrack', 'some' => 'overwritten']), + $request('foobar=notrack&some=overwritten'), null, true, ]; yield [ 'https://example.com/foo/bar?some=overwritten', - $request(['foobar' => 'notrack', 'some' => 'overwritten']), + $request('foobar=notrack&some=overwritten'), null, null, ]; yield [ 'https://example.com/foo/bar?some=thing', - $request(['foobar' => 'notrack', 'some' => 'overwritten']), + $request('foobar=notrack&some=overwritten'), null, false, ]; yield ['https://example.com/foo/bar/something/else-baz?some=thing', $request(), '/something/else-baz', true]; yield [ 'https://example.com/foo/bar/something/else-baz?some=thing&hello=world', - $request(['hello' => 'world']), + $request('hello=world',), '/something/else-baz', true, ]; yield [ 'https://example.com/foo/bar/something/else-baz?some=thing&hello=world', - $request(['hello' => 'world']), + $request('hello=world',), '/something/else-baz', null, ]; yield [ 'https://example.com/foo/bar/something/else-baz?some=thing', - $request(['hello' => 'world']), + $request('hello=world',), '/something/else-baz', false, ]; + yield [ + 'https://example.com/foo/bar/something/else-baz?some=thing¶meter%20with%20spaces=world', + $request('parameter with spaces=world',), + '/something/else-baz', + true, + ]; } /** From be822646e4452d1522acb8d93191862499be15b5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 13 Oct 2024 09:49:34 +0200 Subject: [PATCH 04/11] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 129e7a16..4f73a443 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#2213](https://github.com/shlinkio/shlink/issues/2213) Fix spaces being replaced with underscores in query parameter names, when forwarded from short URL to long URL. +* [#2217](https://github.com/shlinkio/shlink/issues/2217) Fix docker image tag suffix being leaked to the version set inside Shlink, producing invalid SemVer version patterns. ## [4.2.1] - 2024-10-04 From 83e8801827201d04aecf0d0457918b7636139da4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 13 Oct 2024 12:24:59 +0200 Subject: [PATCH 05/11] Move env var default values to EnvVars enum --- composer.json | 2 +- config/autoload/cache.global.php | 4 +- config/autoload/entity-manager.global.php | 11 +-- config/autoload/matomo.global.php | 2 +- config/autoload/mercure.global.php | 2 +- config/autoload/qr-codes.global.php | 31 ++------ config/autoload/rabbit.global.php | 8 +- config/autoload/redirects.global.php | 9 +-- config/autoload/robots.global.php | 2 +- config/autoload/router.global.php | 2 +- config/autoload/routes.config.php | 2 +- config/autoload/tracking.global.php | 12 +-- config/autoload/url-shortener.global.php | 17 ++--- config/container.php | 4 +- docker/docker-entrypoint.sh | 5 ++ module/CLI/config/cli.config.php | 2 + module/CLI/config/dependencies.config.php | 2 + .../Command/Db/AbstractDatabaseCommand.php | 2 +- module/Core/src/Config/EnvVars.php | 76 ++++++++++++++++++- module/Core/test/Config/EnvVarsTest.php | 30 ++++++-- 20 files changed, 147 insertions(+), 78 deletions(-) diff --git a/composer.json b/composer.json index 5f7078df..2e3b02df 100644 --- a/composer.json +++ b/composer.json @@ -45,7 +45,7 @@ "ramsey/uuid": "^4.7", "shlinkio/doctrine-specification": "^2.1.1", "shlinkio/shlink-common": "^6.3", - "shlinkio/shlink-config": "dev-main#76a96ee as 3.1", + "shlinkio/shlink-config": "dev-main#5e43e96 as 3.1", "shlinkio/shlink-event-dispatcher": "^4.1", "shlinkio/shlink-importer": "^5.3.2", "shlinkio/shlink-installer": "^9.2", diff --git a/config/autoload/cache.global.php b/config/autoload/cache.global.php index 94a9a183..6ae37de7 100644 --- a/config/autoload/cache.global.php +++ b/config/autoload/cache.global.php @@ -6,7 +6,7 @@ use Shlinkio\Shlink\Core\Config\EnvVars; return (static function (): array { $redisServers = EnvVars::REDIS_SERVERS->loadFromEnv(); - $redis = ['pub_sub_enabled' => $redisServers !== null && EnvVars::REDIS_PUB_SUB_ENABLED->loadFromEnv(false)]; + $redis = ['pub_sub_enabled' => $redisServers !== null && EnvVars::REDIS_PUB_SUB_ENABLED->loadFromEnv()]; $cacheRedisBlock = $redisServers === null ? [] : [ 'redis' => [ 'servers' => $redisServers, @@ -16,7 +16,7 @@ return (static function (): array { return [ 'cache' => [ - 'namespace' => EnvVars::CACHE_NAMESPACE->loadFromEnv('Shlink'), + 'namespace' => EnvVars::CACHE_NAMESPACE->loadFromEnv(), ...$cacheRedisBlock, ], 'redis' => $redis, diff --git a/config/autoload/entity-manager.global.php b/config/autoload/entity-manager.global.php index c3aa6632..4338a8b6 100644 --- a/config/autoload/entity-manager.global.php +++ b/config/autoload/entity-manager.global.php @@ -23,11 +23,6 @@ return (static function (): array { $value = $envVar->loadFromEnv(); return $value === null ? null : (string) $value; }; - $resolveDefaultPort = static fn () => match ($driver) { - 'postgres' => '5432', - 'mssql' => '1433', - default => '3306', - }; $resolveCharset = static fn () => match ($driver) { // This does not determine charsets or collations in tables or columns, but the charset used in the data // flowing in the connection, so it has to match what has been set in the database. @@ -43,11 +38,11 @@ return (static function (): array { ], default => [ 'driver' => $resolveDriver(), - 'dbname' => EnvVars::DB_NAME->loadFromEnv('shlink'), + 'dbname' => EnvVars::DB_NAME->loadFromEnv(), 'user' => $readCredentialAsString(EnvVars::DB_USER), 'password' => $readCredentialAsString(EnvVars::DB_PASSWORD), - 'host' => EnvVars::DB_HOST->loadFromEnv(EnvVars::DB_UNIX_SOCKET->loadFromEnv()), - 'port' => EnvVars::DB_PORT->loadFromEnv($resolveDefaultPort()), + 'host' => EnvVars::DB_HOST->loadFromEnv(), + 'port' => EnvVars::DB_PORT->loadFromEnv(), 'unix_socket' => $isMysqlCompatible ? EnvVars::DB_UNIX_SOCKET->loadFromEnv() : null, 'charset' => $resolveCharset(), 'driverOptions' => $driver !== 'mssql' ? [] : [ diff --git a/config/autoload/matomo.global.php b/config/autoload/matomo.global.php index 120ad289..d7369ea7 100644 --- a/config/autoload/matomo.global.php +++ b/config/autoload/matomo.global.php @@ -7,7 +7,7 @@ use Shlinkio\Shlink\Core\Config\EnvVars; return [ 'matomo' => [ - 'enabled' => (bool) EnvVars::MATOMO_ENABLED->loadFromEnv(false), + 'enabled' => (bool) EnvVars::MATOMO_ENABLED->loadFromEnv(), 'base_url' => EnvVars::MATOMO_BASE_URL->loadFromEnv(), 'site_id' => EnvVars::MATOMO_SITE_ID->loadFromEnv(), 'api_token' => EnvVars::MATOMO_API_TOKEN->loadFromEnv(), diff --git a/config/autoload/mercure.global.php b/config/autoload/mercure.global.php index 67143919..c50e1f8e 100644 --- a/config/autoload/mercure.global.php +++ b/config/autoload/mercure.global.php @@ -15,7 +15,7 @@ return (static function (): array { 'mercure' => [ 'public_hub_url' => $publicUrl, - 'internal_hub_url' => EnvVars::MERCURE_INTERNAL_HUB_URL->loadFromEnv($publicUrl), + 'internal_hub_url' => EnvVars::MERCURE_INTERNAL_HUB_URL->loadFromEnv(), 'jwt_secret' => EnvVars::MERCURE_JWT_SECRET->loadFromEnv(), 'jwt_issuer' => 'Shlink', ], diff --git a/config/autoload/qr-codes.global.php b/config/autoload/qr-codes.global.php index 919beffa..23caadf2 100644 --- a/config/autoload/qr-codes.global.php +++ b/config/autoload/qr-codes.global.php @@ -4,32 +4,17 @@ declare(strict_types=1); use Shlinkio\Shlink\Core\Config\EnvVars; -use const Shlinkio\Shlink\DEFAULT_QR_CODE_BG_COLOR; -use const Shlinkio\Shlink\DEFAULT_QR_CODE_COLOR; -use const Shlinkio\Shlink\DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS; -use const Shlinkio\Shlink\DEFAULT_QR_CODE_ERROR_CORRECTION; -use const Shlinkio\Shlink\DEFAULT_QR_CODE_FORMAT; -use const Shlinkio\Shlink\DEFAULT_QR_CODE_MARGIN; -use const Shlinkio\Shlink\DEFAULT_QR_CODE_ROUND_BLOCK_SIZE; -use const Shlinkio\Shlink\DEFAULT_QR_CODE_SIZE; - return [ 'qr_codes' => [ - 'size' => (int) EnvVars::DEFAULT_QR_CODE_SIZE->loadFromEnv(DEFAULT_QR_CODE_SIZE), - 'margin' => (int) EnvVars::DEFAULT_QR_CODE_MARGIN->loadFromEnv(DEFAULT_QR_CODE_MARGIN), - 'format' => EnvVars::DEFAULT_QR_CODE_FORMAT->loadFromEnv(DEFAULT_QR_CODE_FORMAT), - 'error_correction' => EnvVars::DEFAULT_QR_CODE_ERROR_CORRECTION->loadFromEnv( - DEFAULT_QR_CODE_ERROR_CORRECTION, - ), - 'round_block_size' => (bool) EnvVars::DEFAULT_QR_CODE_ROUND_BLOCK_SIZE->loadFromEnv( - DEFAULT_QR_CODE_ROUND_BLOCK_SIZE, - ), - 'enabled_for_disabled_short_urls' => (bool) EnvVars::QR_CODE_FOR_DISABLED_SHORT_URLS->loadFromEnv( - DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS, - ), - 'color' => EnvVars::DEFAULT_QR_CODE_COLOR->loadFromEnv(DEFAULT_QR_CODE_COLOR), - 'bg_color' => EnvVars::DEFAULT_QR_CODE_BG_COLOR->loadFromEnv(DEFAULT_QR_CODE_BG_COLOR), + 'size' => (int) EnvVars::DEFAULT_QR_CODE_SIZE->loadFromEnv(), + 'margin' => (int) EnvVars::DEFAULT_QR_CODE_MARGIN->loadFromEnv(), + 'format' => EnvVars::DEFAULT_QR_CODE_FORMAT->loadFromEnv(), + 'error_correction' => EnvVars::DEFAULT_QR_CODE_ERROR_CORRECTION->loadFromEnv(), + 'round_block_size' => (bool) EnvVars::DEFAULT_QR_CODE_ROUND_BLOCK_SIZE->loadFromEnv(), + 'enabled_for_disabled_short_urls' => (bool) EnvVars::QR_CODE_FOR_DISABLED_SHORT_URLS->loadFromEnv(), + 'color' => EnvVars::DEFAULT_QR_CODE_COLOR->loadFromEnv(), + 'bg_color' => EnvVars::DEFAULT_QR_CODE_BG_COLOR->loadFromEnv(), 'logo_url' => EnvVars::DEFAULT_QR_CODE_LOGO_URL->loadFromEnv(), ], diff --git a/config/autoload/rabbit.global.php b/config/autoload/rabbit.global.php index fd8cda68..86e2dd6a 100644 --- a/config/autoload/rabbit.global.php +++ b/config/autoload/rabbit.global.php @@ -7,13 +7,13 @@ use Shlinkio\Shlink\Core\Config\EnvVars; return [ 'rabbitmq' => [ - 'enabled' => (bool) EnvVars::RABBITMQ_ENABLED->loadFromEnv(false), + 'enabled' => (bool) EnvVars::RABBITMQ_ENABLED->loadFromEnv(), 'host' => EnvVars::RABBITMQ_HOST->loadFromEnv(), - 'use_ssl' => (bool) EnvVars::RABBITMQ_USE_SSL->loadFromEnv(false), - 'port' => (int) EnvVars::RABBITMQ_PORT->loadFromEnv('5672'), + 'use_ssl' => (bool) EnvVars::RABBITMQ_USE_SSL->loadFromEnv(), + 'port' => (int) EnvVars::RABBITMQ_PORT->loadFromEnv(), 'user' => EnvVars::RABBITMQ_USER->loadFromEnv(), 'password' => EnvVars::RABBITMQ_PASSWORD->loadFromEnv(), - 'vhost' => EnvVars::RABBITMQ_VHOST->loadFromEnv('/'), + 'vhost' => EnvVars::RABBITMQ_VHOST->loadFromEnv(), ], ]; diff --git a/config/autoload/redirects.global.php b/config/autoload/redirects.global.php index 26a3c032..dbf026db 100644 --- a/config/autoload/redirects.global.php +++ b/config/autoload/redirects.global.php @@ -4,9 +4,6 @@ declare(strict_types=1); use Shlinkio\Shlink\Core\Config\EnvVars; -use const Shlinkio\Shlink\DEFAULT_REDIRECT_CACHE_LIFETIME; -use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE; - return [ 'not_found_redirects' => [ @@ -16,10 +13,8 @@ return [ ], 'redirects' => [ - 'redirect_status_code' => (int) EnvVars::REDIRECT_STATUS_CODE->loadFromEnv(DEFAULT_REDIRECT_STATUS_CODE->value), - 'redirect_cache_lifetime' => (int) EnvVars::REDIRECT_CACHE_LIFETIME->loadFromEnv( - DEFAULT_REDIRECT_CACHE_LIFETIME, - ), + 'redirect_status_code' => (int) EnvVars::REDIRECT_STATUS_CODE->loadFromEnv(), + 'redirect_cache_lifetime' => (int) EnvVars::REDIRECT_CACHE_LIFETIME->loadFromEnv(), ], ]; diff --git a/config/autoload/robots.global.php b/config/autoload/robots.global.php index 8954dc53..35235d72 100644 --- a/config/autoload/robots.global.php +++ b/config/autoload/robots.global.php @@ -7,7 +7,7 @@ namespace Shlinkio\Shlink\Core; return [ 'robots' => [ - 'allow-all-short-urls' => (bool) Config\EnvVars::ROBOTS_ALLOW_ALL_SHORT_URLS->loadFromEnv(false), + 'allow-all-short-urls' => (bool) Config\EnvVars::ROBOTS_ALLOW_ALL_SHORT_URLS->loadFromEnv(), 'user-agents' => splitByComma(Config\EnvVars::ROBOTS_USER_AGENTS->loadFromEnv()), ], diff --git a/config/autoload/router.global.php b/config/autoload/router.global.php index d13bf7d4..db389f3a 100644 --- a/config/autoload/router.global.php +++ b/config/autoload/router.global.php @@ -8,7 +8,7 @@ use Shlinkio\Shlink\Core\Config\EnvVars; return [ 'router' => [ - 'base_path' => EnvVars::BASE_PATH->loadFromEnv(''), + 'base_path' => EnvVars::BASE_PATH->loadFromEnv(), 'fastroute' => [ // Disabling config cache for cli, ensures it's never used for RoadRunner, and also that console diff --git a/config/autoload/routes.config.php b/config/autoload/routes.config.php index 6d072228..f7c6ce07 100644 --- a/config/autoload/routes.config.php +++ b/config/autoload/routes.config.php @@ -21,7 +21,7 @@ return (static function (): array { $overrideDomainMiddleware = Middleware\ShortUrl\OverrideDomainMiddleware::class; // TODO This should be based on config, not the env var - $shortUrlRouteSuffix = EnvVars::SHORT_URL_TRAILING_SLASH->loadFromEnv(false) ? '[/]' : ''; + $shortUrlRouteSuffix = EnvVars::SHORT_URL_TRAILING_SLASH->loadFromEnv() ? '[/]' : ''; return [ diff --git a/config/autoload/tracking.global.php b/config/autoload/tracking.global.php index 267bb76d..713a209e 100644 --- a/config/autoload/tracking.global.php +++ b/config/autoload/tracking.global.php @@ -11,25 +11,25 @@ return [ 'tracking' => [ // Tells if IP addresses should be anonymized before persisting, to fulfil data protection regulations // This applies only if IP address tracking is enabled - 'anonymize_remote_addr' => (bool) EnvVars::ANONYMIZE_REMOTE_ADDR->loadFromEnv(true), + 'anonymize_remote_addr' => (bool) EnvVars::ANONYMIZE_REMOTE_ADDR->loadFromEnv(), // Tells if visits to not-found URLs should be tracked. The disable_tracking option takes precedence - 'track_orphan_visits' => (bool) EnvVars::TRACK_ORPHAN_VISITS->loadFromEnv(true), + 'track_orphan_visits' => (bool) EnvVars::TRACK_ORPHAN_VISITS->loadFromEnv(), // A query param that, if provided, will disable tracking of one particular visit. Always takes precedence 'disable_track_param' => EnvVars::DISABLE_TRACK_PARAM->loadFromEnv(), // If true, visits will not be tracked at all - 'disable_tracking' => (bool) EnvVars::DISABLE_TRACKING->loadFromEnv(false), + 'disable_tracking' => (bool) EnvVars::DISABLE_TRACKING->loadFromEnv(), // If true, visits will be tracked, but neither the IP address, nor the location will be resolved - 'disable_ip_tracking' => (bool) EnvVars::DISABLE_IP_TRACKING->loadFromEnv(false), + 'disable_ip_tracking' => (bool) EnvVars::DISABLE_IP_TRACKING->loadFromEnv(), // If true, the referrer will not be tracked - 'disable_referrer_tracking' => (bool) EnvVars::DISABLE_REFERRER_TRACKING->loadFromEnv(false), + 'disable_referrer_tracking' => (bool) EnvVars::DISABLE_REFERRER_TRACKING->loadFromEnv(), // If true, the user agent will not be tracked - 'disable_ua_tracking' => (bool) EnvVars::DISABLE_UA_TRACKING->loadFromEnv(false), + 'disable_ua_tracking' => (bool) EnvVars::DISABLE_UA_TRACKING->loadFromEnv(), // A list of IP addresses, patterns or CIDR blocks from which tracking is disabled by default 'disable_tracking_from' => splitByComma(EnvVars::DISABLE_TRACKING_FROM->loadFromEnv()), diff --git a/config/autoload/url-shortener.global.php b/config/autoload/url-shortener.global.php index 43bd5a74..98b07ea2 100644 --- a/config/autoload/url-shortener.global.php +++ b/config/autoload/url-shortener.global.php @@ -5,29 +5,28 @@ declare(strict_types=1); use Shlinkio\Shlink\Core\Config\EnvVars; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlMode; -use const Shlinkio\Shlink\DEFAULT_SHORT_CODES_LENGTH; use const Shlinkio\Shlink\MIN_SHORT_CODES_LENGTH; return (static function (): array { $shortCodesLength = max( - (int) EnvVars::DEFAULT_SHORT_CODES_LENGTH->loadFromEnv(DEFAULT_SHORT_CODES_LENGTH), + (int) EnvVars::DEFAULT_SHORT_CODES_LENGTH->loadFromEnv(), MIN_SHORT_CODES_LENGTH, ); - $modeFromEnv = EnvVars::SHORT_URL_MODE->loadFromEnv(ShortUrlMode::STRICT->value); + $modeFromEnv = EnvVars::SHORT_URL_MODE->loadFromEnv(); $mode = ShortUrlMode::tryFrom($modeFromEnv) ?? ShortUrlMode::STRICT; return [ 'url_shortener' => [ 'domain' => [ // TODO Refactor this structure to url_shortener.schema and url_shortener.default_domain - 'schema' => ((bool) EnvVars::IS_HTTPS_ENABLED->loadFromEnv(true)) ? 'https' : 'http', - 'hostname' => EnvVars::DEFAULT_DOMAIN->loadFromEnv(''), + 'schema' => ((bool) EnvVars::IS_HTTPS_ENABLED->loadFromEnv()) ? 'https' : 'http', + 'hostname' => EnvVars::DEFAULT_DOMAIN->loadFromEnv(), ], 'default_short_codes_length' => $shortCodesLength, - 'auto_resolve_titles' => (bool) EnvVars::AUTO_RESOLVE_TITLES->loadFromEnv(true), - 'append_extra_path' => (bool) EnvVars::REDIRECT_APPEND_EXTRA_PATH->loadFromEnv(false), - 'multi_segment_slugs_enabled' => (bool) EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->loadFromEnv(false), - 'trailing_slash_enabled' => (bool) EnvVars::SHORT_URL_TRAILING_SLASH->loadFromEnv(false), + 'auto_resolve_titles' => (bool) EnvVars::AUTO_RESOLVE_TITLES->loadFromEnv(), + 'append_extra_path' => (bool) EnvVars::REDIRECT_APPEND_EXTRA_PATH->loadFromEnv(), + 'multi_segment_slugs_enabled' => (bool) EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->loadFromEnv(), + 'trailing_slash_enabled' => (bool) EnvVars::SHORT_URL_TRAILING_SLASH->loadFromEnv(), 'mode' => $mode, ], diff --git a/config/container.php b/config/container.php index 13d64353..3a1e3355 100644 --- a/config/container.php +++ b/config/container.php @@ -19,8 +19,8 @@ require 'vendor/autoload.php'; loadEnvVarsFromConfig('config/params/generated_config.php', enumValues(EnvVars::class)); // This is one of the first files loaded. Configure the timezone and memory limit here -ini_set('memory_limit', EnvVars::MEMORY_LIMIT->loadFromEnv('512M')); -date_default_timezone_set(EnvVars::TIMEZONE->loadFromEnv(date_default_timezone_get())); +ini_set('memory_limit', EnvVars::MEMORY_LIMIT->loadFromEnv()); +date_default_timezone_set(EnvVars::TIMEZONE->loadFromEnv()); // This class alias tricks the ConfigAbstractFactory to return Lock\Factory instances even with a different service name // It needs to be placed here as individual config files will not be loaded once config is cached diff --git a/docker/docker-entrypoint.sh b/docker/docker-entrypoint.sh index faa506a9..6dddf104 100644 --- a/docker/docker-entrypoint.sh +++ b/docker/docker-entrypoint.sh @@ -8,6 +8,11 @@ mkdir -p data/cache data/locks data/log data/proxies flags="--no-interaction --clear-db-cache" +# Read env vars through Shlink command, so that it applies the `_FILE` env var fallback logic +GEOLITE_LICENSE_KEY=$(bin/cli env-var:read GEOLITE_LICENSE_KEY) +SKIP_INITIAL_GEOLITE_DOWNLOAD=$(bin/cli env-var:read SKIP_INITIAL_GEOLITE_DOWNLOAD) +INITIAL_API_KEY=$(bin/cli env-var:read INITIAL_API_KEY) + # Skip downloading GeoLite2 db file if the license key env var was not defined or skipping was explicitly set if [ -z "${GEOLITE_LICENSE_KEY}" ] || [ "${SKIP_INITIAL_GEOLITE_DOWNLOAD}" = "true" ]; then flags="${flags} --skip-download-geolite" diff --git a/module/CLI/config/cli.config.php b/module/CLI/config/cli.config.php index e60bb2e1..8283a7b6 100644 --- a/module/CLI/config/cli.config.php +++ b/module/CLI/config/cli.config.php @@ -45,6 +45,8 @@ return [ Command\RedirectRule\ManageRedirectRulesCommand::class, Command\Integration\MatomoSendVisitsCommand::NAME => Command\Integration\MatomoSendVisitsCommand::class, + + Command\Config\ReadEnvVarCommand::NAME => Command\Config\ReadEnvVarCommand::class, ], ], diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 3853fd1d..0ee1a331 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -75,6 +75,8 @@ return [ Command\RedirectRule\ManageRedirectRulesCommand::class => ConfigAbstractFactory::class, Command\Integration\MatomoSendVisitsCommand::class => ConfigAbstractFactory::class, + + Command\Config\ReadEnvVarCommand::class => InvokableFactory::class, ], ], diff --git a/module/CLI/src/Command/Db/AbstractDatabaseCommand.php b/module/CLI/src/Command/Db/AbstractDatabaseCommand.php index f803c50c..a85cb999 100644 --- a/module/CLI/src/Command/Db/AbstractDatabaseCommand.php +++ b/module/CLI/src/Command/Db/AbstractDatabaseCommand.php @@ -17,7 +17,7 @@ abstract class AbstractDatabaseCommand extends AbstractLockedCommand public function __construct( LockFactory $locker, - private ProcessRunnerInterface $processRunner, + private readonly ProcessRunnerInterface $processRunner, PhpExecutableFinder $phpFinder, ) { parent::__construct($locker); diff --git a/module/Core/src/Config/EnvVars.php b/module/Core/src/Config/EnvVars.php index 01436967..d9fb1442 100644 --- a/module/Core/src/Config/EnvVars.php +++ b/module/Core/src/Config/EnvVars.php @@ -4,12 +4,27 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Config; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlMode; + +use function date_default_timezone_get; use function file_get_contents; +use function getenv; use function is_file; use function Shlinkio\Shlink\Config\env; use function Shlinkio\Shlink\Config\parseEnvVar; use function sprintf; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_COLOR; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_ERROR_CORRECTION; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_FORMAT; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_MARGIN; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_ROUND_BLOCK_SIZE; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_SIZE; +use const Shlinkio\Shlink\DEFAULT_REDIRECT_CACHE_LIFETIME; +use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE; +use const Shlinkio\Shlink\DEFAULT_SHORT_CODES_LENGTH; + enum EnvVars: string { case DELETE_SHORT_URL_THRESHOLD = 'DELETE_SHORT_URL_THRESHOLD'; @@ -74,10 +89,67 @@ enum EnvVars: string case ROBOTS_USER_AGENTS = 'ROBOTS_USER_AGENTS'; case TIMEZONE = 'TIMEZONE'; case MEMORY_LIMIT = 'MEMORY_LIMIT'; + case INITIAL_API_KEY = 'INITIAL_API_KEY'; + case SKIP_INITIAL_GEOLITE_DOWNLOAD = 'SKIP_INITIAL_GEOLITE_DOWNLOAD'; - public function loadFromEnv(mixed $default = null): mixed + public function loadFromEnv(): mixed { - return env($this->value) ?? $this->loadFromFileEnv() ?? $default; + return env($this->value) ?? $this->loadFromFileEnv() ?? $this->defaultValue(); + } + + private function defaultValue(): string|int|bool|null + { + return match ($this) { + self::MEMORY_LIMIT => '512M', + self::TIMEZONE => date_default_timezone_get(), + + self::DEFAULT_SHORT_CODES_LENGTH => DEFAULT_SHORT_CODES_LENGTH, + self::SHORT_URL_MODE => ShortUrlMode::STRICT->value, + self::IS_HTTPS_ENABLED, self::AUTO_RESOLVE_TITLES => true, + self::REDIRECT_APPEND_EXTRA_PATH, + self::MULTI_SEGMENT_SLUGS_ENABLED, + self::SHORT_URL_TRAILING_SLASH => false, + self::DEFAULT_DOMAIN, self::BASE_PATH => '', + self::CACHE_NAMESPACE => 'Shlink', + + self::REDIS_PUB_SUB_ENABLED, + self::MATOMO_ENABLED, + self::ROBOTS_ALLOW_ALL_SHORT_URLS => false, + + self::DB_NAME => 'shlink', + self::DB_HOST => self::DB_UNIX_SOCKET->loadFromEnv(), + self::DB_DRIVER => 'sqlite', + self::DB_PORT => match (self::DB_DRIVER->loadFromEnv()) { + 'postgres' => '5432', + 'mssql' => '1433', + default => '3306', + }, + + self::MERCURE_INTERNAL_HUB_URL => self::MERCURE_PUBLIC_HUB_URL->loadFromEnv(), + + self::DEFAULT_QR_CODE_SIZE, => DEFAULT_QR_CODE_SIZE, + self::DEFAULT_QR_CODE_MARGIN, => DEFAULT_QR_CODE_MARGIN, + self::DEFAULT_QR_CODE_FORMAT, => DEFAULT_QR_CODE_FORMAT, + self::DEFAULT_QR_CODE_ERROR_CORRECTION, => DEFAULT_QR_CODE_ERROR_CORRECTION, + self::DEFAULT_QR_CODE_ROUND_BLOCK_SIZE, => DEFAULT_QR_CODE_ROUND_BLOCK_SIZE, + self::QR_CODE_FOR_DISABLED_SHORT_URLS, => DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS, + self::DEFAULT_QR_CODE_COLOR, => DEFAULT_QR_CODE_COLOR, + + self::RABBITMQ_ENABLED, self::RABBITMQ_USE_SSL => false, + self::RABBITMQ_PORT => 5672, + self::RABBITMQ_VHOST => '/', + + self::REDIRECT_STATUS_CODE => DEFAULT_REDIRECT_STATUS_CODE->value, + self::REDIRECT_CACHE_LIFETIME => DEFAULT_REDIRECT_CACHE_LIFETIME, + + self::ANONYMIZE_REMOTE_ADDR, self::TRACK_ORPHAN_VISITS => true, + self::DISABLE_TRACKING, + self::DISABLE_IP_TRACKING, + self::DISABLE_REFERRER_TRACKING, + self::DISABLE_UA_TRACKING => false, + + default => null, + }; } /** diff --git a/module/Core/test/Config/EnvVarsTest.php b/module/Core/test/Config/EnvVarsTest.php index dd83393b..5af82c58 100644 --- a/module/Core/test/Config/EnvVarsTest.php +++ b/module/Core/test/Config/EnvVarsTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Core\Config; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\TestWith; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Config\EnvVars; @@ -44,19 +45,16 @@ class EnvVarsTest extends TestCase } #[Test, DataProvider('provideEnvVarsValues')] - public function expectedValueIsLoadedFromEnv(EnvVars $envVar, mixed $expected, mixed $default): void + public function expectedValueIsLoadedFromEnv(EnvVars $envVar, mixed $expected): void { - self::assertEquals($expected, $envVar->loadFromEnv($default)); + self::assertEquals($expected, $envVar->loadFromEnv()); } public static function provideEnvVarsValues(): iterable { - yield 'DB_NAME without default' => [EnvVars::DB_NAME, 'shlink', null]; - yield 'DB_NAME with default' => [EnvVars::DB_NAME, 'shlink', 'foobar']; - yield 'BASE_PATH without default' => [EnvVars::BASE_PATH, 'the_base_path', null]; - yield 'BASE_PATH with default' => [EnvVars::BASE_PATH, 'the_base_path', 'foobar']; - yield 'DB_DRIVER without default' => [EnvVars::DB_DRIVER, null, null]; - yield 'DB_DRIVER with default' => [EnvVars::DB_DRIVER, 'foobar', 'foobar']; + yield 'DB_NAME (is set)' => [EnvVars::DB_NAME, 'shlink']; + yield 'BASE_PATH (is set)' => [EnvVars::BASE_PATH, 'the_base_path']; + yield 'DB_DRIVER (has default)' => [EnvVars::DB_DRIVER, 'sqlite']; } #[Test] @@ -64,4 +62,20 @@ class EnvVarsTest extends TestCase { self::assertEquals('this_is_the_password', EnvVars::DB_PASSWORD->loadFromEnv()); } + + #[Test] + #[TestWith(['mysql', '3306'])] + #[TestWith(['maria', '3306'])] + #[TestWith(['postgres', '5432'])] + #[TestWith(['mssql', '1433'])] + public function defaultPortIsResolvedBasedOnDbDriver(string $dbDriver, string $expectedPort): void + { + putenv(EnvVars::DB_DRIVER->value . '=' . $dbDriver); + + try { + self::assertEquals($expectedPort, EnvVars::DB_PORT->loadFromEnv()); + } finally { + putenv(EnvVars::DB_DRIVER->value . '='); + } + } } From 14ba9fd6a4672b016b5c80e647f9b58f4e17c353 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 13 Oct 2024 12:25:19 +0200 Subject: [PATCH 06/11] Create command to return the value of an env var for current env --- .../src/Command/Config/ReadEnvVarCommand.php | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 module/CLI/src/Command/Config/ReadEnvVarCommand.php diff --git a/module/CLI/src/Command/Config/ReadEnvVarCommand.php b/module/CLI/src/Command/Config/ReadEnvVarCommand.php new file mode 100644 index 00000000..4eac16e4 --- /dev/null +++ b/module/CLI/src/Command/Config/ReadEnvVarCommand.php @@ -0,0 +1,58 @@ +setName(self::NAME) + ->setHidden() + ->setDescription('Display current value for an env var') + ->addArgument('envVar', InputArgument::REQUIRED, 'The env var to read'); + } + + protected function interact(InputInterface $input, OutputInterface $output): void + { + $io = new SymfonyStyle($input, $output); + $envVar = $input->getArgument('envVar'); + $validEnvVars = enumValues(EnvVars::class); + + if ($envVar === null) { + $envVar = $io->choice('Select the env var to read', $validEnvVars); + } + + if (! contains($envVar, $validEnvVars)) { + throw new InvalidArgumentException(sprintf('%s is not a valid Shlink environment variable', $envVar)); + } + + $input->setArgument('envVar', $envVar); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $envVar = $input->getArgument('envVar'); + $output->writeln(formatEnvVarValue(EnvVars::from($envVar)->loadFromEnv())); + + return ExitCode::EXIT_SUCCESS; + } +} From 1ec950ee1e2ad29e825a3ec662e86035c80b5cb3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 13 Oct 2024 12:32:59 +0200 Subject: [PATCH 07/11] Fix tests not properly unsetting env vars --- module/Core/src/Config/EnvVars.php | 1 - module/Core/test/Config/EnvVarsTest.php | 14 +++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/module/Core/src/Config/EnvVars.php b/module/Core/src/Config/EnvVars.php index d9fb1442..d1352f18 100644 --- a/module/Core/src/Config/EnvVars.php +++ b/module/Core/src/Config/EnvVars.php @@ -8,7 +8,6 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlMode; use function date_default_timezone_get; use function file_get_contents; -use function getenv; use function is_file; use function Shlinkio\Shlink\Config\env; use function Shlinkio\Shlink\Config\parseEnvVar; diff --git a/module/Core/test/Config/EnvVarsTest.php b/module/Core/test/Config/EnvVarsTest.php index 5af82c58..c2d8f8e7 100644 --- a/module/Core/test/Config/EnvVarsTest.php +++ b/module/Core/test/Config/EnvVarsTest.php @@ -25,9 +25,9 @@ class EnvVarsTest extends TestCase protected function tearDown(): void { - putenv(EnvVars::BASE_PATH->value . '='); - putenv(EnvVars::DB_NAME->value . '='); - putenv(EnvVars::DB_PASSWORD->value . '_FILE='); + putenv(EnvVars::BASE_PATH->value); + putenv(EnvVars::DB_NAME->value); + putenv(EnvVars::DB_PASSWORD->value . '_FILE'); } #[Test, DataProvider('provideExistingEnvVars')] @@ -38,9 +38,9 @@ class EnvVarsTest extends TestCase public static function provideExistingEnvVars(): iterable { - yield 'DB_NAME' => [EnvVars::DB_NAME, true]; - yield 'BASE_PATH' => [EnvVars::BASE_PATH, true]; - yield 'DB_DRIVER' => [EnvVars::DB_DRIVER, false]; + yield 'DB_NAME (is set)' => [EnvVars::DB_NAME, true]; + yield 'BASE_PATH (is set)' => [EnvVars::BASE_PATH, true]; + yield 'DB_DRIVER (has default)' => [EnvVars::DB_DRIVER, true]; yield 'DEFAULT_REGULAR_404_REDIRECT' => [EnvVars::DEFAULT_REGULAR_404_REDIRECT, false]; } @@ -75,7 +75,7 @@ class EnvVarsTest extends TestCase try { self::assertEquals($expectedPort, EnvVars::DB_PORT->loadFromEnv()); } finally { - putenv(EnvVars::DB_DRIVER->value . '='); + putenv(EnvVars::DB_DRIVER->value); } } } From d79f11eeb8ee3c99647c92e365aa84383bc16fe8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 13 Oct 2024 12:37:59 +0200 Subject: [PATCH 08/11] Add missing default value for DEFAULT_QR_CODE_BG_COLOR env var --- module/Core/src/Config/EnvVars.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/module/Core/src/Config/EnvVars.php b/module/Core/src/Config/EnvVars.php index d1352f18..f478048b 100644 --- a/module/Core/src/Config/EnvVars.php +++ b/module/Core/src/Config/EnvVars.php @@ -13,6 +13,7 @@ use function Shlinkio\Shlink\Config\env; use function Shlinkio\Shlink\Config\parseEnvVar; use function sprintf; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_BG_COLOR; use const Shlinkio\Shlink\DEFAULT_QR_CODE_COLOR; use const Shlinkio\Shlink\DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS; use const Shlinkio\Shlink\DEFAULT_QR_CODE_ERROR_CORRECTION; @@ -133,6 +134,7 @@ enum EnvVars: string self::DEFAULT_QR_CODE_ROUND_BLOCK_SIZE, => DEFAULT_QR_CODE_ROUND_BLOCK_SIZE, self::QR_CODE_FOR_DISABLED_SHORT_URLS, => DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS, self::DEFAULT_QR_CODE_COLOR, => DEFAULT_QR_CODE_COLOR, + self::DEFAULT_QR_CODE_BG_COLOR, => DEFAULT_QR_CODE_BG_COLOR, self::RABBITMQ_ENABLED, self::RABBITMQ_USE_SSL => false, self::RABBITMQ_PORT => 5672, From e17556a7ae86f6151c32e8ad8fca640cb45a8fd9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 14 Oct 2024 08:55:09 +0200 Subject: [PATCH 09/11] Add ReadEnvVarCommand test --- docker/docker-entrypoint.sh | 12 ++--- .../src/Command/Config/ReadEnvVarCommand.php | 12 ++++- .../Command/Config/ReadEnvVarCommandTest.php | 54 +++++++++++++++++++ 3 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 module/CLI/test/Command/Config/ReadEnvVarCommandTest.php diff --git a/docker/docker-entrypoint.sh b/docker/docker-entrypoint.sh index 6dddf104..e2cce68f 100644 --- a/docker/docker-entrypoint.sh +++ b/docker/docker-entrypoint.sh @@ -9,18 +9,18 @@ mkdir -p data/cache data/locks data/log data/proxies flags="--no-interaction --clear-db-cache" # Read env vars through Shlink command, so that it applies the `_FILE` env var fallback logic -GEOLITE_LICENSE_KEY=$(bin/cli env-var:read GEOLITE_LICENSE_KEY) -SKIP_INITIAL_GEOLITE_DOWNLOAD=$(bin/cli env-var:read SKIP_INITIAL_GEOLITE_DOWNLOAD) -INITIAL_API_KEY=$(bin/cli env-var:read INITIAL_API_KEY) +geolite_license_key=$(bin/cli env-var:read GEOLITE_LICENSE_KEY) +skip_initial_geolite_download=$(bin/cli env-var:read SKIP_INITIAL_GEOLITE_DOWNLOAD) +initial_api_key=$(bin/cli env-var:read INITIAL_API_KEY) # Skip downloading GeoLite2 db file if the license key env var was not defined or skipping was explicitly set -if [ -z "${GEOLITE_LICENSE_KEY}" ] || [ "${SKIP_INITIAL_GEOLITE_DOWNLOAD}" = "true" ]; then +if [ -z "${geolite_license_key}" ] || [ "${skip_initial_geolite_download}" = "true" ]; then flags="${flags} --skip-download-geolite" fi # If INITIAL_API_KEY was provided, create an initial API key -if [ -n "${INITIAL_API_KEY}" ]; then - flags="${flags} --initial-api-key=${INITIAL_API_KEY}" +if [ -n "${initial_api_key}" ]; then + flags="${flags} --initial-api-key=${initial_api_key}" fi php vendor/bin/shlink-installer init ${flags} diff --git a/module/CLI/src/Command/Config/ReadEnvVarCommand.php b/module/CLI/src/Command/Config/ReadEnvVarCommand.php index 4eac16e4..1f436eeb 100644 --- a/module/CLI/src/Command/Config/ReadEnvVarCommand.php +++ b/module/CLI/src/Command/Config/ReadEnvVarCommand.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\Config; +use Closure; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Core\Config\EnvVars; use Symfony\Component\Console\Command\Command; @@ -22,6 +23,15 @@ class ReadEnvVarCommand extends Command { public const NAME = 'env-var:read'; + /** @var Closure(string $envVar): mixed */ + private readonly Closure $loadEnvVar; + + public function __construct(?Closure $loadEnvVar = null) + { + $this->loadEnvVar = $loadEnvVar ?? static fn (string $envVar) => EnvVars::from($envVar)->loadFromEnv(); + parent::__construct(); + } + protected function configure(): void { $this @@ -51,7 +61,7 @@ class ReadEnvVarCommand extends Command protected function execute(InputInterface $input, OutputInterface $output): int { $envVar = $input->getArgument('envVar'); - $output->writeln(formatEnvVarValue(EnvVars::from($envVar)->loadFromEnv())); + $output->writeln(formatEnvVarValue(($this->loadEnvVar)($envVar))); return ExitCode::EXIT_SUCCESS; } diff --git a/module/CLI/test/Command/Config/ReadEnvVarCommandTest.php b/module/CLI/test/Command/Config/ReadEnvVarCommandTest.php new file mode 100644 index 00000000..c377cf86 --- /dev/null +++ b/module/CLI/test/Command/Config/ReadEnvVarCommandTest.php @@ -0,0 +1,54 @@ +commandTester = CliTestUtils::testerForCommand(new ReadEnvVarCommand(fn () => $this->envVarValue)); + } + + #[Test] + public function errorIsThrownIfProvidedEnvVarIsInvalid(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('foo is not a valid Shlink environment variable'); + + $this->commandTester->execute(['envVar' => 'foo']); + } + + #[Test] + public function valueIsPrintedIfProvidedEnvVarIsValid(): void + { + $this->commandTester->execute(['envVar' => EnvVars::BASE_PATH->value]); + $output = $this->commandTester->getDisplay(); + + self::assertStringNotContainsString('Select the env var to read', $output); + self::assertStringContainsString($this->envVarValue, $output); + } + + #[Test] + public function envVarNameIsRequestedIfArgumentIsMissing(): void + { + $this->commandTester->setInputs([EnvVars::BASE_PATH->value]); + $this->commandTester->execute([]); + $output = $this->commandTester->getDisplay(); + + self::assertStringContainsString('Select the env var to read', $output); + self::assertStringContainsString($this->envVarValue, $output); + } +} From d514f39a82faa42c3116568ea0dd2573df70f8f3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 14 Oct 2024 09:36:13 +0200 Subject: [PATCH 10/11] Update changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f73a443..07cae7fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). -## [Unreleased] +## [4.2.2] - 2024-10-14 ### Added * *Nothing* @@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#2213](https://github.com/shlinkio/shlink/issues/2213) Fix spaces being replaced with underscores in query parameter names, when forwarded from short URL to long URL. * [#2217](https://github.com/shlinkio/shlink/issues/2217) Fix docker image tag suffix being leaked to the version set inside Shlink, producing invalid SemVer version patterns. +* [#2212](https://github.com/shlinkio/shlink/issues/2212) Fix env vars read in docker entry point not properly falling back to their `_FILE` suffixed counterpart. ## [4.2.1] - 2024-10-04 From f118ea252c3c88075c411fb913cc212346bef315 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 14 Oct 2024 09:40:16 +0200 Subject: [PATCH 11/11] Depend on shlink-config 3.2 --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 2e3b02df..11109b64 100644 --- a/composer.json +++ b/composer.json @@ -45,7 +45,7 @@ "ramsey/uuid": "^4.7", "shlinkio/doctrine-specification": "^2.1.1", "shlinkio/shlink-common": "^6.3", - "shlinkio/shlink-config": "dev-main#5e43e96 as 3.1", + "shlinkio/shlink-config": "^3.2", "shlinkio/shlink-event-dispatcher": "^4.1", "shlinkio/shlink-importer": "^5.3.2", "shlinkio/shlink-installer": "^9.2",