Symfony 4 upgrade #562

Shaky212 · 2022-11-21T08:53:01Z

This is the upgrade from Symfony 3.4 tot Symfony 4.4. The major steps are:

resolve deprecations
upgrade composer to Symfony 4.4
installl Symfony/flex

Within Symfony 4 the application and directory structure has changed. The app folder has partly been moved to the config folder en the .env environment. All twig templates have been moved to the templates folder.

Notice the .env contains a parameter administrator_teams used in the dashboard_saml.yaml and checked during compile time in Configuration.php. Previously this was an array, but not supported because of it's dynamic character. The administrator_teams is now a scalar, a string with comma separated values.

See also: https://symfonycasts.com/screencast/symfony4-upgrade/upgrade4

MKodde

Awesome work so far! I did note quiet a few issues you might need to address.

For example:

Some of the old, unused folders are still in the project web, app
Swiftmailer is still showing up in config
See code review below for more details.

MKodde · 2022-11-22T07:24:33Z

.env.dist

+MAILER_URL=null://localhost
+###< symfony/swiftmailer-bundle ###
+
+locale=en


I'm looking at this .env.dist file as a replacement for the parameters.yaml.dist file. From that perspective:

I'd love to see all comments that where previously there, instructing a user on how to fill the parameters.

While at it, please group related parameters (like they where in parameters.yaml.dist) and hence improve readability of this file.

MKodde · 2022-11-22T07:25:32Z

.github/workflows/test-integration.yml

- - name: Copy parameters.yml.dist to parameters.yml
- run: |
- docker-compose -f docker-compose.yml -f docker-compose-ci.yml exec -T php-fpm cp /var/www/html/app/config/parameters.yml.dist /var/www/html/app/config/parameters.yml


No need to cp .env.dist .env?

MKodde · 2022-11-22T07:26:16Z

.gitignore

 docker-compose.override.yml
-app/js/coverage
+assets/js/coverage
 /build/


/build/ stil in action? I thougth encore drops the web assets in the public/build folder.

MKodde · 2022-11-22T07:26:34Z

.gitignore

 /web/build
 /web/coverage


web folder is outahere right? If it is still in the repo. I think we should actually remove it from VCS!

MKodde · 2022-11-22T07:29:10Z

ansible/roles/spdashboard/tasks/install-branch.yml

 - name: Delete config
 file: path={{spdashboard_data_dir}}/current/app/{{ item }} state=absent
 with_items: 
- - config/parameters.yml
+ - config/parameters.yaml


This should not be a thing anymore right? If we need to remove some distributed configuration it should be a .env file we are removing here?

@quartje as you've been informed of. We are walking away from parameters.yaml configuration. And started using the .env way instead. May I ask you to review the changes (or missing changes) to the deployment code (ansible + github actions)? I will try to do so as well, but your Docker evengalist eagle eye will probably spot a thing or two 🦜

MKodde · 2022-11-22T07:56:57Z

issues.json

@@ -0,0 +1 @@
+{"9628d851-abd1-2283-a8f1-a29ba5036174":{"key":"9628d851-abd1-2283-a8f1-a29ba5036174","issueType":"spd-request-change-request-prod","ticketStatus":"To Do"}}


This file should not be here right?

MKodde · 2022-11-22T07:58:46Z

public/index.php

@@ -0,0 +1,28 @@
+<?php
+
+//use App\Kernel;


MKodde · 2022-11-22T07:59:38Z

src/Surfnet/ServiceProviderDashboard/Domain/Entity/PrivacyQuestions.php

@@ -33,7 +33,7 @@ class PrivacyQuestions
 * @var int
 *
 * @ORM\Id
- * @ORM\Column(type="integer")
+ * @ORM\Column(type="integer", options={"autoincrement":true})


Is this now a required option? Or a dev-left over?

MKodde · 2022-11-22T08:00:25Z

...net/ServiceProviderDashboard/Infrastructure/DashboardBundle/Controller/ServiceController.php

- /** @var LoggerInterface $logger */
- $logger = $this->get('logger');


🦾 🎖️

MKodde · 2022-11-22T08:03:33Z

templates/Exception/error.html.twig

@@ -1,4 +1,4 @@
-{% extends 'base.html.twig' %}
+{% extends '../../../../../templates/base.html.twig' %}


Why are the ../ required here. Given the other templates that can simply extend the base template by its name?

MKodde · 2022-11-22T08:13:46Z

docker/Dockerfiledev

@@ -1,6 +1,7 @@
 FROM ghcr.io/openconext/openconext-deploy/openconext-core:feature_build_image_with_teams_included AS openconext
 COPY ./conf/prep_oc.sh /usr/local/sbin/prep_oc.sh
-COPY ./conf/saml20_sp.json /tmp/saml20_sp.json
+#COPY ./conf/saml20_sp.json /tmp/saml20_sp.json


Also on line 13, we install composer 1 in the fpm container. But that conflicts with our new composer packages installed. They require composer 2 to install. Please upgrade to composer 2 :)

MKodde · 2022-11-22T08:14:24Z

The jest test still fail as they refer to old, moved fixture files. Should be an easy fix to get a fully green build!

Upgrade Symfony 4 Upgrade Symfony 4

These extensions are already installed in OpenConext-containers. No need to waste all the cpu cycles here doing it twice.

MKodde · 2023-01-04T07:28:31Z

Closing this PR, the work is continued on #565

* Upgrade Symfony 5 A mighty PR where multiple disciplines entailing the Symfony upgrade come together. Among them: #562 Upgrade to Symfony 4 (in preparation to this SF5 upgrade) #571 Move to Github actions for test integrations At first these tasks where created separate. But to keep things more tight, I chose to merge/rebase them into this branch. Beside an upgrade to this version a major update of the security has been done. Most of the security part is handled within the stepup saml bundle and SPD is a consumer of this implementation. see: https://www.pivotaltracker.com/story/show/173217517 Co-authored-by: Michiel Kodde <mkodde@ibuildings.nl> Co-authored-by: Bart Geesink <bart.geesink@surf.nl>

Shaky212 force-pushed the feature/symfony-5-upgrade branch 12 times, most recently from 68880f9 to 118967e Compare November 21, 2022 15:54

MKodde reviewed Nov 22, 2022

View reviewed changes

MKodde requested changes Nov 22, 2022

View reviewed changes

Shaky212 force-pushed the feature/symfony-5-upgrade branch 5 times, most recently from 7b47cc2 to 46f4d7d Compare December 6, 2022 09:52

Upgrade Symfony 4

f890942

Upgrade Symfony 4 Upgrade Symfony 4

Shaky212 force-pushed the feature/symfony-5-upgrade branch 5 times, most recently from c21875e to 3a2b53f Compare December 8, 2022 14:48

Upgrade Symfony 4

859978a

Shaky212 force-pushed the feature/symfony-5-upgrade branch from 3a2b53f to 859978a Compare December 8, 2022 15:57

quartje added 2 commits December 9, 2022 11:36

Docker: No need to install php extensions

aa77253

These extensions are already installed in OpenConext-containers. No need to waste all the cpu cycles here doing it twice.

GitHub actions: Run on Ubuntu 20.04, allowing for cgroups v1

3794e8c

quartje and others added 3 commits December 13, 2022 11:29

GHA: The project no longer has a logs dir. Removing it

f837e1e

Repair syntax error by adding an empty body

8ab9b0c

Repair missing space

58e07c1

MKodde closed this Jan 4, 2023

MKodde mentioned this pull request Jan 5, 2023

Symfony 5 upgrade #565

Merged

MKodde deleted the feature/symfony-5-upgrade branch July 5, 2023 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Symfony 4 upgrade #562

Symfony 4 upgrade #562

Shaky212 commented Nov 21, 2022

MKodde left a comment

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde Nov 22, 2022

MKodde commented Nov 22, 2022

MKodde commented Jan 4, 2023

		@@ -0,0 +1 @@
		{"9628d851-abd1-2283-a8f1-a29ba5036174":{"key":"9628d851-abd1-2283-a8f1-a29ba5036174","issueType":"spd-request-change-request-prod","ticketStatus":"To Do"}}

		/** @var LoggerInterface $logger */
		$logger = $this->get('logger');

		@@ -1,4 +1,4 @@
		{% extends 'base.html.twig' %}
		{% extends '../../../../../templates/base.html.twig' %}

Symfony 4 upgrade #562

Symfony 4 upgrade #562

Conversation

Shaky212 commented Nov 21, 2022

MKodde left a 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

MKodde commented Nov 22, 2022

MKodde commented Jan 4, 2023