-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Use PHP 7.1 syntax #22862
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
Use PHP 7.1 syntax #22862
Conversation
This is going to create a hell lot of merge conflicts. |
That's why I kept different changes in different commits. While bumping reqs to 7.1 is great, keeping syntax from 5.3 is not... |
Now that constants can be private maybe some should be reviewed to see if they should be private and not public? As making them private would be too big of a BC, we could go with the |
I agree with that @theofidry . Without accessor, it's public, so let write it instead of letting the compiler assume that. If some of them could not be public, it shall be changed in separated PRs with proper BC breaking changelog |
$a = isset($a['priority']) ? $a['priority'] : 0; | ||
$b = isset($b['priority']) ? $b['priority'] : 0; | ||
$a = $a['priority'] ?? 0; | ||
$b = $b['priority'] ?? 0; | ||
|
||
return $a > $b ? -1 : 1; |
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.
retun $a <=> $b1
?
👎 for sure (all 4 commits) and for the same reason we keep using Note that @theofidry's idea LGTM: replacing |
@keradus I wish we could use these new features, but we can't. To avoid merging issues, we can only use PHP 7.1 for new features added to master. For the existing code, we can only upgrade when the last Symfony version using the old PHP version finishes its support. This means that:
|
that sadly means code would always follow few-years old syntax :( |
In fact, merging hell is not related to old PRs. It's related to the fact that several times per week, we merge 2.7 into 2.8 into 3.2 into 3.3 into 3.4 into master. Any change in any branch <=3.4 will way more likelly trigger a merge conflict when 3.4 is then merged into master. |
what about accepting version of lower branch and reapply CS as conflict fix ? |
@keradus this won't work, because it may break newer features if the conflict is not strictly about coding standards |
Closing as explained, that's our burden :) |
@@ -27,7 +27,7 @@ protected function handleConfiguration(array &$arguments) | ||
|
||
$result = parent::handleConfiguration($arguments); | ||
|
||
$arguments['listeners'] = isset($arguments['listeners']) ? $arguments['listeners'] : array(); | ||
$arguments['listeners'] = $arguments['listeners'] ?? array(); |
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.
`$arguments['listeners'] = $arguments['listeners'] ?? [];
in that case milestone could be misleading |
This PR was merged into the 4.0-dev branch. Discussion ---------- Make internal constants private | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Changing some internal constants to private as suggested in #22862 (these are the only ones I found in the codebase) Commits ------- 367b055 Make internal constants private
After #22733, it's possible to use new language syntax