From 0d37eb65c90d4bfd0e8d3f9f5b660ade5370e838 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 11 Jan 2022 19:05:53 +0100 Subject: [PATCH 1/7] Used PhpFileProvider to load installer generated config --- config/config.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/config.php b/config/config.php index 330cd836..33c78896 100644 --- a/config/config.php +++ b/config/config.php @@ -37,7 +37,7 @@ return (new ConfigAggregator\ConfigAggregator([ new ConfigAggregator\PhpFileProvider('config/autoload/{{,*.}global,{,*.}local}.php'), env('APP_ENV') === 'test' ? new ConfigAggregator\PhpFileProvider('config/test/*.global.php') - : new ConfigAggregator\LaminasConfigProvider('config/params/generated_config.php'), + : new ConfigAggregator\PhpFileProvider('config/params/generated_config.php'), ], 'data/cache/app_config.php', [ Core\Config\BasePathPrefixer::class, ]))->getMergedConfig(); From c6f16b055807057426089c2b015f26abb7230bf6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 15 Jan 2022 11:34:17 +0100 Subject: [PATCH 2/7] Updated to latest installer with support for env vars --- composer.json | 2 +- config/autoload/entity-manager.global.php | 2 +- config/autoload/installer.global.php | 6 ++---- config/config.php | 17 +++++++++++++++-- module/Core/functions/functions.php | 19 +++++++++++++++++++ module/Core/src/Options/TrackingOptions.php | 12 +++++++----- 6 files changed, 45 insertions(+), 13 deletions(-) diff --git a/composer.json b/composer.json index 3213403b..7901dde3 100644 --- a/composer.json +++ b/composer.json @@ -51,7 +51,7 @@ "shlinkio/shlink-config": "^1.5", "shlinkio/shlink-event-dispatcher": "^2.3", "shlinkio/shlink-importer": "^2.5", - "shlinkio/shlink-installer": "dev-develop#a008036 as 7.0", + "shlinkio/shlink-installer": "dev-develop#ba32503 as 7.0", "shlinkio/shlink-ip-geolocation": "^2.2", "symfony/console": "^6.0", "symfony/filesystem": "^6.0", diff --git a/config/autoload/entity-manager.global.php b/config/autoload/entity-manager.global.php index 19113c22..c83db2a8 100644 --- a/config/autoload/entity-manager.global.php +++ b/config/autoload/entity-manager.global.php @@ -38,7 +38,7 @@ return (static function (): array { 'dbname' => env('DB_NAME', 'shlink'), 'user' => env('DB_USER'), 'password' => env('DB_PASSWORD'), - 'host' => env('DB_HOST', $driver === 'postgres' ? env('DB_UNIX_SOCKET') : null), + 'host' => env('DB_HOST', env('DB_UNIX_SOCKET')), 'port' => env('DB_PORT', $resolveDefaultPort()), 'unix_socket' => $isMysqlCompatible ? env('DB_UNIX_SOCKET') : null, 'charset' => $resolveCharset(), diff --git a/config/autoload/installer.global.php b/config/autoload/installer.global.php index 9259b061..81f9941a 100644 --- a/config/autoload/installer.global.php +++ b/config/autoload/installer.global.php @@ -18,8 +18,6 @@ return [ Option\Database\DatabaseUserConfigOption::class, Option\Database\DatabasePasswordConfigOption::class, Option\Database\DatabaseUnixSocketConfigOption::class, - Option\Database\DatabaseSqlitePathConfigOption::class, - Option\Database\DatabaseMySqlOptionsConfigOption::class, Option\UrlShortener\ShortDomainHostConfigOption::class, Option\UrlShortener\ShortDomainSchemaConfigOption::class, Option\Visit\VisitsWebhooksConfigOption::class, @@ -27,12 +25,12 @@ return [ Option\Redirect\BaseUrlRedirectConfigOption::class, Option\Redirect\InvalidShortUrlRedirectConfigOption::class, Option\Redirect\Regular404RedirectConfigOption::class, - Option\Visit\CheckVisitsThresholdConfigOption::class, Option\Visit\VisitsThresholdConfigOption::class, Option\BasePathConfigOption::class, Option\Worker\TaskWorkerNumConfigOption::class, Option\Worker\WebWorkerNumConfigOption::class, - Option\RedisConfigOption::class, + Option\Redis\RedisServersConfigOption::class, + Option\Redis\RedisSentinelServiceConfigOption::class, Option\UrlShortener\ShortCodeLengthOption::class, Option\Mercure\EnableMercureConfigOption::class, Option\Mercure\MercurePublicUrlConfigOption::class, diff --git a/config/config.php b/config/config.php index 33c78896..33cf602b 100644 --- a/config/config.php +++ b/config/config.php @@ -12,12 +12,25 @@ use Mezzio\Swoole; use function class_exists; use function Shlinkio\Shlink\Config\env; +use function Shlinkio\Shlink\Core\putNotYetDefinedEnv; use const PHP_SAPI; $isCli = PHP_SAPI === 'cli'; +$isTestEnv = env('APP_ENV') === 'test'; return (new ConfigAggregator\ConfigAggregator([ + ! $isTestEnv + ? new ConfigAggregator\ArrayProvider((new ConfigAggregator\ConfigAggregator([ + new ConfigAggregator\PhpFileProvider('config/params/generated_config.php'), + ], null, [function (array $generatedConfig) { + foreach ($generatedConfig as $envVar => $value) { + putNotYetDefinedEnv($envVar, $value); + } + + return []; + }]))->getMergedConfig()) + : new ConfigAggregator\ArrayProvider([]), Mezzio\ConfigProvider::class, Mezzio\Router\ConfigProvider::class, Mezzio\Router\FastRouteRouter\ConfigProvider::class, @@ -35,9 +48,9 @@ return (new ConfigAggregator\ConfigAggregator([ CLI\ConfigProvider::class, Rest\ConfigProvider::class, new ConfigAggregator\PhpFileProvider('config/autoload/{{,*.}global,{,*.}local}.php'), - env('APP_ENV') === 'test' + $isTestEnv ? new ConfigAggregator\PhpFileProvider('config/test/*.global.php') - : new ConfigAggregator\PhpFileProvider('config/params/generated_config.php'), + : new ConfigAggregator\ArrayProvider([]), ], 'data/cache/app_config.php', [ Core\Config\BasePathPrefixer::class, ]))->getMergedConfig(); diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index 567fde47..870f84c4 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -13,9 +13,13 @@ use PUGX\Shortid\Factory as ShortIdFactory; use Shlinkio\Shlink\Common\Util\DateRange; use function Functional\reduce_left; +use function implode; use function is_array; +use function is_scalar; use function print_r; +use function putenv; use function Shlinkio\Shlink\Common\buildDateRange; +use function Shlinkio\Shlink\Config\env; use function sprintf; use function str_repeat; @@ -116,3 +120,18 @@ function fieldWithUtf8Charset(FieldBuilder $field, array $emConfig, string $coll default => $field, }; } + +function putNotYetDefinedEnv(string $key, mixed $value): void +{ + $isArray = is_array($value); + if (!($isArray || is_scalar($value)) || env($key) !== null) { + return; + } + + $normalizedValue = $isArray ? implode(',', $value) : match ($value) { + true => 'true', + false => 'false', + default => $value, + }; + putenv(sprintf('%s=%s', $key, $normalizedValue)); +} diff --git a/module/Core/src/Options/TrackingOptions.php b/module/Core/src/Options/TrackingOptions.php index db74b61b..ba51b8e9 100644 --- a/module/Core/src/Options/TrackingOptions.php +++ b/module/Core/src/Options/TrackingOptions.php @@ -8,7 +8,9 @@ use Laminas\Stdlib\AbstractOptions; use function array_key_exists; use function explode; +use function Functional\map; use function is_array; +use function trim; class TrackingOptions extends AbstractOptions { @@ -108,10 +110,10 @@ class TrackingOptions extends AbstractOptions protected function setDisableTrackingFrom(string|array|null $disableTrackingFrom): void { - if (is_array($disableTrackingFrom)) { - $this->disableTrackingFrom = $disableTrackingFrom; - } else { - $this->disableTrackingFrom = $disableTrackingFrom === null ? [] : explode(',', $disableTrackingFrom); - } + $this->disableTrackingFrom = match (true) { + is_array($disableTrackingFrom) => $disableTrackingFrom, + $disableTrackingFrom === null => [], + default => map(explode(',', $disableTrackingFrom), static fn (string $value) => trim($value)), + }; } } From 91192a8a8f39099855e571688e8c03fe2cadf700 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 15 Jan 2022 16:06:24 +0100 Subject: [PATCH 3/7] Updated to latest shlink-installer and shlink-config, ensuring env vars are properly loaded --- composer.json | 4 ++-- config/config.php | 12 ++---------- module/Core/functions/functions.php | 19 ------------------- 3 files changed, 4 insertions(+), 31 deletions(-) diff --git a/composer.json b/composer.json index 7901dde3..41389109 100644 --- a/composer.json +++ b/composer.json @@ -48,10 +48,10 @@ "pugx/shortid-php": "^1.0", "ramsey/uuid": "^4.2", "shlinkio/shlink-common": "dev-main#cbcff58 as 4.4", - "shlinkio/shlink-config": "^1.5", + "shlinkio/shlink-config": "dev-main#483cf8a as 1.6", "shlinkio/shlink-event-dispatcher": "^2.3", "shlinkio/shlink-importer": "^2.5", - "shlinkio/shlink-installer": "dev-develop#ba32503 as 7.0", + "shlinkio/shlink-installer": "dev-develop#3ca7ec5 as 7.0", "shlinkio/shlink-ip-geolocation": "^2.2", "symfony/console": "^6.0", "symfony/filesystem": "^6.0", diff --git a/config/config.php b/config/config.php index 33cf602b..0adb0208 100644 --- a/config/config.php +++ b/config/config.php @@ -9,10 +9,10 @@ use Laminas\Diactoros; use Mezzio; use Mezzio\ProblemDetails; use Mezzio\Swoole; +use Shlinkio\Shlink\Config\ConfigAggregator\EnvVarLoaderProvider; use function class_exists; use function Shlinkio\Shlink\Config\env; -use function Shlinkio\Shlink\Core\putNotYetDefinedEnv; use const PHP_SAPI; @@ -21,15 +21,7 @@ $isTestEnv = env('APP_ENV') === 'test'; return (new ConfigAggregator\ConfigAggregator([ ! $isTestEnv - ? new ConfigAggregator\ArrayProvider((new ConfigAggregator\ConfigAggregator([ - new ConfigAggregator\PhpFileProvider('config/params/generated_config.php'), - ], null, [function (array $generatedConfig) { - foreach ($generatedConfig as $envVar => $value) { - putNotYetDefinedEnv($envVar, $value); - } - - return []; - }]))->getMergedConfig()) + ? new EnvVarLoaderProvider('config/params/generated_config.php') : new ConfigAggregator\ArrayProvider([]), Mezzio\ConfigProvider::class, Mezzio\Router\ConfigProvider::class, diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index 870f84c4..567fde47 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -13,13 +13,9 @@ use PUGX\Shortid\Factory as ShortIdFactory; use Shlinkio\Shlink\Common\Util\DateRange; use function Functional\reduce_left; -use function implode; use function is_array; -use function is_scalar; use function print_r; -use function putenv; use function Shlinkio\Shlink\Common\buildDateRange; -use function Shlinkio\Shlink\Config\env; use function sprintf; use function str_repeat; @@ -120,18 +116,3 @@ function fieldWithUtf8Charset(FieldBuilder $field, array $emConfig, string $coll default => $field, }; } - -function putNotYetDefinedEnv(string $key, mixed $value): void -{ - $isArray = is_array($value); - if (!($isArray || is_scalar($value)) || env($key) !== null) { - return; - } - - $normalizedValue = $isArray ? implode(',', $value) : match ($value) { - true => 'true', - false => 'false', - default => $value, - }; - putenv(sprintf('%s=%s', $key, $normalizedValue)); -} From a1366f0ef14ea5eae03fe968d543f6c32c8fef80 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 15 Jan 2022 16:52:48 +0100 Subject: [PATCH 4/7] Exposed port 8888 on php container for experimentation --- docker-compose.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docker-compose.yml b/docker-compose.yml index 3d552f9a..5e2a3bd6 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -18,6 +18,8 @@ services: build: context: . dockerfile: ./data/infra/php.Dockerfile + ports: + - '8888:8888' volumes: - ./:/home/shlink/www - ./data/infra/php.ini:/usr/local/etc/php/php.ini From 199d976e3d050478d656cc9bfeb91f77920e7e41 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 15 Jan 2022 16:55:57 +0100 Subject: [PATCH 5/7] Updated changelog and upgrading doc --- CHANGELOG.md | 1 + UPGRADE.md | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31f225b5..0a33bd5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#1283](https://github.com/shlinkio/shlink/issues/1283) Changed behavior of `DELETE_SHORT_URL_THRESHOLD` env var, disabling the feature if a value was not provided. * [#1300](https://github.com/shlinkio/shlink/issues/1300) Changed default ordering for short URLs list, returning always from newest to oldest. * [#1299](https://github.com/shlinkio/shlink/issues/1299) Updated to the latest base docker images, based in PHP 8.1.1, and bumped openswoole to v4.9.1. +* [#1282](https://github.com/shlinkio/shlink/issues/1282) Env vars now have precedence over installer options. ### Deprecated * [#1315](https://github.com/shlinkio/shlink/issues/1315) Deprecated `GET /tags?withStats=true` endpoint. Use `GET /tags/stats` instead. diff --git a/UPGRADE.md b/UPGRADE.md index 53ff49f3..7988276d 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -38,6 +38,7 @@ * `VALIDATE_URLS`: There's no replacement. URLs are not validated, unless explicitly requested during creation or edition. * The next env vars behavior has changed: * `DELETE_SHORT_URL_THRESHOLD`: Now, if this env var is not provided, the "visits threshold" won't be checked at all when deleting short URLs. Make sure you explicitly provide a value if you want to enable this feature. +* Environment variables now have precedence over configuration set via the installer tool. ### Other changes From f53305c404025bd67e896f1b315c716d3cd6753e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 15 Jan 2022 17:17:22 +0100 Subject: [PATCH 6/7] Added ADR for the changes to load env vars on top of installer config --- ...-have-precedence-over-installer-options.md | 50 +++++++++++++++++++ docs/adr/README.md | 1 + 2 files changed, 51 insertions(+) create mode 100644 docs/adr/2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md diff --git a/docs/adr/2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md b/docs/adr/2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md new file mode 100644 index 00000000..63231bc1 --- /dev/null +++ b/docs/adr/2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md @@ -0,0 +1,50 @@ +# Update env vars behavior to have precedence over installer options + +* Status: Accepted +* Date: 2022-01-15 + +## Context and problem statement + +Shlink supports providing configuration via the installer tool that generates a config file that gets merged with the rest of the config, or via environment variables. + +It is potentially possible to combine both, but if you do so, you will find out the installer tool config has precedence over env vars, which is not very intuitive. + +A [Twitter survey](https://twitter.com/shlinkio/status/1480614855006732289) has also showed up all participants also found the behavior should be the opposite. + +## Considered option + +* Move the logic to read env vars to another config file which always overrides installer options. +* Move the logic to read env vars to a config post-processor which overrides config dynamically, only if the appropriate env var had been defined. +* Make the installer generate a config file which also includes the logic to load env vars on it. +* Make the installer no longer generate the config structure, and instead generate a map with env vars and their values. Then Shlink would define those env vars if not defined already. + +## Decision outcome + +The most viable option was finally to re-think the installer tool, and make it generate a map of env vars and their values. + +Then Shlink reads this as the first config file, which sets the values as env vars if not yet defined, and later on, the values are read as usual wherever needed. + +## Pros and Cons of the Options + +### Read all env vars in a single config file + +* Bad: This option had to be discarded, as it would always override the installer config no matter what. + +### Read all env vars in a config post-processor + +* Good because it would not require any change in the installer. +* Bad because it requires moving all env var reading logic somewhere else, while having it together with their contextual config is quite convenient. + +### Make the installer generate a config file which also reads env vars + +* Good because it would not require changing Shlink. +* Bad because it requires looking for a new way to generate the installer config. +* Bad because it would mean reading the env vars in multiple places. + +### Re-think the installer to no longer generate internal config, and instead, just define values for regular env vars + +* Bad because it requires changes both in Shlink and the installer. +* Bad because it's more error-prone, and the option with higher chances to introduce a regression. +* Good because it finally decouples Shlink internal config (which is an implementation detail) from any external tool, including the installer, allowing to change it at will. +* Good because it opens the door to eventually simplify the installer. For the moment, it requires a bit of extra logic to support importing the old config. +* Good because it allows keeping the logic to read env vars next to the config where it applies. diff --git a/docs/adr/README.md b/docs/adr/README.md index af03faac..8fd4a662 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -2,6 +2,7 @@ Here listed you will find the different architectural decisions taken in the project, including all the reasoning behind it, options considered, and final outcome. +* [2022-01-15 Update env vars behavior to have precedence over installer options](2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md) * [2021-08-05 Migrate to a new caching library](2021-08-05-migrate-to-a-new-caching-library.md) * [2021-02-07 Track visits to 'base_url', 'invalid_short_url' and 'regular_404'](2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md) * [2021-01-17 Support restrictions and permissions in API keys](2021-01-17-support-restrictions-and-permissions-in-api-keys.md) From 545da96d15b8aeee81cef76bfd6a0181911c1d23 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 15 Jan 2022 17:21:36 +0100 Subject: [PATCH 7/7] Updated env vars ADR --- ...nv-vars-behavior-to-have-precedence-over-installer-options.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/adr/2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md b/docs/adr/2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md index 63231bc1..df11538c 100644 --- a/docs/adr/2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md +++ b/docs/adr/2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md @@ -34,6 +34,7 @@ Then Shlink reads this as the first config file, which sets the values as env va * Good because it would not require any change in the installer. * Bad because it requires moving all env var reading logic somewhere else, while having it together with their contextual config is quite convenient. +* Bad because it requires defining a map between the config path from the installer and the env var to set. ### Make the installer generate a config file which also reads env vars