Use Symfony Local Web Server instead of deprecated WebServerBundle #6850

yguedidi · 2023-08-17T22:17:03Z

WebServerBundle is deprecated in Symfony 4.4 and is removed in Symfony 5.
This PR remove it and make use of the Symfony CLI and its Local Web Server to run Wallabag locally.

Just changing the Dockerfile exposed an issue: the IP visible by Symfony is not anymore 127.0.0.1 but the Docker container IP and so app_dev.php block all requests.
To solve that, we needed the TRUSTED_PROXIES feature to allow Docker container IPs, feature that is available in an up to date index.php file instead of both app.php and app_dev.php.
So I updated some files to match as close as possible to the framework-bundle recipe in version 4.4
If it's requested I can extract those updates in a prerequisite PR to ease the review.

Edit: in order to make tests pass, I also update some test related files to match as close as possible to the phpunit-bridge recipe in version 4.3

yguedidi · 2023-08-17T22:18:03Z

.env

@@ -0,0 +1,4 @@
+APP_ENV=dev
+APP_SECRET=ch4n63m31fy0uc4n
+TRUSTED_PROXIES=127.0.0.1/8,10.0.0.0/8,172.16.0.0/12,172.17.0.0/16,172.18.0.0/15,172.20.0.0/14,172.24.0.0/13,192.168.0.0/16


original value is 127.0.0.0/8,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16
updated to add more based on this article

yguedidi · 2023-08-17T22:19:34Z

.env

@@ -0,0 +1,4 @@
+APP_ENV=dev
+APP_SECRET=ch4n63m31fy0uc4n


value taken from docker/php/env.example

yguedidi · 2023-08-17T22:20:22Z

.gitignore

@@ -20,9 +29,6 @@ docker-compose.override.yml
 # Parameters
 /app/config/parameters.yml

-# Managed by Composer
-/vendor/


removed as covered by the ignore rules from framework-bundle recipe

yguedidi · 2023-08-17T22:20:51Z

app/config/parameters.yml.dist

@@ -30,9 +30,6 @@ parameters:

    locale: en

-    # A secret key that's used to generate certain security-related tokens
-    secret: CHANGE_ME_TO_SOMETHING_SECRET_AND_RANDOM


removed as it became an env var

yguedidi · 2023-08-17T22:22:04Z

web/index.php

+    Request::setTrustedHosts([$trustedHosts]);
+}
+
+$kernel = new AppKernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']);


original is Kernel from App namespace, adapted to work with existing AppKernel

yguedidi · 2023-08-17T22:22:37Z

web/index.php

+//$kernel = new AppCache($kernel);
+
+// When using the HttpCache, you need to call the method in your front controller instead of relying on the configuration parameter
+//Request::enableHttpMethodParameterOverride();


this was not in original from recipe, added because it was in app.php

yguedidi · 2023-08-17T22:23:23Z

web/.htaccess

@@ -3,7 +3,7 @@
 # mod_rewrite). Additionally, this reduces the matching process for the
 # start page (path "/") because otherwise Apache will apply the rewriting rules
 # to each configured DirectoryIndex file (e.g. index.php, index.html, index.pl).
-DirectoryIndex app.php
+DirectoryIndex index.php


searched for occurences of app.php and app_dev.php in the project to use the new common file

yguedidi · 2023-08-17T22:24:30Z

config/bootstrap.php

added as is from the recipe

yguedidi · 2023-08-17T22:24:56Z

docker/php/Dockerfile

@@ -94,6 +100,7 @@ RUN groupmod -g 1000 www-data ; \
    chown -R www-data: /var/www/html \
        /usr/local/etc/php/conf.d/wallabag-php.ini \
        /var/www/.cache \
-        /var/www/.yarnrc
+        /var/www/.yarnrc \
+        /var/www/.symfony5


needed because Symfony CLI writes in it

yguedidi · 2023-08-17T22:25:33Z

docker/php/Dockerfile

+        /var/www/.cache \
+        /var/www/.symfony5
+
+WORKDIR /var/www/html


needed because it seems php:8.1 image uses / as default working dir

yguedidi · 2023-08-17T22:26:29Z

docker/php/Dockerfile

@@ -15,6 +15,8 @@ RUN apt-get update \
 RUN curl 'https://deb.nodesource.com/gpgkey/nodesource.gpg.key' | apt-key add - \
    && echo "deb https://deb.nodesource.com/node_${NODE_VERSION}.x $(lsb_release -cs) main" > /etc/apt/sources.list.d/nodesource.list

+RUN curl -1sLf 'https://dl.cloudsmith.io/public/symfony/stable/setup.deb.sh' | bash


followed default installation instruction for Ubuntu/Debian from the official website

yguedidi · 2023-08-17T23:11:04Z

.env.test

@@ -0,0 +1,5 @@
+KERNEL_CLASS='AppKernel'


original is App/Kernel, adapted to work with existing AppKernel

yguedidi · 2023-08-17T23:11:23Z

.env.test

@@ -0,0 +1,5 @@
+KERNEL_CLASS='AppKernel'
+APP_SECRET='$ecretf0rt3st'
+SYMFONY_DEPRECATIONS_HELPER=weak


original is 999999, update to current configuration

yguedidi · 2023-08-17T23:13:04Z

tests/bootstrap.php

@@ -0,0 +1,7 @@
+<?php
+
+require dirname(__DIR__) . '/config/bootstrap.php';


original wrap this by a condition that lead to a PHPStan error.
as the file exist for sure in our case, I simplified the code to avoid that error

yguedidi · 2023-08-17T23:15:20Z

.gitignore

-.phpunit.result.cache
-phpunit.xml


removed as covered by the ignore rules from phpunit-bridge recipe

nicosomb · 2023-08-18T05:09:49Z

.env.test

+KERNEL_CLASS='AppKernel'
+APP_SECRET='$ecretf0rt3st'
+SYMFONY_DEPRECATIONS_HELPER=weak
+PANTHER_APP_ENV=panther


symfony\panther is not used in our project

@nicosomb I know, I kept it to be as close as possible to the recipe. Maybe one day it will be used for e2e tests. And as it's a env var I don't think it hurts having it.
Another option would be to make those a comment like TRUSTED_HOSTS, what do you think?
If not OK I can of course remove them :)

nicosomb

@yguedidi do we need to write some documentation?

yguedidi · 2023-08-24T11:00:20Z

do we need to write some documentation?

@nicosomb I don't think so, or at least I don't see what could be documented in this change

j0k3r

We should warn that people will have to update their server configuration because web/app.php is replaced by web/index.php.
https://github.com/search?q=repo%3Awallabag%2Fdoc%20app.php&type=code

So maybe we should update the documentation to explain that starting from 2.7.0 version, the index is now web/index.php

nicosomb · 2023-12-07T10:08:15Z

@yguedidi do you have some work to do on this PR?

yguedidi · 2023-12-07T10:32:36Z

@nicosomb first sorry for not being that available lately for wallabag...
I think it's worth doing a rebase just in case, also extract few change to facilitate review, and also I'd like to do few changed so that it can pass in a minor version without BC breaks.
What do you think?

nicosomb · 2023-12-07T12:48:44Z

So, you suggest to release a 2.6.x ?

yguedidi · 2023-12-07T13:30:06Z

@nicosomb had a bit of time during lunch break so I checked again the diff, and maybe the move to env variables may be an issue for users upgrading wallabag, as they'll need to define those env variables in prod.
is this a considered a BC break, and so the must be release in 3.x, or not a BC break but a matter of documenting the upgrade?
if I remember correctly the BC policy is only about the API, is this still true?

yguedidi · 2023-12-07T19:47:19Z

@j0k3r @Kdecherf any opinion on is the switch to env vars an issue for a minor/patch release?