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.
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.
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.
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.
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.
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.