Convert all array usage to 5.4 short array syntax #4463
Conversation
@sminnee mentioned that we can now use short array syntax in silverstripe#4462 This change coverts the framework codebase to the short array syntax, which not only makes the code nicer to read IMO, but also will encourage future contributors to use the 5.4 syntax when contributing.
|
Minimum PHP version will also need to be updated: https://github.com/camspiers/silverstripe-framework/blob/array-syntax/composer.json#L19 And in docs too. |
|
Yeah I had done this, unfortunately it's buried in the config PR i did. I might extract out to its own PR and cross-post to ss-dev for added visibility. |
|
I think this PR can be left as-is but we'll merge it second. |
|
What is the advantage of short array syntax? I, personally, think it's much less readable myself. Also, it wilbreak any potential 5.3 comparability for a fairly arbitrary reason. I'm not a fan of the short syntax because it does cause needless breakages and makes libraries that could be 5.3 compat break |
|
Good questions @dhensby The only advantage in my eyes from a language perspective is that it is more terse (and IMO more readable). But there other advantages to introducing a change like this against master at the earliest possible time. It sends a clear signal to the community that PHP 5.4 is required for SS4. PHP 5.3 is no longer supported by PHP (not even for security releases, these ended 1 year ago). Attempting to maintain compatibility with 5.3 limits what SilverStripe can reuse from the PHP community, and also what languages features SilverStripe 4 can use in general (no traits for example). SilverStripe has already tentatively decided that 5.4 will be the lowest supported version for SilverStripe 4, (and it looks likely the only direction they will move is up not down). Your preferences are possibly biased by what you are used to. I have been using PHP 5.4+ since 2012 so the short array syntax is much preferred by me, and I suspect you will also grow accustomed to it. People in the PHP community really need to start watching how PHP intends to release over the coming years. It is going to be on a much faster cycle, and the stagnation that we are currently seeing is not going to go well. 5.3 for example is unsupported, 5.4 only has security support now, and that support will actually end this September! PHP 5.5 has been out for 2 years, and now only has security support which ends July next year and PHP 5.6 was released almost a year ago. PHP7 is on the horizon and will be released this year (which is going to be amazing in what performance benefits it brings). In my opinion considering SilverStripe 4 will not be released until after PHP7 is release, SS4 should actually have PHP 5.6 as its minimum version (this will unlock numerous useful language features that can be leveraged), and if it really is high priority to provide some kind of compatibility with 5.3-5.4 then SilverStripe could invest in something like https://github.com/endel/galapagos to provide a compatible version (this would be like what the JS community has been doing for years with source-source transpilers). PHP7 is going to have a huge impact, and I am hoping demand for it will really improve the hosting ecosystem. It will also save hosting providers money as it brings significant efficiency benefits that will lower power consumption and CPU utilization. PHP7 is already on its second beta :) download here: http://php.net/archive/2015.php#id2015-07-24-1 |
|
I agree with @camspiers about a minimum of PHP 5.6 for SS4.0. Upgrading an existing site to SS4 will be a pretty big change so I don't think it's unreasonable to require a relatively small change to the hosting environment - especially given the security implications Cam outlined. |
|
If you are interested in how PHP versions are managed here is the RFC that was accepted in 2011: This documents the support of current releases: Here is the PHP7 timeline: |
|
This site has an awesome crowd-sourced list of PHP versions supported by hosting providers if you are struggling to find a host with a modern version of PHP: |
|
We need to be very mindful that whatever minimum PHP version we choose for SS4 is what we will have as the minimum version for the whole of SS4. It's not just 4.0, 4.1 etc, but any release within the major version 4. If SilverStripe is happy following projects like Guzzle, babel etc, and have no issue incrementing major versions at a rapid pace, then this of course will be no issue, but if not we should be forwards thinking, paying special attention to the release cycle of PHP and when security support for our minimum version end. |
|
If we’re making SilverStripe 4 PHP 5.4+ only then I don’t have any objections to this change, it’s kind of a stylistic thing that just takes a little while to get used to IMO, same as different coding conventions for example. I don’t really agree with requiring 5.6 for SilverStripe 4, though. Especially not just for the sake of it. Yes, ideally we’d all be running PHP 5.6 but the reality is very different. Probably worthy of a dev list discussion of its own, but in my opinion trying to encourage people to upgrade by making our software unavailable to them is a pointless exercise unless we actually need some of the new language features. PHP 5.5 security support probably will be ending by the time SilverStripe 4 comes out (roughly speaking), and anyone still on 5.5 will hopefully be looking to upgrade to 5.6 around that time, so I think we should at least support 5.5 to allow a grace period. |
I don't think anyone is proposing things just for the sake of it.
This is a good point, plus 5.6 doesn't add a huge amount over 5.5 (const arrays, const expressions, variadics). Though variadics would tidy up and improve performance of some important SS code (think Object, ViewableData etc). https://github.com/silverstripe/silverstripe-framework/search?utf8=%E2%9C%93&q=func_get_args&type=Code |
I don't think SilverStripe needs any language features from 5.4 (nor 5.3 really), so I don't really see how that is relevant. It isn't a need, which is why I think most compelling arguments for 5.5 minimum (or even 5.6), are actually related to security, PHP support, and moving the community forward in general. |
|
For inspiration, this is where the Symfony community landed with respect to PHP versions (the vote was in 2014, they decided 5.5). Symfony is much closer to their 3.0 release that we are to SilverStripe 4.
|
|
OK I've put something about dropping 5.3 support on ss-dev: |
|
👍 |
Sorry, that was a rubbish choice of words. What I was getting at is that I don’t think we can decide on a minimum version of 5.6 for SilverStripe 4 now. We’re proposing 5.4+ at the moment because there are specific language features, and thirdparty libraries, that we want to use. SilverStripe 4 doesn’t have a fixed release date (only a target) so if it comes sooner we may want to leave our PHP requirement lower, or if it comes later we may want to say 5.6+. That said, we definitely won’t be releasing it before September when 5.4 support ends. I’m not familiar with the “long term support” options that Sam mentioned in the dev-list post, how long would these extend the life of 5.4 by? Enough to warrant us supporting it? My guess is that’s unlikely, and given the point above about having to support it for all 4.0 releases, we should definitely be considering dropping 5.4 too...
Definitely,
Point well taken. |
|
I suppose my point applies more to PHP libraries or components that make no use of features in 5.4 or up other than syntax changes as you're arbitrarily locking out projects that may want to use your component and contribute back just for the sake of a new syntax. With SS this is, perhaps, moot. As we will make use of more than just short array syntax and its not component based... Yet. I am personally not a fan of the short syntax, its definitely less explicit and obvious, though I don't dispute if you've used it for 3 years you'd find it familiar. When short PHP open tags were added I'm sure people loved those too, now look where that got us. My only real strong view point on the PHP version is that we should definitely support the same PHP version that comes in the enterprise Linux repos. If we want to be taken seriously, we need to run on those OSs IMO |
|
CentOS/RHEL 7 ships with PHP 5.4, which is probably the biggest barrier. Although there are better facilities these days for getting newer versions of packages for existing versions of RHEL, this may be met with resistance in some environments: people using RHEL will have to ask for this change to be made, it might make change-averse infosec people worried, etc. However, given that I've seen IE6 rolled out until 2014 in some places, I'm not sure that supporting the worst-case scenario is necessarily the best idea. ;-) To answer Loz's question about "long term support" options:
I'd probably advocate for PHP 5.5 support once there is a clear reason to do so, most likely because a package that we could bring in as a dependency to reduce the amount of code we maintain requires PHP 5.5+ on its master branch. |
|
All really great points!
Hehe. While I think the comparison is humorous, I don't think short-array syntax is going to go away, I suspect we will even get a terser version of lambdas following Hack in 7.1.
I think this is false. Are we to believe Symfony, Laravel and Zend Framework won't be taken seriously given that their minimum versions are all > 5.5? |
|
Once we get 👍 on 5.4 (I've given people until the end of the week to object on ss-dev) I think this is good to merge. I much prefer the terse syntax; it's more in line with other languages and reduces the amount of noise in things like parameter-maps passed as function arguments. However, this a bikeshed argument. I'm happy to go with the consensus. Does anyone else share @dhensby's preference for the longer |
|
FYI, on PHP version history, here are some stats from a sample of SilverStipe new installations over the last 12 months (install.php has an optional callback to report stats)
On the basis of that I'd be pretty nervous about dropping PHP 5.4 support. We'll check again in 6 months, I guess. |
|
IMHO it's the old chicken-egg problem. People (me included) won't update their machine until they really have to. Even latest Debian supports php5.5 out of the box (and you always have other repositories with up-to-date versions). |
|
@sminnee that is really useful information. Thanks! |
|
Let me know when this is ready to land and I will rebase. |
|
@silverstripe/core-team can we get some views on whether it's good for use to recommend short array syntax from ss4, and therefore merge this PR? So far I've got: +1 from me |
|
I'll admit that its pretty subjective. My justification is that more explicit is more clear and that full array syntax is more clear. I assume the PSRs don't dictate one way or another? As that would be the trump card... |
|
I don't think PSR2 makes any mention of it. ZF2 and laravel both use the short array syntax. Symfony uses the older array syntax. I'm happy to abandon this if there isn't consensus. I completely admit that this change has no code benefits beyond certain peoples personal preference. While I see this as providing a clear signal within the codebase that 5.3 isn't supported, it isn't for me to decide the coding standards of SilverStripe. On a related note, does SilverStripe intend to adopt PSR2? |
|
I don't feel strongly one way or another between the two syntaxes. It will make merging up from 3 branch harder, and even though it's simpler, it is less explicit. I've always held that fewer characters != less code. It's just expressed differently. I'm not against NEW features using the short array syntax though. :D I weigh in with @dhensby then as -1. I would change to +1 if we had some solution to the merging issue, though. |
|
The issue that I have with just using it for new features is that we'll have an inconsistent codebase. I'd prefer that we had a coding standard for ss4 that recommended one approach or the other. Personally my advocacy for using short array syntax is that it reduces the noise in complex array blocks—you can see the information, not just the word "array" over & over again—and it is more consistent with JavaScript, which a SilverStripe developer will also be developing in. Because of the JavaScript consistency I find it extremely unlikely that developers will have difficult interpreting short array syntax. Fair point on the 3->4 merge issues, but with all the other changes planned for 4, is this actually likely to be an issue? And if forced to choose my preference would be a consistent codebase over easy merges. I'd like to get a view from others in the core team on what array syntax should be our recommendation for ss4+? |
|
-1 from me. 👎 I would encourage the use of I think |
|
pro: it's like ruby, javascript, python, c and a basically any other "c" type language Personally I've started doing this for some modules and functionality that is not core, but doing this for the core is a big statement that will make it harder to merge security fixes or improvements. That said, I'm for this since it will imply things like: Basically see https://groups.google.com/forum/#!topic/silverstripe-dev/7mVvhNNeNps, this is one part of it |
|
+1 from me, less ugly syntax. I think @tractorcow's vote should count twice though, since he'll likely have the majority of merge hassles. I'm willing to take over the odd merge as "payment" for my vote though :D @camspiers Have you changed code examples in Markdown as well? Diff is too big to check this online. |
|
I also do merges sometimes! :) |
|
I think this raises a good point that we actually need our own set of coding standards that we should probably apply on top of PSR coding standards (when we adopt them). |
|
@chillu I haven't. That is a good point. This was a automatic codemod. Do you think that would be better done as a separate PR? |
|
"I think @tractorcow's vote should count twice though" Great point, fully agree. |
|
@camspiers Yeah separate PR would work - it makes it easier to review :) |
|
@chillu You are going to manually review 3122 changes? legend. ;) |
|
is this going anywhere? if so, it needs re-doing. I guess in the name of "progress" we should do this? however it means different coding standards across different branches - I don't like that. perhaps we leave this until support for 3 is dropped? |
|
I think that given both the age of this pull request, and the fact that it's not going to get merged any time soon, we should probably close it, sorry. If and when we do this conversion, we can do it closer to release of 4.0 using an automated conversion tool, so no need to rebase this PR. |
|
I left this how it was because I didn't feel I got any consensus on whether we should do it. When someone attempts this in the future, use this tool as I did: https://github.com/thomasbachem/php-short-array-syntax-converter |
@sminnee mentioned that we can now use short array syntax in
#4462
This change coverts the framework codebase to the short array syntax,
which not only makes the code nicer to read IMO, but also will
encourage future contributors to use the 5.4 syntax when contributing.