-
Notifications
You must be signed in to change notification settings - Fork 1
Upgrade Symfony 6.4 > 7.4 #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- `composer update -W` - Update phpstan, add rector - fix phplint cache dir - add correct phpunit cache dir - make test-integration.yml conform other projects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
Code wise this looks good, see some questions/remarks below.
Functionally, I could not get a functioning version of the app.
- The
federation_metadata_cache_locationparameter default value is not pointing to a sensible directory. Maybe fix that? - The
registrationroute fails with a 500 error on my dev setup. Yielding this exception message (also cache related it seems)
The controller for URI "/registration" is not callable: The cache directory "" does not exist or is not a directory.
EDIT Feb 3 1207
Turns out I misconfigured the cache directory while re-setting it from the faulty default. So in the end I used this one: federation_metadata_cache_location: /var/www/html/var/cache/federation-metadata And that works as intended.
Maybe configure that path in the parameters.yaml.dist file? And add a .gitkeep to the var/cache/federation-metadata folder?
EDIT Feb 5 1151
You are right! I completely overlooked that already existing folder. See a possible fix in my code review comment below
| throw new RuntimeException("invalid idp cache data encountered"); | ||
| } | ||
|
|
||
| if (!is_string($object['updated'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the exact same behavior as we used to have. The default to an empty string seems to be replaced by an exception? Maybe my PHP-FU is not as strong anymore, but I think this is a change in behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! I this was triggered by a 'mixed' type error. But I did not take into account the null case. Fixed.
tests/Unit/Application/Institution/Service/MetadataIdentityProviderServiceTest.php
Show resolved
Hide resolved
I'm confused. The default points to I notice it's not in the parameters.yaml.test.dist, so I added it there as well. |
This seems to be stemming from |
composer.json
Outdated
| ], | ||
| "fix-dev-permissions": [ | ||
| "chown -R www-data:www-data /var/www/html/var/log", | ||
| "chown -R www-data:www-data /var/www/html/var/cache" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In response to your confusion post:
I'm confused. The default points to /var/www/html/federation-metadata, which exists in the repo, and has a .gitignore. So it seems sensible to me. I'm not sure if we can just move the cache dir.
I notice it's not in the parameters.yaml.test.dist, so I added it there as well.
I think the fix would be to add a line to the fix-dev-permissions, including the federation-metadata in the fix permissions run
"fix-dev-permissions": [
"chown -R www-data:www-data /var/www/html/var/log",
"chown -R www-data:www-data /var/www/html/var/cache",
"chown -R www-data:www-data /var/www/html/federation-metadata"
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated fix-dev-permissions
- AddReturnTypeDeclarationRector - RenameAttributeRector - SimplifyFormRenderingRector
…ripts to build frontend with correct permissions of var dir.
And apply the relevant rectors automatically
9c6d36f to
8e888c2
Compare
No description provided.