Merge branch 'develop' into feature/all-visits-endpoint

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.

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
 

composer.json

@@ -18,7 +18,7 @@
         "akrabat/ip-address-middleware": "^2.1",
         "cakephp/chronos": "^2.3",
         "doctrine/migrations": "^3.3",
-        "doctrine/orm": "^2.10",
+        "doctrine/orm": "^2.11",
         "endroid/qr-code": "^4.4",
         "geoip2/geoip2": "^2.12",
         "guzzlehttp/guzzle": "^7.4",
@@ -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#a008036 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",

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(),

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,

config/config.php

@@ -9,6 +9,7 @@ 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;
@@ -16,8 +17,12 @@ use function Shlinkio\Shlink\Config\env;
 use const PHP_SAPI;
 
 $isCli = PHP_SAPI === 'cli';
+$isTestEnv = env('APP_ENV') === 'test';
 
 return (new ConfigAggregator\ConfigAggregator([
+    ! $isTestEnv
+        ? new EnvVarLoaderProvider('config/params/generated_config.php')
+        : new ConfigAggregator\ArrayProvider([]),
     Mezzio\ConfigProvider::class,
     Mezzio\Router\ConfigProvider::class,
     Mezzio\Router\FastRouteRouter\ConfigProvider::class,
@@ -35,9 +40,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\LaminasConfigProvider('config/params/generated_config.php'),
+        : new ConfigAggregator\ArrayProvider([]),
 ], 'data/cache/app_config.php', [
     Core\Config\BasePathPrefixer::class,
 ]))->getMergedConfig();

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

docs/adr/2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md

@@ -0,0 +1,51 @@
+# 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.
+* 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
+
+* 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.

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)

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 = match (true) {
-            $this->disableTrackingFrom = $disableTrackingFrom;
+            is_array($disableTrackingFrom) => $disableTrackingFrom,
-        } else {
+            $disableTrackingFrom === null => [],
-            $this->disableTrackingFrom = $disableTrackingFrom === null ? [] : explode(',', $disableTrackingFrom);
+            default => map(explode(',', $disableTrackingFrom), static fn (string $value) => trim($value)),
-        }
+        };
     }
 }

module/Core/src/Repository/ShortUrlRepository.php

@@ -192,6 +192,9 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU
         return $this->doShortCodeIsInUse($identifier, $spec, LockMode::PESSIMISTIC_WRITE);
     }
 
+    /**
+     * @param LockMode::PESSIMISTIC_WRITE|null $lockMode
+     */
     private function doShortCodeIsInUse(ShortUrlIdentifier $identifier, ?Specification $spec, ?int $lockMode): bool
     {
         $qb = $this->createFindOneQueryBuilder($identifier, $spec)->select('s.id');