Rejuvinate the Stepup Middleware #416
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MKodde
force-pushed
the
feature/rejuvination
branch
from
1ec4235 to
78ee79c
Compare
Base automatically changed from
feature/build-and-publish-test-container
to
main
February 20, 2024 15:35
MKodde
force-pushed
the
feature/rejuvination
branch
4 times, most recently
from
ea8aa70 to
4e910cc
Compare
They are monitored using Dependabot
The latest and greatest dependencies are installed.
The composer packages have been upgraded. Some code is now broken as proven by tests. This commit aims to eat the low hanging fruits, and get the tests as green as possible. The harder nuts will be fixed in the following commits
In v6, we set the rootnode name in the constructor of the tree builder. And then retrieve the rootnode via a getter
For now they: - Implement the find method (add return types) - Use the bridge ManagerRegistry
1. Added return type definitions 2. When possible; add parameter type definitions 3. Use imports, remove FQN
Simplify certain code constructs like: - Simplify return statement - Make 'is empty' statements more specific - Simplify regex statements
As performed by Rectors SYMFONY_CODE_QUALITY set list
And clean up the code accordingly
Many repos referred to a moved ManagerRegistry class. That was changed in this commit
Type definitions have been added where possible
pablothedude
force-pushed
the
feature/rejuvination
branch
from
9a06585 to
9d834af
Compare
This is done to prevent failing replays
Fix bugs after manual testing
pablothedude
force-pushed
the
feature/rejuvination
branch
from
d4cd580 to
894632a
Compare
MKodde
commented
Dec 11, 2024
There was a problem hiding this comment.
Awesome work @pablothedude. This was quite an undertaking!
I have some questions, submitted as comments in this review thread, but also have a couple of peer review questions I'd like to look at with you IRL.
src/Surfnet/StepupMiddleware/ApiBundle/Doctrine/Type/SelfVetOptionType.php
Show resolved
Hide resolved
src/Surfnet/StepupMiddleware/GatewayBundle/Entity/InstitutionConfiguration.php
Show resolved
Hide resolved
src/Surfnet/StepupMiddleware/ApiBundle/Identity/Projector/SecondFactorProjector.php
Outdated
Show resolved
Hide resolved
src/Surfnet/StepupMiddleware/ApiBundle/Controller/RecoveryTokenController.php
Show resolved
Hide resolved
johanib
reviewed
Dec 12, 2024
johanib
reviewed
Dec 16, 2024
|
Addressed all feedback |
johanib
approved these changes
Dec 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The aim of this PR is to make the Stepup-Middleware project compatible with PHP 8.2 and Symfony 6.4.
This is not simply achieved by installing those requirements and call it a day. Many of the project dependencies need to be made compatible with the new standards Symfony adheres to. And that can quickly become a slippery slope.
In this PR so far I mainly focussed on improving the code quality to modern standards. For example by implementing PHP Stan's rules. Almost the entire app has been made type safe and PHP 8 compatible.
Some major steps to take are:
config/packages/[dev/prod/...]folders and merge them into the main package config/migrationsfolder imho, and be excluded from qa tests?For more details on things to do, this private Google Drive document shows more information and an estimation on what's left to do.