-
Notifications
You must be signed in to change notification settings - Fork 129
Symfony 7.0 support and refactoring #240
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
Conversation
…. Bump min PHP version to 8.0.2. Minor logic fixes. Migrate to new ECS version.
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.
Please review my explanation for some of the controversial changes
@@ -74,7 +62,7 @@ public function saveAsFile($contents): string | |||
|
|||
public function collectGarbage(): bool | |||
{ | |||
if (1 == !mt_rand(1, $this->gcFreq)) { | |||
if (1 != mt_rand(1, $this->gcFreq)) { |
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.
@Gregwar I think this had to be a long-time bug in the code, as the old code would be ALWAYS false. Hence the garbage collection was running EVERY SINGLE TIME. But check, maybe I didnt understant properly
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 think you should use strict comparison here
if (1 != mt_rand(1, $this->gcFreq)) { | |
if (1 !== mt_rand(1, $this->gcFreq)) { |
ecs.php
Outdated
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 was unable to port completely all ECS rules from the old YAML file (dropped support for YAML was with ECS 9). But I did copy most of them. Some are even discontinued now. The ECS <=7 did not support PHP 8, so I had to update it
}, | ||
{ | ||
"name": "Jeremy Livingston", | ||
"email": "[email protected]" | ||
} | ||
], | ||
"require": { | ||
"php": ">=7.1.3", | ||
"php": ">=8.0.2", |
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 is the PHP version required by Symfony 6.0
"symfony/form": "~5.0|~6.0", | ||
"symfony/framework-bundle": "~5.0|~6.0", | ||
"symfony/translation": "~5.0|^6.0", | ||
"twig/twig": "^2.10|^3.0" |
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.
twig 2.x is EOL as of Dec 2023
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 would even force Symfony 6.0 to 6.4, as it will be the only supported in ~6
@Gregwar can you please review this? We are stuck with symfony 6.x just because of this package. thanks! |
Sounds good to me, thanks for contributing @michnovka can you confirm me everything it's ok ? |
@Gregwar The |
@ajgarlag I deleted |
Strong typed everything. Symfony 7 support. Dropped Symfony 5 support. Bump min PHP version to 8.0.2. Minor logic fixes. Migrate to new ECS version.