8000 [3.0] [] for array syntax · Issue #12456 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[3.0] [] for array syntax #12456

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
samsonasik opened this issue Nov 11, 2014 · 39 comments
Closed

[3.0] [] for array syntax #12456

samsonasik opened this issue Nov 11, 2014 · 39 comments

Comments

@samsonasik
Copy link
Contributor

Based on http://symfony.com/blog/symfony-3-0-the-roadmap , since Symfony 3.0 will use PHP 5.5 as minimum requirement. Will then the array() syntax be changed to [] ?

There is good tools for that : https://github.com/thomasbachem/php-short-array-syntax-converter

@fabpot
Copy link
Member
fabpot commented Nov 11, 2014

I would refrain from making such changing in the core, at least for now. We have much more important things to do and it would (potentially) make merging 2.x branches into 3.x ones more complex.

@samsonasik
Copy link
Contributor Author

Ok, thanks ;)

@tseho
Copy link
Contributor
tseho commented Nov 11, 2014

fabpot +1, there is no real interest in this.
It's just a new syntax which is not spread in PHP projects.

@javiereguiluz
Copy link
Member

@tseho I know a lot of developers interested in changes like this. It's a minor change from a technical point of view, but it delivers a strong message about using modern PHP versions.

And you are right about not being spread in PHP projects. That's because people were afraid of moving beyond 5.3. Now that Drupal 8 (5.4), Symfony 3 (5.5) and other big projects are finally upgrading, we could see a quick upgrade movement soon.

@linaori
Copy link
Contributor
linaori commented Nov 11, 2014

When working on 3.0 changes (BC breaks), it should be very well possible to do so already if you know your code won't be merged back.

@xabbuh
Copy link
Member
xabbuh commented Nov 11, 2014

I think it doesn't hurt to use the new syntax for things that are introduced in 3.0. Though I wouldn't change existing code. It would make merging from 2.7 into master a nightmare I guess.

@wouterj
Copy link
Member
wouterj commented Nov 11, 2014

I'm -1 for this syntax. It's less clear, less verbose and doesn't have any advantages

@perajovic
Copy link
Contributor

+1, but it is not a right time.

@GrahamCampbell
Copy link
Contributor

👍 for this syntax

@kingcrunch
Copy link
Contributor

+1 for new code. There is no advantage to change existing code and on the other hand it "breaks" the codes history for no reason.

@tseho That it is not widely used in PHP projects is only partially true. It's not widely used in public projects, because of course they want to keep BC. All closed source code I worked on recently used this syntax for new code. It's something different if you know the platform it has to work with.

@GrahamCampbell
Copy link
Contributor

Most open source projects I see that have php 5.4 as a min version do actually use the short array syntax.

@wouterj
Copy link
Member
wouterj commented Nov 11, 2014

-1 for a mix. Inconsistency is the worst thing in a code base imo

Op di 11 nov. 2014 22:42 schreef Sebastian Krebs notifications@github.com:

+1 for new code. There is no advantage to change existing code and on
the other hand it "breaks" the codes history for no reason.

@tseho https://github.com/tseho That it is not widely used in PHP
projects is only partially true. It's not widely used in public
projects, because of course they want to keep BC. All closed source code I
worked on recently used this syntax for new code. It's something different
if you know the platform it has to work with.


Reply to this email directly or view it on GitHub
#12456 (comment).

@GrahamCampbell
Copy link
Contributor

@wouterj. I agree there. All or nothing. We have an opportunity for a clean split in 3.0. No point in just making new code use the short array syntax.

@linaori
Copy link
Contributor
linaori commented Nov 11, 2014

I'm for [] all the way:

if (hello() || (true && doSomething([[1, 2, 3], [4, 5, 6]])));

if (hello() || (true && doSomething(array(array(1, 2, 3), array(4, 5, 6)))));

The amount of parenthesis will become hard to follow. [] gives a far more clear representation of what belongs to which.

$a = array_merge([$this->something()], $input->something([$this->getSomeKey()]));

$a = array_merge(array($this->something()), $input->something(array($this->getSomeKey())));

Far more readable.

Just don't put it in code that will be merged back to 2.3, 2.5, 2.6 or 3.0

@tseho
Copy link
Contributor
tseho commented Nov 11, 2014

@kingcrunch I'm not against using this syntax in closed source projects or in new libraries for PHP 5.5+, I'm just thinking it's a not a good idea to use it in Symfony3, at least for now.

If the community start using it only for Symfony3 specific code, we are loosing consistency inside the project.
If we update all the components to this syntax, merging between 2.x and 3.x will be more complex.

@HardieBoeve
Copy link

+1 for the short array syntax in Symfony 3

@Taluu
Copy link
Contributor
Taluu commented Nov 12, 2014

I'm also 👍 if the syntax is widely used, but not if it is only the newer code, for the same reasons @wouterj and @fabpot mentionned. Even though I'm using it everywhere.

@yguedidi
Copy link
Contributor

@iltar I don't think there will be some code that will be merged back to 2.7, there already is a dev branch for 2.7.

@mayeco
Copy link
Contributor
mayeco commented Nov 13, 2014

👍 for new arrays [] syntax

@stof
Copy link
Member
stof commented Nov 13, 2014

I don't think there will be some code that will be merged back to 2.7, there already is a dev branch for 2.7.

@yguedidi it is the other way actually. 2.7 will receive bug fixes for a long time after the 3.0 release, given it will be a LTS. This means that the 2.7 will be merged into the 3.0 branch on a regular basis (just like done currently for the 2.3 branch)

@jderusse
Copy link
Member

👎 for switching now. Because mixing new and old syntaxe in the same project is a bad idea, and because updating the >20 000 ocurances of array( will make merging very hard.

@yguedidi
Copy link
Contributor

@stof Yes I know, I just said to @iltar that nothing will be merged back to 2.7 from master.

@mattattui
Copy link
Contributor

I think it's a great idea to do this (readability++) but only when all supported Symfony versions are pho 5.4+. I guess that's when 3.3 comes out and supersedes 2.7?

@dirkluijk
Copy link

👍 for using the [] syntax consistently in 3.0. It will be a great change when it comes down to the number of occurrences (both in code and in docs); but I think it's good to adapt to the new PHP 5.4+ features for the new Symfony version.

@cjunge-work
Copy link

+1 for new components, -1 for updating all instances of array().

And from a practical POV, mixed syntax is a necessity, otherwise there will be changes to code simply for syntax purposes, and that seems a waste of resource to me.

Maybe 3.3+ changing existing code can be done when changes are implemented in various components, and then when some percentage is done we can migrate the test.

It seems to me there will be more negatives by changing code for components that aren't updated often, until a majority of code uses the new syntax. I'm sure devs using the new syntax are also aware of/use/are comfortable with a mix of old & new.

@kingcrunch
Copy link
Contributor

From my work experience mixing both notations (even in a single file) is less an issue, than some wants to tell here. We are not talking about completely different constructs, but just a different, but similar notation

$foo = array('a','b', 'c');
$bar = [1,2,3];

doSomething($foo, $bar);

I'd still consider using the new notation for new code and keep the old one for untouched code. One day -- with 3.3 for example -- one can change the remaining occurences, or something like that. Or just keep it as it is 😉

@GromNaN
Copy link
Member
GromNaN commented Nov 17, 2014

👎 replacing array() by [] doesn't add value to the existing code while it adds a lot of complexity such as determining the array syntax to use in a PR depending on the target branch. And what appends when the target branch is changed ?

And this concerns only internal code, this has no impact on the developer interface of the framework.

The only place where it could make sense to use the short array syntax is in the documentation; where the code samples are made to be read.

@linaori
Copy link
Contributor
linaori commented Nov 17, 2014

@samsonasik I think the question is a bit vague. @javiereguiluz I think the questions asked should be:

  • Should we replace all occurrences of array() with []?
  • Should we add [] instead of array() in pull requests that are only for 3.0?

In my opinion, replacing is not a good idea. This will make it very hard to merge back into 2.3, 2.6 regarding bug fixes and merging it into 3.0 will cause new array() usages, which then need to be patched. Features for 2.7 should be based on the 2.7 branch and not the master, therefore it should be obvious to use array().

When it comes to pull requests to 3.0 and 3.0 only (thus no merging back to 2.*, 3.0 from 2.7), I think it's a good idea to already include the short hand notation. My reasons are simple; It makes code a lot more readable and shorter to write.

@ByScripts
Copy link

+1 for new code
-1 for old code (why to bother?)

@yguedidi
Copy link
Contributor

I'm 👍 for changing array() to [] in master, but only once Symfony 2.7 LTS is released.
Then, all new features will be added to master with the [] syntax.
Only bugfixes in 2.7 could cause a merge conflit when merging in master, but I think it's not a huge pain to resolve them.

@Taluu
Copy link
Contributor
Taluu commented Nov 17, 2014

@iltar As it was said multiple times, nothing will be merged from the master (3.x) branch into the 2.x branches... the reverse is not true though. If a bug fix is also concerning the 2.x serie, then it should first be based on the 2.x branches and only then merged on 3.x (mainly for compatibility reason)

As it is the case right now (i.e you do not backport something from 2.6 into 2.5)

@linaori
Copy link
Contributor
linaori commented Nov 17, 2014

@Taluu where did you read that I said stuff should be merged from 3.0 to 2.7 and lower? That was not my intention. There are changes that can only be done for 3.0. Those are the changes that I think can get the short notation instead.

@Taluu
Copy link
Contributor
Taluu commented Nov 17, 2014

In my opinion, replacing is not a good idea. This will make it very hard to merge back into 2.3, 2.6 and 2.7 regarding bug fixes. Features for 2.7 should be based on the 2.7 branch and not the master, therefore it should be obvious to use array().

I may have misinterpreted. If that is the case, I'm sorry. :)

If you meant the nerge from the 2.x branches (such as the conflicts mentionned by @yguedidi), that is one of the reason why I'm against this change (even though I'm using it everywhere else in my projects...)

@linaori
Copy link
Contributor
linaori commented Nov 17, 2014

@Taluu I was pointing at merges from 2.7 back into the lower branches, I should not have stated 2.7 with 2.3, etc. I'll change that to avoid more confusion.

Edit: I've updated my reply above, I hope it's more clear now :)

@yguedidi
Copy link
Contributor

@iltar

Features for 2.7 should be based on the 2.7 branch and not the master, therefore it should be obvious to use array().

That's why I propose to make the syntax change only once 2.7 is released :)

@rvanlaak
Copy link
Contributor

👍 for changing array() to [] in master, when Symfony 2.7 LTS is released.

Readability as @iltar shows in #12456 (comment) sounds like a win in comparison with the 'polluted' commit history to me.

@piotaixr
Copy link

Personally, i find the short syntax better when the array is inlined, but as soon as you put the array on multiple lines, i prefer the array() notation...

@linaori
Copy link
Contributor
linaori commented Nov 22, 2014

Talk about inconsistencies ;)

@fabpot
Copy link
Member
fabpot commented Dec 24, 2014

Closing as that won't happen. We are using array() internally and we will keep doing that for consistency. Use [] in your own code, but there is no need to change the whole Symfony code base.

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

No branches or pull requests

0