Skip to content

Use strict comparison #6980

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.

Sign up for GitHub

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

Merged
xabbuh merged 1 commit into from
Oct 14, 2016
Merged

Use strict comparison #6980

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup/bundles.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ Before diving into the specifics of the most common edge cases, the general
recommendation is to **not rely on the Symfony Kernel version** to decide which
code to use::

if (Kernel::VERSION_ID <= 20800) {
if (Kernel::VERSION_ID < 20800) {
Copy link
Member

@xabbuh xabbuh Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@greg0ire greg0ire Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@greg0ire greg0ire Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

@xabbuh xabbuh Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@greg0ire greg0ire Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@wouterj wouterj Sep 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please change this to 2 === Kernel::MAJOR_VERSION?

Copy link
Contributor Author

@greg0ire greg0ire Sep 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@greg0ire greg0ire Sep 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@greg0ire greg0ire Sep 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xabbuh @wouterj still think this "Needs Work" ?

Copy link
Member

@xabbuh xabbuh Sep 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think you are right. When targetting Symfony 2.8 you will not want to use the deprecated code paths.

// code for Symfony 2.x
} else {
// code for Symfony 3.x
Expand Down