Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Closed
wants to merge 4 commits into from
Closed

Use PHP 7.1 syntax #22862

wants to merge 4 commits into from

Conversation

keradus
Copy link
Member

@keradus keradus commented May 22, 2017

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? n/a
Fixed tickets #22733
License MIT
Doc PR n/a

After #22733, it's possible to use new language syntax

@keradus keradus changed the title 4 php71 Use PHP 7.1 syntax May 22, 2017
@nicolas-grekas
Copy link
Member

This is going to create a hell lot of merge conflicts.
All changes that are pure CS should be reverted IMHO.
And the remaining ones challenged for technical benefit.

@keradus
Copy link
Member Author

keradus commented May 22, 2017

That's why I kept different changes in different commits.
During merging lower to higher branch one could accept lower-branch version of conflicted code and re-apply styles.
On the other hand, probably there is not much value in keeping PRs open even since 2012.

While bumping reqs to 7.1 is great, keeping syntax from 5.3 is not...
Since produced diff is only 1k lines, it's small change from CS point of view.
It's not like I am suggesting using short array syntax ;)

@theofidry
Copy link
Contributor

theofidry commented May 22, 2017

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 @private tag on them.

@keradus
Copy link
Member Author

keradus commented May 22, 2017

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

retun $a <=> $b1 ?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 23, 2017

👎 for sure (all 4 commits) and for the same reason we keep using array() vs []: this is pure code style change. It will create merge conflicts that we'll have to deal with for years to come.

Note that @theofidry's idea LGTM: replacing @internal consts by private const when possible.

@javiereguiluz
Copy link
Member

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

  • We could upgrade Symfony code to use PHP 5.5 on November 2019 (when Symfony 2.8 dies)
  • We could upgrade Symfony code to use PHP 7.1 on November 2021 (when Symfony 3.4 dies).

@keradus
Copy link
Member Author

keradus commented May 23, 2017

that sadly means code would always follow few-years old syntax :(
I bet there is a way to avoid merging hell, probably first step would be to close outdated PRs...

@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 23, 2017

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.

@keradus
Copy link
Member Author

keradus commented May 23, 2017

what about accepting version of lower branch and reapply CS as conflict fix ?

@stof
Copy link
Member

stof commented May 23, 2017

@keradus this won't work, because it may break newer features if the conflict is not strictly about coding standards

@nicolas-grekas nicolas-grekas added this to the 4.0 milestone May 23, 2017
@nicolas-grekas
Copy link
Member

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();
Copy link
Contributor

Choose a reason for hiding this comment

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

`$arguments['listeners'] = $arguments['listeners'] ?? [];

@keradus keradus deleted the 4_php71 branch May 24, 2017 17:35
@keradus
Copy link
Member Author

keradus commented May 24, 2017

in that case milestone could be misleading

@stof stof removed this from the 4.0 milestone May 24, 2017
nicolas-grekas added a commit that referenced this pull request May 25, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.