[RFC] Use Array PHP 5.4+ notation in cms 4.0 branch

[andrepereiradasilva]

Made several PR in 4.0 to change Array to PHP 5.4+ notation.

I followed what as as been done in framework 2.0 branchs and many PHP projects i saw to make code a little more modern.

Is just code Style, so was asked to discuss here.

Opinions?

[Bakual]

Personally I don't care if an array is created using array()or []. I don't think it's about being "modern" code, it's just personal flavor what you want to use.

I think it makes sense to standardise on a way thought. However please don't do it right now in the 4.0 branch. It will only create unneeded conflicts. Also having different codestyle rules between staging, 3.8 and 4.0 isn't wise at this time because developers switch between both branches quite a lot and code gets backported from 4.0 to 3.8/9.

Once 4.0 is merged into staging and becomes the main branch, it will be fine to apply new codestyle rules and change the code. Personally I would even wait till after 4.0 is released since it will likely affect less PRs at that time.

[frankmayer]

The short style array syntax is much better readable and distinguishable. Especially if you have arrays that are inside parentheses for any reason. I believe It's not just personal flavor.

As for when the changes should be merged, that would depend on how many conflicts there really will trigger, because of these specific changes. Maybe it is not that bad, as we are thinking it is.

Personally I would like to set a code style and the relevant code changes for 4.0 prior to its release and also have people get acquainted with the new rule as they see it in the 4.0 branch. PR's will then hopefully mostly comply to the short syntax rule, even if the submitter missed reading the guidelines.

[andrepereiradasilva]

first let's me say i made the first PR to test the waters. since @wilsonge merged the two first and @frankmayer and @Quy were code reviewing them (thanks by the way) i assumed the change is accepted.

Also we are already getting the two array notations in 4.0 branch, so IMO this should be standartized. And standartize in a old notation array() is a bad choice IMO, so that leave us with the the new notation []. I also agree with @frankmayer that is more readable and that will not exist as many conflicts as you think.

i also think @wilsonge and @mbabker opinion, since they are the release leds for 3.8, 3.9 and 4.0, is important.

@Bakual sorry i will not be able to mantain those PR until 4.0 is merged into staging of course that's not a project problem :), but if that's the decision i will have to close all PR (or someone took over) because i will not have time to mantain them (was hoping for a fast merge). of course, others can do it.

[Bakual]

The short style array syntax is much better readable and distinguishable. Especially if you have arrays that are inside parentheses for any reason. I believe It's not just personal flavor.

It is personal flavor for sure. It's only better readable if you are used to it. But it's something one gets used to fast, especially since JavaScript (and probably other languages) allow that syntax as well. So I'm not opposed in general, don't get me wrong here :smile:

As for when the changes should be merged, that would depend on how many conflicts there really will trigger, because of these specific changes. Maybe it is not that bad, as we are thinking it is.

Keep in mind that we also backport things from 4.0 to 3.x. You would need to change the arrays back. Sounds like a lot of potential issues and work for no reason when applied to early. There is absolutely no hurry for that change, so lets take the time with the least impact to change it.

PR's will then hopefully mostly comply to the short syntax rule, even if the submitter missed reading the guidelines.

Once we decide to use the new style we will need a codestyle sniffer and PRs will have to comply to that rule.

[Bakual]

And standartize in a old notation array() is a bad choice IMO

It's not "old" vs "new". It's just a short syntax for the same thing. The existing "regular" syntax is not going away in a future PHP version.

[andrepereiradasilva]

yes, right, sorry for the bad express of words. what i mean is i'm seeing a trend in php 5.4+ projects to move to the "short" syntax. including joomla (see for instance framework 2.0 branch https://github.com/joomla-framework/filter/blob/2.0-dev/src/InputFilter.php) so the "regular" notation IMO will be perceived as the "old" notation.

[mbabker]

On my part, the reason I try to use newer language features is to purposefully break support for older PHP versions since either the parser won't understand some syntax or some method gets used that's not available. I do feel that short array syntax is also a better choice when it comes to working with frontend, it's the same as JavaScript.

So I personally prefer it. But I wouldn't start bulk changing it for the sake of it in the CMS repo. Changing it in Framework repos has caused me a few headaches doing branch merges and that is a much smaller set of code. The way Symfony handles things generally is newer features can use newer language syntax, but people aren't going through their repo often changing array syntax or using the ::class notation over writing out the FQCN as examples.

[andrepereiradasilva]

ok, if you guys don't agree to merge now because of potencial conflicts i will stop proposing those changes. i just think the potencial for conflicts will always exists no matter when you do change like this.

so what shall i do with the existing PR's? close them? (i have 3 more branches with tests folder changes ready that i didn't submit PR because of this RFC).

Note: keep in mind that my PR's didn't touch the 3 big folders "administrator/components", "components" and "libraries" (which probably are the ones with most potential conflicts). i was leaving those for last.

[andrepereiradasilva]

Just an additional note: if a cs rule is estabelished as discussed above to use short notation will it not force to do the "bulk changes" ?

[mbabker]

I don't think we have to do a "short array syntax required" rule. For me it's perfectly fine to introduce it in new code, but we probably shouldn't do a bulk change for the sake of it. George and I already have enough fights doing branch merges with the renamed files, Composer differences, and changes already landed in the new versions. These changes, while I personally have nothing against and do on smaller code bases just because I can (and they usually aren't disruptive), would probably drive us both to the loony bin.

[andrepereiradasilva]

well i tried. closed all PR didn't delete the branchs if anyone interest in the future can reuse them.

[Bakual]

if a cs rule is estabelished as discussed above to use short notation will it not force to do the "bulk changes" ?

Travis used to only check the lines which got changed in a commit/PR, so only those had to comply with our codestyle rules going forward. I'm not sure how it is with Drone, but probably similar.

[mbabker]

CI platforms run the full test suite. We aren't doing anything to only run tests on changed files.