-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Only add classes to compile on PHP < 7.0 #387
Only add classes to compile on PHP < 7.0 #387
Conversation
addClassesToCompile is deprecated and doesn't improve performances on PHP > 7.0
@@ -72,7 +72,9 @@ public function load(array $configs, ContainerBuilder $container) | |||
$this->configureProfiler($container, $config); | |||
$this->configureException($container, $config); | |||
$this->configureMenus($container, $config); | |||
$this->configureClassesToCompile(); | |||
if (\PHP_VERSION_ID < 70000) { |
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 would be more readable to use version_compare
IMHO.
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 personally prefer using the constant:
- It's what symfony uses in it's codebase
- The php optimizer can optimize it and remove this part of code on the corresponding php versions
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.
@soullivaneuh do you know why they choose to do it like this in sf?
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.
@greg0ire it's done to allow the optimization of opcodes : symfony/symfony#12497
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.
Yeah plus it's less error prone, and it's still easy to ready with such a constant.
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 this case, go for it.
It would be a great Fixer idea for php-cs-fixer BTW...
I am targeting this branch, because this fix a deprecation warning and is BC.
Changelog
Subject
Only call addClassesToCompile on PHP < 7.0 since it is deprecated and doesn't improve performances with PHP >= 7.0