Flex structure #7115

yguedidi · 2023-12-12T19:53:50Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Documentation	no
Translation	no
CHANGELOG.md	no
License	MIT

Upgrade to Symfony 5 #7137
Drop PHP 7.4 and 8.0 #7178
Remove need for TEST_DATABASE_PATH #7181
rework the move of packages config to use the when@ENV notation
install Symfony Runtime
update to use recipe code for Symfony 5
fix tests

This PR is initiated from the work on #6850 as it needed the some files to be updated from their updated version from Flex recipes, namely the framework-bundle, console and phpunit-bridge recipes.

So I decided to extract that update in a separate PR, but instead of updating just the files the other PR needed, partially taking the content from the recipe, I decided that it could be the opportunity to embrasse the new Flex structure, and update completely the files from above listed recipes, and all this without yet using Flex itself!

As I want this to be accepted in a minor version of wallabag, so that we can advance on upgrading to Symfony 5, I did my best to make the changes in a backward compatible way!
What I mean by "BC way"?
I covered by this (draft) BC policy the case of users using wallabag "as is", not customizing its code or something else, just configure the parameters requested during the setup and that's it.
And I want them to be able to upgrade their wallabag instance to the new minor, is it setup by the Docker, a git clone, or via ZIP file.

So I identified those critical pieces that must stay so that an upgrade setup continues to work:

parameters.yml file, contains the configuration
parameters.yml.dist file, model used to generate the configuration
web/ folder, used as document root by server configuration
app.php file, used as index file by server configuration
app_dev.php files, maybe used by server configuration, shouldn't be used in prod!
var/logs/ folder configured to be writable
var/cache/ folder configured to be writable
var/sessions/ folder configured to be writable
AppCache.php file as maybe app.php was altered to use it
AppKernel.php file as used by app.php and app_dev.php

Leaving those elements should keep a setup working, even if wallabag internals change!

It may look like a very big PR, 62 commits, 188 files changed, +989 lines −786 lines... but I don't think it is!
I did my best to tell a story, showing the path I went through to achieve the goal, with baby steps.
This means that even if they are lot of commits, each should be a very easy to review!
So you guessed it, the recommended way to review this is commit by commit, open each in a new tab.
One important thing: I did my best to keep tests green at each commit!
Hope those efforts will really help reviewing this PR :)

I'm opening this PR as draft but it's not, it's just that there are some fixup commits that don't make sense on their own that I'd like to squash before merge, but are there to highlight some minor changes that are needed by the commit they fix to make it work.
Those changes would have been hard to catch if done directly along other changes of the commit they fix.

I will go through the commits and add some commit related comments to add more details.

Next steps:

move to use env vars with a BC layer using the env(MY_VAR): '%my_var%' fallback syntax
move to no bundle pattern for app code
install Flex and update configs based on recipes
check what new configuration we can take from recipes

yguedidi · 2023-12-12T20:05:35Z

app/config/config.yaml

@@ -25,3 +24,4 @@ imports:
    - { resource: ../../config/packages/twig.yaml }
    - { resource: ../../config/packages/validator.yaml }
    - { resource: ../../config/packages/wallabag.yaml }
+    - { resource: ../../config/services.yaml }


Load services after packages: this move is done because it's the way the new Kernel will load them

yguedidi · 2023-12-12T20:06:34Z

app/AppKernel.php

-use Symfony\Component\Config\Loader\LoaderInterface;
-use Symfony\Component\DependencyInjection\ContainerBuilder;
-use Symfony\Component\HttpKernel\Kernel;
+use Wallabag\Kernel;


decided to use Wallabag as root namespace instead of the default App one.

yguedidi · 2023-12-12T20:09:12Z

config/routes/dev/routes.yaml

-_profiler:
-    resource: "@WebProfilerBundle/Resources/config/routing/profiler.xml"
-    prefix: /_profiler
+WebProfilerBundle:


using the bundle name here as done in the recipe, I applied this pattern for other routes move, as there is no chances that the bundle name is used as a route name, so no conflicts

yguedidi · 2023-12-12T20:11:15Z

src/Wallabag/CoreBundle/Resources/views/Entry/entries.xml.twig

@@ -27,7 +27,7 @@
        <name>{{ user }}</name>
    </author>
    <icon>{{ asset('favicon.ico') }}</icon>
-    <logo>{{ asset('bundles/wallabagcore/themes/_global/img/logo-square.png') }}</logo>
+    <logo>{{ asset('img/logo-square.png') }}</logo>


not really sure about this one to be honest, but it looks it was broken before as there is no themes/_global/img/logo-square.png in CoreBundle

yguedidi · 2023-12-12T20:12:04Z

config/services.yaml

@@ -4,6 +4,7 @@ imports:
    - { resource: parameters_addons.yaml }

 parameters:
+    env(APP_SECRET): '%secret%'


this is the trick to preserve BC! if the APP_SECRET env var is not set, it will use the value from the %secret% parameter!

yguedidi · 2023-12-12T20:14:01Z

config/packages/test/monolog.yaml

this is a pure copy of the one from dev config, as the test config was extending from dev one, but there one this monolog config and a single setting that needed to be added, so no more extending from dev

yguedidi · 2023-12-12T20:14:13Z

config/packages/test/web_profiler.yaml

@@ -1,5 +1,6 @@
 framework:
    profiler:
+        only_exceptions: false


this comes from the dev config

yguedidi · 2023-12-12T20:15:17Z

config/services_test.yaml

@@ -1,3 +1,6 @@
+imports:
+    - { resource: parameters_test.yaml }


needed so tests get parameters test values to override defaults ones from normal parameters.yml, as we're not using env vars (yet)

yguedidi · 2023-12-12T20:16:22Z

.env

@@ -0,0 +1,6 @@
+###> symfony/framework-bundle ###


even not installed with Flex, used this syntax so Flex is ready to identify those places when we will install it and update recipes

yguedidi · 2023-12-12T20:17:31Z

.env

@@ -0,0 +1,6 @@
+###> symfony/framework-bundle ###
+APP_ENV=dev
+APP_SECRET=CHANGE_ME_TO_SOMETHING_SECRET_AND_RANDOM


being here it means dev setup will use the env var, so this needs to be updated.
maybe I should remove it? kept it because it impacts only dev setup, not prod ones

yguedidi · 2023-12-12T20:18:19Z

config/packages/framework.yaml

    session:
        # handler_id set to null will use default session handler from php.ini
        handler_id: session.handler.native_file
        save_path: "%kernel.project_dir%/var/sessions/%kernel.environment%"
        cookie_secure: auto
-    fragments: ~
-    http_method_override: true
+        cookie_samesite: lax


this one I decide to keep it as it's security related

yguedidi · 2023-12-12T20:18:39Z

config/routes/dev/framework.yaml

@@ -0,0 +1,3 @@
+_errors:
+    resource: '@FrameworkBundle/Resources/config/routing/errors.xml'


this should remove some deprecation :)

yguedidi · 2023-12-12T20:19:02Z

config/services.yaml

+    # App\:
+    #     resource: '../src/'
+    #     exclude:
+    #         - '../src/DependencyInjection/'
+    #         - '../src/Entity/'
+    #         - '../src/Kernel.php'


kept this as a model for later

yguedidi · 2023-12-12T20:19:33Z

config/packages/framework.yaml

+    #php_errors:
+    #    log: true


commented as was not in our config

yguedidi · 2023-12-12T20:23:23Z

web/app.php

-$response = $kernel->handle($request);
-$response->send();
-$kernel->terminate($request, $response);
+require __DIR__ . '/../public/index.php';


this is the trick that keeps BC, keeping the app.php file but internally using the new index one.
small BC break here is for users who have altered their app.php file to use the AppCache, now they'll need to alter the index.php to use CacheKernel.
see https://symfony.com/doc/4.4/http_cache.html#symfony-reverse-proxy

I think it's acceptable as I don't think users do that, maybe I'm wrong

yguedidi commented Dec 12, 2023

View reviewed changes

config/packages/framework.yaml

#php_errors:

# log: true

Copy link

Contributor Author

yguedidi Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented as was not in our config

yguedidi commented Dec 12, 2023

View reviewed changes

yguedidi force-pushed the flex-structure branch 3 times, most recently from 0e9ff13 to 9aead7c Compare December 12, 2023 20:45

yguedidi requested review from j0k3r, nicosomb, Kdecherf and a team December 12, 2023 20:47

Kdecherf added Ready for review topic: technical Issues and pull requests without user impact labels Dec 13, 2023

yguedidi force-pushed the flex-structure branch from 9aead7c to cd8ff08 Compare December 23, 2023 17:09

yguedidi added this to the 2.8.0 milestone Dec 25, 2023

yguedidi force-pushed the flex-structure branch from cd8ff08 to 9da333a Compare January 13, 2024 00:42

yguedidi added 3 commits January 13, 2024 23:23

Regenerate PHPStan baseline

2a118a4

Upgrade to Symfony 5.4

b95882c

Use error routing from FrameworkBundle

d67f965

yguedidi added 24 commits January 14, 2024 23:21

Move NelmioApiDocBundle routes

a217d47

Move FOSUserBundle routes

0516045

Move FOSOAuthServerBundle routes

fe8d57f

Move CraueConfigBundle routes

231e59b

Move FOSJsRoutingBundle routes

388f264

Move SchebTwoFactorBundle routes

313fabc

Move bundles routes at the top

8f571e3

Move migrations

63b1e82

Move app resources to assets

ed21eb9

Use an APP_SECRET env var with fallback

c574ce0

Install symfony/dotenv

082dac5

Install symfony/runtime

653c17b

Sync Kernel with framework-bundle recipe

7ac3dc1

Sync remaining files with framework-bundle recipe

23ea7b3

fixup! Sync remaining files with framework-bundle recipe

cdc1564

fixup! Sync remaining files with framework-bundle recipe

2af7a29

fixup! Sync remaining files with framework-bundle recipe

4ea21f5

fixup! Sync remaining files with framework-bundle recipe

24cfea0

Make app.php and app_dev.php use index.php

0c0d6bd

Sync with console recipe

2a72622

Sync with phpunit-bridge recipe

3beeb28

fixup! Sync with phpunit-bridge recipe

7339837

fixup! Sync with phpunit-bridge recipe

205d1d5

Remove explicit setting of test env in bootstrap commands

70c02e8

yguedidi force-pushed the flex-structure branch 2 times, most recently from 23082dc to 70c02e8 Compare January 14, 2024 23:00

test

51790d9

yguedidi mentioned this pull request Jan 22, 2024

Move to one bundle #7151

Merged

1 task

Kdecherf added Work in progress and removed Ready for review labels Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flex structure #7115

Flex structure #7115

yguedidi commented Dec 12, 2023 •

edited

yguedidi Dec 12, 2023 •

edited

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

yguedidi Dec 12, 2023

		@@ -0,0 +1,3 @@
		_errors:
		resource: '@FrameworkBundle/Resources/config/routing/errors.xml'

Flex structure #7115

Are you sure you want to change the base?

Flex structure #7115

Conversation

yguedidi commented Dec 12, 2023 • edited

yguedidi Dec 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yguedidi commented Dec 12, 2023 •

edited

yguedidi Dec 12, 2023 •

edited