-
Notifications
You must be signed in to change notification settings - Fork 2
Upgrade Symfony 6.4 > 7.4 #463
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
Prior to this change, clearing the cache required the manual correction of the var/log and var/cache dirs to be owned by 33/www-data. This change corrects this on frontend-install automatically.
- fix phplint cache dir - make phpunit fail in dev on any deprecation warning - update phpstan baseline after update
246ed4b to
ec8232b
Compare
8708c9a to
ae1d727
Compare
The button group does not have data to map so needs mapped > false. And no data to inherit as wel. Symfony became more strict.
RecastingRemovalRector RemoveUselessParamTagRector RemoveUselessReturnTagRector
MKodde
left a comment
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.
I do not see any definitive requests for changes. Although some of my feedback might require some minor rework. See what clicks with you and feel free to discuss.
ci/qa/phpstan.neon
Outdated
| rules: | ||
|
|
||
| parameters: | ||
| treatPhpDocTypesAsCertain: false |
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.
Should we add this for the other StepUp projects? Or maybe ensure we do not need it here? The other projects do not seem to have this setting? I'm not rejecting the PR because of this, just curious.
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.
Initially, I thought this created a lot of issues. But 4, with 1 being fixable.
So removed it for now. We don't usually have this.
|
|
||
| use DateTime as CoreDateTime; | ||
|
|
||
| class DateTime |
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.
Is this class used anywhere? I could not find any usages myself. The Surfnet\StepupRa\RaBundle\Value\DateTime seems to be used instead.
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 spot. It is unused.
| public static function now(): DateTime | ||
| { | ||
| return self::$now ?: new self(new CoreDateTime); | ||
| return self::$now ?? new self(new CoreDateTime); |
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.
Does this change cause the addition of the phpstan-ignore-next-line property.unusedType ? This feels like a low gain optimization.
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.
the ignore is needed because this is how $now is used:
$nowProperty = new ReflectionProperty(DateTime::class, 'now');
$nowProperty->setValue(null, $now);I thought ?? was because of phpstan, but it is not, so reverted to ?:
| * between a closing angle bracket and an opening angle bracket ("> <" => "><"). Whitespace inside | ||
| * tags or textual content is preserved. | ||
| */ | ||
| class Spaceless |
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.
1: I'd expect the introduction of this replacement feature to be in a commit of it's own. But OK to keep it like this..
2: Have you tried to use the 'replacement' feature that was introduced?
See: https://twig.symfony.com/doc/3.x/templates.html#whitespace-control
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.
I don't know the exact reason why the spaceless is used here. I cannot confidently determine if the whitespace control does the exact same. It seems more targeted towards specific expressions. So I opted for this to be 100% backwards compatible.
New code should use the whitespace control.
| $command->orderBy = $request->query->get('orderBy'); | ||
| $command->orderDirection = $request->query->get('orderDirection'); |
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.
Why not use the trait here?
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.
This targeted way is still preferred imo. The trait is because the GET + POST + default lines got too long and messy.. This only does GET + default
|
|
||
| use Symfony\Component\HttpFoundation\Request; | ||
|
|
||
| trait OrderFromRequest |
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.
Maybe create two dedicated methods; one to get the sort order and one to get the column to sort on? orderBy & orderDirection. That way the trait becomes more focused on this specific purpose. I would be OK with hard coding the name of the request parameter for these methods. They always seem to be: orderDirection and orderBy.
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.
👍
| $command->orderBy = $this->getString($request, 'orderBy'); | ||
| $command->orderDirection = $this->getString($request, 'orderDirection'); |
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.
Use OrderFromRequest trait?
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.
it does? ;)
After merging, also merge:
OpenConext/OpenConext-devconf#72