search

Amsterdam / schulddossier

public mirror repository
Mozilla Public License 2.0
1 stars 0 forks source link

Updates Symfony to version 5.4 and fixes compatibility issues with Symfony 5.4 #277

Closed rjzondervan closed 2 months ago

rjzondervan commented 2 years ago

Also contains a few developer-friendliness fixes, although a separate PR to finish that up is prepared to be made after this PR is merged.

Keys and passwords (primarily in docker-compose-dev.yml) are strictly for development container purposes, and hence will only work in the development environment as described in docker-compose-dev.yml file. These keys do not in any way relate to actual keys used in production environments.

rjzondervan commented 2 years ago

Dank voor de review! Ik ga er maandag mee aan de slag :). Voor wat betreft de commit 08070dc, ik ga even kijken of ik Git(Hub) zo ver kan krijgen om de actie die een stylechecker (ik ben nog niet zeker welke, ik vermoed mijn IDE, maar het kan ook Git zelf zijn) nodig vond om alle trailing spaces van regels weg te halen los te trekken van de daadwerkelijke code changes daar.

rjzondervan commented 2 years ago

Goedemorgen!

Ik ben even gaan zoeken naar een manier om de whitespace-changes die een stylechecker in 08070dc heeft afgedwongen uit het overzicht te halen. Daarbij kwam ik tegen dat de GitHub PR viewer een functie kent om whitespace only-changes te verbergen. Als je op 'Files changed' klikt bovenin de PR, en de changed files verschijnen, komt er een balkje in beeld waarmee gefilterd kan worden. Als je daarin op het 'settings' tandwiel klikt verschijnt er een modaltje waarin de checkbox 'Hide whitespace' staat. Door deze aan te vinken verdwijnen alle changes uit de viewer die alleen om whitespace draaien. image

Verder, voor wat betreft de commit met de message 'test', dit was een commit van een collega om dingen te testen of hij de code überhaupt aan de praat kreeg, waarmee hij verder was gekomen dan hij zelf dacht op het moment dat ik dit van hem overnam. In het vervolg zal je geen commits met de message 'test' of iets soortgelijks in principe niet meer tegenkomen.

De uitgecommentarieerde code/configuratieblokken die je in de review hebt aangegeven of die ik in mijn wijzigingen heb gespot, heb ik hetzij verwijderd, hetzij met een comment toegelicht (er zit een comment-block in waarmee geswitched kan worden tussen inlogmethodes), hetzij teruggeplaatst na configuratieverbetering.

jmassink commented 2 years ago

Dankjewel voor je verbeteringen Robert. Ik heb er even vlug doorheen gescanned en om op deze manier is de code beter te reviewen. Ik ga er morgen met een scherp oog naar kijken.

jmassink commented 2 years ago

Hoi Robert, ik heb een paar vragen en opmerkingen. Over het algemeen ziet de PR er nu goed uit. Groeten, Jasper

rjzondervan commented 2 years ago

Hi Jasper,

Ik zal proberen aankomende week een volledige reactie op je review te geven.

Robert

rjzondervan commented 2 years ago

@jmassink en @tobifaf, ik heb jullie comments zo veel mogelijk beantwoord of verwerkt. Excuus voor de vertraging!

jmassink commented 2 years ago

Hoi Robert, Dankjewel voor het reageren. Ik denk dat het efficiënter is om een overdrachtsessie in te plannen in de vorm van een call. Dan kunnen we o.a. deze PR behandelen en kan je ook iets meer achtergrond geven over de opzet van de code. Art zal hierover contact opnemen. Tot die tijd laten we deze PR open staan. Groeten, Jasper