Problem/Motivation
As it's been decided we're going for PHP 5.4 for Drupal 8, we could switch to the short array syntax provided by PHP 5.4. That is
["apples", "oranges", "bananas"]
instead of
array("apples", "oranges", "bananas")
Pros:
- Developers already know the square bracket syntax from other programming languages.
- When closing an array statement with "]" you can more easily distinguish it from closing function round brackets, example:
$x = t(\Drupal::config('foo')->get('message'), ['@bar' => \Drupal::config('bar')->get('name')]);
Cons:
- Patches are harder to backport to Drupal 7, which puts more work on the security team when working on multiple security patches.
Proposed resolution
Current standard:
# Arrays
Arrays should be formatted with a space separating each element (after the comma), and spaces around the => key association operator, if applicable:
$some_array = array('hello', 'world', 'foo' => 'bar');
Note that if the line declaring an array spans longer than 80 characters (often the case with form and menu declarations), each element should be broken into its own line, and indented one level:
$form['title'] = array(
'#type' => 'textfield',
'#title' => t('Title'),
'#size' => 60,
'#maxlength' => 128,
'#description' => t('The title of your node.'),
);
Note the comma at the end of the last array element; This is not a typo! It helps prevent parsing errors if another element is placed at the end of the list later.
The use of PHP 5.4+ short array syntax for Drupal 8.x is being discussed and is used in some patches already. When used, try to apply it consistently to at least a whole method or function. Short array syntax should not be used in Drupal 7.x or 6.x core or contributed modules.
New Standard:
# Arrays
Arrays should be formatted using short array syntax with a space separating each element (after the comma), and spaces around the => key association operator, if applicable:
$some_array = ['hello', 'world', 'foo' => 'bar'];
Note that if the line declaring an array spans longer than 80 characters (often the case with form and menu declarations), each element should be broken into its own line, and indented one level:
$form['title'] = [
'#type' => 'textfield',
'#title' => t('Title'),
'#size' => 60,
'#maxlength' => 128,
'#description' => t('The title of your node.'),
];
Note the comma at the end of the last array element; This is not a typo! It helps prevent parsing errors if another element is placed at the end of the list later.
Please note, short array syntax is unsupported in versions of PHP prior to 5.4. This means that Drupal 7 core and Drupal 7 contributed projects without an explicit PHP 5.4+ requirement must use long array syntax.
Remaining tasks
- Gather votes on the proposal
Comment | File | Size | Author |
---|
Comments
Comment #1
drifter CreditAttribution: drifter commentedComment #2
drifter CreditAttribution: drifter commentedI've used Thomas Bachem's short array syntax converter to run through the Drupal codebase and convert arrays to the short notation. It's a big patch, and testbots might not support 5.4 yet, but it seemed to do a good job.
Comment #3
drifter CreditAttribution: drifter commentedComment #5
klausiNot sure we should do this.
Comment #6
Crell CreditAttribution: Crell commentedThis patch is predicated on the idea that Drupal Must Only Use One True Array Syntax For All The Things(tm). That is a completely wrong approach.
Once we actually have testbot running on 5.4, we should start using short-array syntax "where it makes sense". There is absolutely no runtime problem with the existing syntax. If you're touching a piece of code and feel like converting to short arrays in the process, fine. But a mega patch like this has no benefit other than breaking hundreds of existing patches in the queue. Don't do that.
Comment #7
donutdan4114 CreditAttribution: donutdan4114 commentedI support this initiative.
At the very least, if we could have the official "Drupal Coding Standards" note that either syntax is 100% acceptable in D8.
Currently in D8 (including Symfony components), there is roughly 52900 occurrences of array long-syntax:
grep -roh 'array(' . | wc -w
That means in the codebase, developers have typed an extra 264500 characters.
That's equal to ~2116KB worth of data.
For a good DX, I prefer short-syntax.
Comment #8
superspring CreditAttribution: superspring commentedComment #9
rob3000 CreditAttribution: rob3000 commentedI'm happy to give this one a go :)
Comment #10
Crell CreditAttribution: Crell commentedWe're not going to break every patch in the queue by forcing all of Drupal to short-array syntax at once. Not OK.
I would propose instead the following change to the coding standards:
That allows us to piecemeal transition to short arrays over time as we are working on code. (Ie, if you're futzing with a given class, you can switch it while you're at it. Or just switch one class at a time. etc.)
Comment #11
sunIt's not clear to me why we need to formulate any kind of standard or recommendation for this to begin with.
The only part that makes sense to me would be the part about "try to stay consistent within a single file." — But that's not really limited to this topic in the first place.
...is pretty worthless and senseless to document.
Also, the consistency part pretty much means that D8 core will not use the short-hand syntax - unless we'd convert all of core - for which I do not see a justification.
Comment #12
grom358 CreditAttribution: grom358 commentedWith pharborist I can create a patch todo this in couple lines of code. As @Crell points out the issue is when can this patch be created and applied. It takes my script about 5 minutes to create a patch, just need to coordinate a time can disrupt all the patch queue.
Comment #13
damiankloip CreditAttribution: damiankloip commentedI think I certainly agree with #10 & #11; It seems non-sensical to do a mega patch and replace every instance of array() in core = for no real gain. This is not really a massive DX win. It makes sense to use this if you want, keeping consistency in a file.
Comment #14
grom358 CreditAttribution: grom358 commentedarray_syntax_convert.php is the code for automatically doing this. So what I propose if this wants to be done. Is apply a patch from the patch queue. Then run this script to then reapply short array syntax. That way we don't disrupt the patch queue.
Comment #15
webchickI really lean towards "won't fix" on this, sorry. The DX to workflow disruption trade-off is not worth it. In fact, it may even make it worse.. while ["..", "..."] may be standard a few years from now, for the past 15 years or whatever PHP arrays have been "array()" so I could see this change really confusing people and unable to google it to figure out what's going on (google doesn't like "[" as a search term :D).
Comment #16
Crell CreditAttribution: Crell commentedI still stand by "use it, don't use it, whatever, both work."
Comment #17
sunWhat @Crell said. We have instances of the short array syntax in HEAD anyway already, and more instances are guaranteed to follow.
It's a native syntax of the PHP language. — Coding standards only rule how you're supposed to write something if you write something. They do not rule what you are allowed to write.
Due to that, "won't fix" is not really within the list of available options — unless the issue title is interpreted literally, since "favoring" one option over the other is no business of coding standards either.
We can decide to "Be consistent within a file.", or we can decide that we do not care at all.
In either case, that's a docs change only (no patch).
Comment #18
pwolanin CreditAttribution: pwolanin commentedFor Druapl 8 core, I think the array() syntax should be used for code we write. I suggest that as the revised coding standard.
I'd agree that contrib any use that's consistent within a file is ok.
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commented+1 to what Crell said in #16
Lets not care at all
Comment #20
donutdan4114 CreditAttribution: donutdan4114 commented"Let's not care at all" is not a good approach.
Perhaps this type of change is too revolutionary for Drupal 8 (9 perhaps?)
I don't see how documenting this is any different than the numerous other standards Drupal has specified.
"Short array syntax is preferred." is all we need.
Using
[ ... ]
greatly reduces the number of characters a developer has to type.It decreases the length of code for arrays passed to functions, Drupal's renderable arrays, etc.
Comment #21
damiankloip CreditAttribution: damiankloip commentedWe should not enforce this either way. As mentioned before by many others, we should keep this consistent within a file. Its not worth doing a mass conversion, or breaking most patches in the queue. There are many many more important things that need to be done.
Sun is totally right, we should not dictate what parts of the native language syntax is allowed/not allowed.
Comment #22
donutdan4114 CreditAttribution: donutdan4114 commented"Sun is totally right, we should not dictate what parts of the native language syntax is allowed/not allowed."
Drupal standards already does specify the syntax you should use.
- Specifying exact whitespace to use for everything. PHP supports varying levels of whitespace.
- Using
print
notecho
- Using uppercase
FALSE/TRUE
vsfalse/true
- many more examples........
Comment #23
pwolanin CreditAttribution: pwolanin commentedIndeed. The number of characters typed is a totally bogus argument.
Being able to more easily port security and bug patches from 8 to 7 (to 6?) more easily is alone enough reason we should document the standard for core as array()
Comment #24
pwolanin CreditAttribution: pwolanin commentedupdating the title
Comment #25
sunerr, that title/direction change is a clear won't fix from my perspective.
You could as well say "Code must not use namespaced classes, because we need to backport security patches to D6." — Nonsense.
Except of the print/echo case (which only existed for consistency in PHPTemplate templates and is obsolete/bogus today), all of the examples in #22 are misinterpreted: They rule how you write code, if you write it. They do not rule what code you write.
With regard to the short array syntax, our existing array rules/recommendations apply as-is, since only the outer language construct is changed; the inner code is identical for both forms. Therefore, our existing coding standards don't need additional adjustments.
A. We can decide to "Be consistent within a file."
B. We can decide that we do not care at all.
Anything else is not on the table, really. I personally think that A would be a good idea, but B is fine, too.
Comment #26
sunComment #27
andypost+1 to consistent within function
PHP 5.3 era is over... and all newcomers would prefer to "type less", so this becomes argument.
Backport to D7 (d6) will require a lot of work anyway (d8 files are different, I could say slightly different)
Suppose D7 sites will start to move to PHP > 5.3 or D8 soon.
PS: I see no reason to derail/stop patches on the array syntax because the issue bikeshed could lasts so long
Comment #28
dawehnerWe tend to overthink problems. Most people are actually more familiar using [] for arrays anyway (JSON, python etc.)
so if we mix up for now, it might be a little be annoying, but nothing which stops you from being able to read code.
Also for new code, we should just allow people to use both and not block patches based upon it. In reality I could
imagine that many people will use [] because it is just a little bit less annoying to type.
I think some additional line we could add is:
Comment #29
jhodgdonPeople who have been programming with PHP for 10+ years know that the [] they tried when they first migrated to PHP from other languages doesn't work (prior to 5.4) and have long since broken themselves of the habit... so I wouldn't think that most of us are more familiar with [] than array() at this point!
Comment #30
sunFortunately, PHP does evolve, and so should we. People also had to adapt their habits to account for new constants, closures, function array dereferencing, namespaces, object references, shorthand ternary operators, etc.pp. They'll soon have to adapt for generators, finally keywords, etc. They'll have to rethink all of their habits when scalar objects and function return type hinting become available in PHP7.
Meanwhile, I'd recommend to go with #25.B: Don't care at all.
Enforcing a legacy PHP5.3 syntax in an application that requires PHP5.4 doesn't make any sense. Consistency within a file can't be ensured, because diff context is limited. Consistency within in a function… mayhaps, but pointless.
Thus, as @Crell phrased it:
+1
Comment #31
Crell CreditAttribution: Crell commentedFWIW, I've mentally switched over to []-style entirely for anything I write, and if I'm already touching a bit of code I often switch it to []. I am not blocking (or even commenting on, usually) any patches that use array(), however, since there's nothing actually wrong with it.
I recommend others adopt a similar "meh, we'll get there, don't hold us up" approach and it will all work out.
(And I've been doing PHP for almost my entire professional life. I fell back into [] like it was a warm bath.)
Comment #32
sunThanks for bringing up the point of reviews, @Crell. I agree we should simply ignore it altogether. I don't even care (enough) if a single line was using different syntaxes. Both work just fine and are identical. It's pointless to point out in reviews and hold up progress on that.
Comment #33
dawehner@sun
Totally, it is a nice to have, but nothing more than that. This is the reason I have chosen "can" in the addition above.
Comment #34
geerlingguy CreditAttribution: geerlingguy commentedI hate to add to the bikeshedding here, but mixing the short syntax and
array()
really throws me off; it feels a bit like reading shakespeare (not comparingarray()
to that, just giving an example) and seeing a page with some txt speech all the sudden... makes my brain start context switching, and completely distracts.Also looks ugly (really, really ugly) in API docs... for example:
https://api.drupal.org/api/drupal/core%21modules%21system%21system.modul...
Notice the 'link to any page' permission there...
Comment #35
sunThat's obviously a bug somewhere. The actual code is:
Comment #36
geerlingguy CreditAttribution: geerlingguy commentedRegardless of the display, seeing the mixed syntax still throws me for a loop. When I'm working with newer libraries, and everything is
[]
, it's no issue (and feels so much better, especially since PHP is finally terse like Python, JS, etc. in that regard).I think it *should* be emphasized that within files (or maybe within functions/methods at least), you *must* be consistent. So if you're going to add a new array with the new syntax, make sure you update the other arrays within the same function/method.
Comment #37
jhodgdonCoding standards changes are now part of the TWG
Comment #38
hussainwebAs it so happens, this was discussed in a few issues under #2394419: Clean-up file module test members - ensure property definition and use of camelCase naming convention. Specifically, there were some points raised in comments 5, 6 and 7.
Some points:
Plus, here are a few points from today's discussion with @alexpott regarding this on IRC:
In summary, I agree that such a change cannot occur even on a patch level at this stage in 8.0.x release. But should we target for a later release? I am not proposing a bulk change as that would break each and every patch but I think we could start with a change to favor the new syntax. Starting from 8.1.x (maybe), we encourage all new patches to use the new syntax, or maybe even have a meta issue to change this per module or component.
Comment #39
hass CreditAttribution: hass commentedMy expierience is that in many many cases it is worse to read code with the short array syntax. In a few cases - typical small arrays it is easier. Large arrays become difficult to read and finding the closing bracket becomes a challenge as one example.
I'm not in large favor to change everything to the new syntax.
Comment #40
bojanz CreditAttribution: bojanz commentedI'm switching all my contribs (Commerce, Inline Entity Form, VBO, etc) to use short array syntax exclusively, most PHP projects (and even core devs) have already switched to using it exclusively, so it feels consistent.
I'm not a fan of using both in core, often in the same method.
Comment #41
jhodgdonI think we all have agreed, and we have an implicit standard, that within one method you should stick to one or the other and not mix them. I also agree that for new Core 8.x code, we are using []. But when we define a standard saying "Only use []" a lot of our code immediately becomes out of compliance. Plus, of course, [] is only OK in 8.x.
Comment #42
Crell CreditAttribution: Crell at Palantir.net commentedDo we have a facility to do things like deprecate coding standards? :-) Most people seem on board with moving to [] incrementally, but with avoiding a big "invalidate ALL THE PATCHES!" push to do so. I think piece by piece is completely acceptable here. We just need to communicate that in a way that doesn't cause widespread panic and patch rejection and testbot fails.
Comment #43
jhodgdonRight now we don't even have a policy for how to adopt coding standards, much less to deprecate them. Maybe once we get through the Adoption policy we can make a policy about deprecating them, or a meta-policy about what happens with code that is "grandfathered".
Comment #44
jcnventura CreditAttribution: jcnventura as a volunteer commentedKnowing that the unwritten rule by Drupal devs is to 'do as core does', I think we should attempt to convert all the D8 code before the release candidate.
It will invalidate a lot of patches, but look at it it this way: there will be a myriad opportunities to get first-time core contributors just from fixing those :)
Comment #45
joelpittet#2168893-84: Views filters groups adding and removing is broken in #70 @joelpittet said
This is what I've been promoting in core, so we can move towards the goal without being overly disruptive.
Comment #46
dawehnerI follow that rule with the addition of not blocking core patches, in case they don't do it.
Comment #47
xjmThis is absolutely, totally not allowable during the beta. :) See https://www.drupal.org/core/beta-changes
If this policy is decided well before RC, it could potentially be something we do during RC. We've discussed possibly making some widespread, low-risk coding standards cleanups during that phase since most other patches won't be committed during those weeks anyway. (We haven't decided 100% that we're going to do so, but this would be a thing to keep in mind for it.)
Well, I'd also advise against this actually. See this draft on scoping meta issues. The patches are far more time-consuming to review than to create, and changing one module at a time still leaves core in the same inconsistent state. When it is exactly one logical change, especially when it can be done with a regex, we're better off doing one single patch that converts every usage in core.
Also, in general, see my comment in #2273925-241: Ensure #markup is XSS escaped in Renderer::doRender(). I'm concerned about the scope creep of adding these changes to unrelated patches, and the back-and-forth of people converting someone else's patch syntax from one to the other. @alexpott has raised similar concerns.
I guess I disagree with this for "changed" hunks:
Edit: I should point out that I think it's okay for new hunks, if they're lines you're writing yourself and not moved lines. Should have mentioned that. Additional edit: +1 for what @dawehner said as well in the case of new lines:
Overall, I think it confuses and misleads people to encourage them to make unrelated changes in a patch or to adopt a coding standard that is still debated. It also teaches what I'd consider bad patch habits. Experienced contributors often can negotiate the line between what will add to a reviewer's workload vs. not (or between what a reviewer will push back on and not), but newer contributors have more difficulty with that anyway (and shouldn't need to think about it). So we should teach them to do the thing that's best practice in terms of patch creation.
In general, every patch should make the minimum changes necessary to accomplish the scope of the issue, and not add other changes. Even if the line is already being changed, because it still affects word diffs and the merge history.
Comment #48
xjmAnd, since I guess I haven't weighed in yet, I personally prefer the short syntax. I find it more readable and I never understood why PHP used a function-looking-thing to make arrays anyway. ;) So I would be in favor of adopting the standard; I'd just like us to do so in a way that doesn't send mixed messages to contributors or muddy merges.
Comment #49
xjmOh, also see my comment in #1518116-81: [meta] Make Core pass Coder Review. If/when we do adopt the standard and make core follow it, ideally we'd do so with a coding standards rule being added to our code review tools and automated testing.
Comment #50
bojanz CreditAttribution: bojanz commentedWe need to at least communicate to contrib that array() is dead (I've converted Commerce and Address, Inline Entity Form and Profile2 will follow).
The current core way of converting line-by-line has resulted in both array() and [] being used within the same method (or even the same parent array), so some level of cleanup before the release will definitely be a good thing.
Comment #51
klausiFor Rules 8.x-3.x we use https://github.com/thomasbachem/php-short-array-syntax-converter and regularly execute this:
This will convert all array() statements to use [] instead. Have not seen false positives with this yet.
Comment #52
dawehner+1
Tried to summarize the current consensus as part of the issue summary.
Feel free to complain :)
The issue summary talks about
I would argue this is simply not any harder/easier, given how much D7 and D8 diverged.
Comment #53
jcnventura CreditAttribution: jcnventura as a volunteer commentedI'd be OK with doing it either in 8.1.x or during the release candidate phase. I'd rather opt for the second, as that would mean that patches would more easily apply to all the 8.x.y branches.
Since there are automated tools that can do this conversion, this can be done near the time of the stable release, so I'm changing the tag to reflect that.
Comment #54
dawehnerWell yeah noone has to do them manually BUT there is the problem that you might want to apply as many of the major patches after the release as possible.
Comment #55
drunken monkey@ bojanz:
So that has been decided already? If so, then it should really be communicated, yes – or at the very least added to the coding standards and changed in the issue summary.
From what I see, though, it hasn't even been "officially" decided yet.
Comment #56
pfrenssenThere are precedences in the coding standards of allowing two standards but preferring one over the other. This case of
array()
vs[]
is even already part of the documentation standards for return types. From Indicating data types in documentation:We could use similar wording, expressing a preference for the shorthand notation, but still allowing the old style.
Comment #57
jhodgdonRE #56, no that's something else actually. That's saying that you should do something like:
@return string[]
in preference to
@return array
in doc blocks. That has nothing to do with in PHP code whether you use $foo = array() vs. $foo = [].
But anyway, yes, we could do something similar in the PHP coding standards to say "We prefer [] in PHP code for creating an array".
Comment #58
tizzo CreditAttribution: tizzo commentedMoving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.
Comment #59
kenorb CreditAttribution: kenorb commentedMy two cents, the syntax
[ ]
(instead ofarray()
)is much cleaner, why not to use it.Comment #60
Perignon CreditAttribution: Perignon as a volunteer commentedI will throw my vote in for the short syntax. I know the Commerce Guys (Bojanz as lead developer) started doing it for all of their modules - I was at the sprint table in Barcelona where they started marching through all of their code. I have also done this on all the modules that I am maintaining.
Comment #61
xjmI am also in favor of adopting the short array syntax consistently for Drupal 8 only (since Drupal 7 does not meet the minimum version requirement). I agree with @dawehner that it is unlikely to make backports too much more difficult, since very little can be directly backported anyway. (And if you do luck out and get a simple backport, you'll get a syntax error right away on the 5.3 testing environment, so it's a very quick and easy fix).
Comment #62
xjmOh, I should add that I am not in favor of a recommendation that is not a requirement, because this will lead to frustrations in the core queue as arrays are noisily changed from one syntax to the other. (This has been a hassle ever since we adopted 5.4 and it compromises the reviewability of patches.) If we choose
[]
overarray()
, then let's make core compliant and add it as a coder rule. If we are not okay with adding it as a coder rule, it would be preferable not to make a recommendation, because core would consistently regress to the "non-recommended" syntax.Comment #63
pfrenssenI'm also in favor of adopting this for D8.
Comment #64
Perignon CreditAttribution: Perignon as a volunteer commentedI believe in the array() -> [] migration. New code should use []. Maybe reject code that isn't using []?
Even though D7 is php 5.3 compatible, I have started changing my D7 modules to short array syntax because it makes reading code so much easier (and setting the module to require PHP 5.4 minimum). Plus I have PHPStorm setup to do it for me. So the moment I touch code, it gets changed to short syntax.
Comment #65
alex.skrypnykThere should not be an issue of adopting short array syntax for Drupal 7, and here is why:
1. The support for PHP5.4 (the earliest version this syntax became available) has finished 3 Sep 2015 (7 month ago from the date of this comment). This means that no new installation should be done on versions equal or lower that 5.4 as they are no longer supported. We can argue that this requires a policy change around minimal installation requirements, but the reality is that we just need to do it.
2. PHPCS already has this sniff and PHPCBF (the tool that performs actual code fixing) can automatically fix the whole Drupal core codebase. This will require a separate release of Drupal, which should not be a big deal (I remember we had a separate release just because someone forgot to change the version number in core files).
3. Drupal 7 will be the most installed version for at least one more year, until module developers port their modules to 8. Not adopting short array syntax for version 7 is quite a big deal since more and more developers use it in their own modules. This leads to inconsistency in standards where core's outdated standards make contrib modules look like they are not compliant.
Overall, it has been 2 years since this issue started and we are still in discussion mode. Actual conversion of core to a short array syntax took me only 10 minutes. We should be able to make such changes much quicker than in 2 years, especially if we actually working with this code every day.
Comment #66
Perignon CreditAttribution: Perignon as a volunteer commented+1
Comment #67
NerOcrO CreditAttribution: NerOcrO at Axess Open Web Services commented+1 for the short array syntax on D8 only!
Comment #68
Perignon CreditAttribution: Perignon as a volunteer commentedI would include D7 as optional. Don't put it as D8 only. PHP is the new Internet Explorer with people not upgrading from 5.3.x.
Comment #69
tizzo CreditAttribution: tizzo commentedComment #70
droplet CreditAttribution: droplet commentedDo you have difficult to read both syntax in the same file? have you ever think about we developers also working in other projects outside the Drupal world?
Shouldn't code style defines:
Comment #71
alexpottI'm in favour of switching to short array syntax. As this will probably be the most disruptive coding standards change ever I propose we do this as the commit before we open 8.3.x so we only have to worry about the few patches that make it back to 8.1.x whilst 8.1.x, 8.2.x and 8.3.x are open - which is the beta/RC period in the lead up to 8.2.x being released.
Comment #72
klausiCan you make a proposal in the issue summary at which place in the coding standards we would replace which sentence?
We should have a concise proposal in place before we make a decision. I'm also interested how this would apply to Drupal 7, since coding standards are always current and apply to any version of Drupal.
Comment #73
dawehnerIf we really want to go the route of switching to short array everywhere for Drupal 7 I would suggest to not allow them in core patches, but not disallow in contrib. We cannot change the code of old versions due to a new CS change.
Comment #74
Chi CreditAttribution: Chi commentedDrupal 7 requires PHP 5.2 or higher but short array syntax is not supported in PHP 5.3 and lower.
Comment #75
joelpittetTo address #72 @klausi. It would be more of a D8 'recommendation' and Drupal 8 core requirement going forward on 8.3.x.
re #73 Drupal 7 should stay
array()
due to it's PHP requirements.Can we say/do that?
Comment #76
dbt102 CreditAttribution: dbt102 commentedJust want to make note here of #2730625-24: hook_help API example contains a deprecated call to Drupal::url() about discussion of patch to fix Drupal::url() . The initial single patch fix there combined the Drupal::url fix with the short array syntax. As we are planning the scope of that 'bigger picture" I wanted to rule in/out if we try to address the 'short array syntax' that we are likely to encounter.
I'm inclined to say 'No', just to keep it simple, but more input will make for a better decision :-)
Comment #77
Perignon CreditAttribution: Perignon as a volunteer commented+1 for #73 in regards to D7. It should not be disallowed in contrib because there are a lot of contrib modules already using it. And in a lot of these modules, they are not compatible with PHP 5.3 or 5.4 anyhow due to API libraries.
Comment #78
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedAgreed, we can say this is allowed for Drupal 7, but only in the case where the code already plans to require PHP 5.4 or higher.
As far as I know, PHP 5.3 is still supported by some Linux distributions (such as Ubuntu 12.04) for another year or so. Also, it's still quite popular (https://w3techs.com/technologies/details/pl-php/5/all puts its usage at 28.6% currently, and https://wordpress.org/about/stats/ puts it at 20.6% for WordPress sites specifically). We might consider increasing the minimum PHP requirements for Drupal 7 at some point, but those usage numbers would have to go way down and/or we'd have to have an extremely compelling reason to do it. Being able to type
[]
rather thanarray()
does not exactly qualify as compelling enough :)Comment #79
xjmI would really love it if we could adopt this. We'd find a way to adopt this definitively with a phpcs rule for D8 for
[]
specified in ourphpcs.xml.dist
, without changing D7 standards. D7 cannot require PHP 5.4 and therefore cannot require the short array syntax, but the vast majority of contributors prefer it for D8 and so use it in their patches. It adds so much noise in the issue queue. I don't even have the words.I would dearly love to convert the entirety of D8 core to the short array syntax during the 8.2.x RC.
And yes, it takes D8 patches a little further from D7 by having different standards for arrays, but that's like saying going up a flight of stairs takes your further from the center of the Earth. D8 and D7 are different frameworks with different standards.
Comment #80
xjmComment #81
joelpittetI was curious how long it would take, a few minutes with the script in the issue summary of this child issue #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase
Not sure what to do with
phpcs.xml.dist
, separate followup @xjm?Comment #82
dawehner@joelpittet
When we fix CS styles we add a
phpcs.xml.dist
entry always, so we can ensure that further patches don't break the CS rules again ...Note: This is also not about the fixing but rather determining the exact rules how the CS should apply.
Comment #83
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe coding standards committee is meeting this Wednesday, and given the discussion on this issue to date, are inclined to approve this.
I opened #2788295: How to handle and document deprecated/versioned coding standards?. Feedback there would be appreciated.
Comment #84
hussainwebThat's great. I hope to see this by 8.2.x finally. Is there a related coder rule we should be updating as well to catch this in new patches?
Comment #85
klausiCoder rule: yes, just enable the following in your phpcs.xml.dist:
Comment #86
tizzo CreditAttribution: tizzo commentedThe coding standards committee is just waiting on an update to the issue summary with a concrete proposal of exactly what to change in the coding standards.
Comment #87
dawehnerI just copied my comment
Comment #88
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks for #87, but:
I think we want language that is stronger than this. Since at least for core and other projects that enable #85, this will not be optional.
Comment #89
effulgentsia CreditAttribution: effulgentsia at Acquia commentedUpdated summary to use the IS template.
Comment #90
tizzo CreditAttribution: tizzo commentedUpdating language with a concrete proposal requiring short array syntax where safe and updating the primary documentation and examples for the new requirement.
Comment #91
drunken monkeyThanks, tizzo, looks great to me!
Just corrected "php" to "PHP" once and updated the "remaining tasks".
+1 from me.
Comment #92
tizzo CreditAttribution: tizzo commentedThis was approved during the December 20, 2016 meeting of the coding standards committee. Moving to the core queue for the "Core committer approved" tag so that core committers can decide when to incorporate this with the lowest impact on outstanding patches as this will touch almost every file PHP file in Drupal core.
The related coder issue (#2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase) already supports this and has a link to a converter to ease the process of updating so many instances of this change.
Comment #93
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTagging for documentation updates per step 6 of https://www.drupal.org/project/coding_standards, since that will need to happen once core complies.
Comment #94
xjmReally glad to see us moving forward on this! I've scheduled #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase for the 8.3.x RC phase, which begins Feb. 27. I also updated that issue to include @klausi's coder rule from #85.
Comment #95
xjmComment #96
xjmCore is patched for this now! Go forth and document. :)
Comment #97
Chi CreditAttribution: Chi commentedThis is a bit confusing me. What is the primary source of Drupal coding standards?
Comment #98
alexpott@Chi the primary source is drupal.org documentation - all standards have to be documented there. phpcs.xml.dist is a list of things the core code adheres to and is tested against. This is a subset of the primary source because (a) some things are hard to test (b) we've not yet got around to fixing core and turning on the coder rule. Coder is a contrib project that implements much of the primary coding standards rules but not all and also occasionally has opinions about things not covered in the standard.
Comment #99
Chi CreditAttribution: Chi commented@alexpott thanks, maybe we shouldn't have changed CS rulesets and applied new policies before updating the documentation.
Comment #100
joelpittet@Chi it's a bit of chicken and egg thing, but the changes were made to code to comply with the upcoming documentation changes. There are follow-ups to the documentation in the API and the standards that can be worked on and the changes were only committed to the -dev branches of 8.3.x and 8.4.x to show core is following the standards in the upcoming releases.
Comment #101
klausiUpdated the coding standards document with the proposal from the issue summary: https://www.drupal.org/docs/develop/standards/coding-standards#array
I think we are done here, thanks everyone!
Comment #102
pfrenssenAwesome, thanks!