8000 Use short array syntax by ostrolucky · Pull Request #29623 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Use short array syntax #29623

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Use short array syntax #29623

wants to merge 2 commits into from

Conversation

ostrolucky
Copy link
Contributor
@ostrolucky ostrolucky commented Dec 15, 2018 •
edited
Loading
Q A
Branch? 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Symfony is one of last projects using traditional array syntax. After dropping support for 2.8, this is best opportunity to switch to short syntax usage. If we do it later, there would be even more places where it will be needed to change and even more conflicts.

This patch is not just running php-cs-fixer. This is a subset of hunk changes which are conflict free!

Great care was taken to:

  • Make this commit mergeable to master, 4.1 and 4.2 without conflicts (removed changes which cause conflicts)
  • Make sure there are no syntax errors (php lint)
  • Make sure phpunit passes (manually skipped dumped fixtures)

@ostrolucky ostrolucky force-pushed the short-array branch 2 times, most recently from 7869878 to 13738f2 Compare December 16, 2018 11:37
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 17, 2018
@ostrolucky
Copy link
Contributor Author

I will rebase it after approval, before merge, since it's prone to conflicts when you merge some branches meanwhile.

@fancyweb
Copy link
Contributor

If this is accepted, the https://symfony.com/doc/current/contributing/code/standards.html page must reflect our new policy.

@ostrolucky
Copy link
Contributor Author

Sure, but not immediately. Similar work in upper branches need to be done before rule can be replaced, which I volunteer to do as well. This PR is first step.

@gmponos
Copy link
Contributor
gmponos commented Dec 28, 2018 •
edited
Loading

@ostrolucky recently I discovered this https://github.com/symfony/phpunit-bridge/blob/master/composer.json#L19

@ostrolucky
Copy link
Contributor Author

yeah that's a good point. We can exclude this for phpunit bridge

@gmponos
Copy link
Contributor
gmponos commented Dec 28, 2018

We can exclude this for phpunit bridge

Although travis is not checking versions below 5.5 so it could be broken already.. maybe someone could advise if supporting 5.3 is by design or not since symfony officially supports 5.5 and above...

if it is not by design... then maybe if this could be considered as a bug. Then submit a PR to throw an exception on simple-phpunit file if the version is below 5.5

@nicolas-grekas
Copy link
Member

PHP 5.3 support was required to test Symfony 2.8.
I think we can bump the bridge to 5.5 in Symfony 4.3.

@gmponos gmponos mentioned this pull request Dec 28, 2018
Merged
fabpot added a commit that referenced this pull request Jan 1, 2019
@fabpot
feature #29718 [PHPUnit bridge] Bump php version of PHPUnit-bridge (g…
c4d6d15 
…mponos)

This PR was squashed before being merged into the 4.3-dev branch (closes #29718).

Discussion
----------

[PHPUnit bridge] Bump php version of PHPUnit-bridge

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no (?)
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

Related to #29623 (comment)

I checked the BC page I think I couldn't find any related to BC changes... anyway...

As far as I understood from the comment above I must target `master` branch.

Commits
-------

5931309 [PHPUnit bridge] Bump php version of PHPUnit-bridge
@nicolas-grekas
Copy link
Member

I'm 👍 for the change, but this will make almost all pending PRs conflict (see how many its already has).
I think the only way to do it is to have a merger do it using php-cs-fixer:

  • first merge all branches up to master
  • then run php-cs-fixer on 3.4
  • merge 3.4 into 4.1 and revert all changes while doing so
  • run php-cs-fixer on 4.1
  • etc up to master

To save us some hassle, I'd wait for 4.1 to be EOLed personally before doing so.

Meanwhile, what about enabling the short array syntax in fabbot right now, to help rebased/new PRs be ready? @fabpot wdyt?

@ostrolucky
Copy link
Contributor Author
ostrolucky commented Jan 4, 2019 •
edited
Loading

You can do it that way for all php-cs-fixer rules, or at least those affecting single line only. I thought it's not being done that way for some reason? But yeah otherwise I'm 👍 for that

But, it's not so simple, as you need to take special care for things like fixtures of dumped containers.

see how many its already has

Yes, I though this will be merged quicker. You can't let PR like this sit around for 20 days. It was 0 conflicts in beginning all the way up to master. Rebasing this now would be difficult.

@ostrolucky
Copy link
Contributor Author

Superseded by #29903

@ostrolucky ostrolucky closed this Jan 16, 2019
@ostrolucky ostrolucky mentioned this pull request Mar 26, 2019
Closed
@ostrolucky ostrolucky mentioned this pull request May 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

@lyrixx lyrixx Awaiting requested review from lyrixx lyrixx is a code owner

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@ostrolucky @fancyweb @gmponos @nicolas-grekas @carsonbot
0