Use strict comparison #6980
Use strict comparison #6980
Conversation
The new code should be used as soon as 2.8.0 is available
| code to use:: | ||
|
|
||
| if (Kernel::VERSION_ID <= 20800) { | ||
| if (Kernel::VERSION_ID < 20800) { |
There was a problem hiding this comment.
This looks wrong to me. The code for Symfony 3.0 would now also be executed on Symfony 2.8 which might not be what you want to do (as all the deprecated code would not be available anymore).
There was a problem hiding this comment.
One thing is sure : if it can be executed on 2.8.1, it should also be executed in 2.8.0, right? And looking at the condition as it is before my change, it can be executed for symfony 2.8.1, right?
There was a problem hiding this comment.
@xabbuh : not sure if you're saying that my change is wrong, or that the thing before is even wronger for other reasons, can you please clarify?
There was a problem hiding this comment.
Indeed, I did not think about 2.8.1. What do you think if we changed the condition to something like Kernel::VERSION_ID < 30000 then?
There was a problem hiding this comment.
Most of the time, you do want to get the shift on 2.8.0, that's what we do on sonata :
if ($conditionAbout2.8) {
// use new way, that way we don't get the deprecation notice
} else {
// use old way
}See for instance : https://github.com/sonata-project/SonataAdminBundle/blob/48cc780dbcd026193e0e98fb2ad6a2a997e2ec22/Form/Type/AclMatrixType.php#L39
There was a problem hiding this comment.
can you please change this to 2 === Kernel::MAJOR_VERSION?
There was a problem hiding this comment.
@wouterj I don't think it is relevant, because this will be true for both 2.7 and 2.8, right? And the code change should generally apply when switching from 2.7 to 2.8, instead of between 2 and 3, otherwise you get deprecation notices for 2.8.
There was a problem hiding this comment.
Of course there are situations when you can need both switches, like choices_as_values, but IMO the most frequent and encourages should be 2.7 to 2.8
There was a problem hiding this comment.
Yeah, I think you are right. When targetting Symfony 2.8 you will not want to use the deprecated code paths.
|
👍 Status: Reviewed |
|
👍 |
1 similar comment
|
👍 |
|
Thank you @greg0ire. |
This PR was merged into the 2.7 branch. Discussion ---------- Use strict comparison Commits ------- 2c1986f Use strict comparison
6 participants
No description provided.