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
CommentFileSizeAuthor
#1 2135291-1.patch12.72 MBdrifter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drifter’s picture

FileSize
12.72 MB
drifter’s picture

I'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.

drifter’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2135291-1.patch, failed testing.

klausi’s picture

Issue summary: View changes
Issue tags: +Coding standards

Not sure we should do this.

Crell’s picture

This 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.

donutdan4114’s picture

I 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.

superspring’s picture

Issue tags: +Novice
rob3000’s picture

I'm happy to give this one a go :)

Crell’s picture

Title: Switch to PHP 5.4 short array syntax » [Policy, no patch] Favor PHP 5.4 short array syntax
Issue tags: -Novice

We'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:

When declaring arrays, PHP 5.4's short-array syntax is preferred.  Use of the extended array syntax (array()) is acceptable.  Individual functions and classes MUST be internally consistent as to which they use, however.

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.)

sun’s picture

It'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.

Code MAY use the PHP 5.4+ short-hand array syntax, but SHOULD be consistent within a single file.

...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.

grom358’s picture

With 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.

damiankloip’s picture

I 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.

grom358’s picture

array_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.

webchick’s picture

I 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).

Crell’s picture

I still stand by "use it, don't use it, whatever, both work."

sun’s picture

Status: Needs work » Active

What @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).

pwolanin’s picture

For 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.

ParisLiakos’s picture

+1 to what Crell said in #16

We can decide to "Be consistent within a file.", or we can decide that we do not care at all.

Lets not care at all

donutdan4114’s picture

"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.

damiankloip’s picture

We 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.

donutdan4114’s picture

"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 not echo
- Using uppercase FALSE/TRUE vs false/true
- many more examples........

pwolanin’s picture

Indeed. 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()

pwolanin’s picture

Title: [Policy, no patch] Favor PHP 5.4 short array syntax » [Policy, no patch] Make array() a clear core coding standard instead of [ ] for consistency across versions

updating the title

sun’s picture

err, 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.

sun’s picture

Title: [Policy, no patch] Make array() a clear core coding standard instead of [ ] for consistency across versions » [Policy, no patch] PHP 5.4 short array syntax coding standards
andypost’s picture

+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

dawehner’s picture

I still stand by "use it, don't use it, whatever, both work."

We 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:

In Drupal 8 code, or code depending on 5.4 you can use the short array syntax. The above example would look like the following:

$some_array = ['hello', 'world', 'foo' => 'bar'];
jhodgdon’s picture

People 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!

sun’s picture

Fortunately, 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:

Use it, don't use it, whatever, both work.

+1

Crell’s picture

FWIW, 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.)

sun’s picture

Thanks 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.

dawehner’s picture

@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.

geerlingguy’s picture

I 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 comparing array() 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...

    'access site in maintenance mode' => array(
      'title' => t('Use the site in maintenance mode'),
    ),
    'access site reports' => array(
      'title' => t('View site reports'),
      'restrict access' => TRUE,
    ),
    'link to any page' => ['title' t('Link to any page') 'description' t('This allows to bypass access checking when linking to internal paths.') 'restrict access' TRUE],
  );

Notice the 'link to any page' permission there...

sun’s picture

That's obviously a bug somewhere. The actual code is:

    'link to any page' => [
      'title' => t('Link to any page'),
      'description' => t('This allows to bypass access checking when linking to internal paths.'),
      'restrict access' => TRUE,
    ],
geerlingguy’s picture

Regardless 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.

jhodgdon’s picture

Project: Drupal core » Drupal Technical Working Group
Version: 8.0.x-dev »
Component: other » Code

Coding standards changes are now part of the TWG

hussainweb’s picture

As 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:

The reason I am doing this is it makes it a bit more readable. I know it is not an earth shattering improvement but every bit helps, IMHO. Arrays in function calls, and nested arrays become much more readable because of a different bracket style.

Lastly, this is just whimsical but I just found that if we replace every array definition, we reduce the source code size by about 350 KB. I know the benefits (1% size decrease) are not that much compared to the effort (~58K usages) and I am not going to even suggest that we do this. :) But I think we can do this with every change that we make.

... There are lots of places in core that use the new array syntax and it seems to me that we almost always use this syntax for new blocks of code (few of them change existing lines).

Plus, here are a few points from today's discussion with @alexpott regarding this on IRC:

<alexpott> hussainweb: it is not worth the disruption at this point
<hussainweb> alexpott, I had reasoned it out in https://www.drupal.org/node/2396717#comment-9453055 too.
<Druplicon> https://www.drupal.org/node/2396717 => Clean-up taxonomy module test members - ensure property definition and use of camelCase naming convention #2396717: Clean-up taxonomy module test members - ensure property definition and use of camelCase naming convention => 5 comments, 1 IRC mention
<hussainweb> alexpott, I did not think it was disruptive. If you feel that way, should it be for 8.1.x?
<alexpott> hussainweb: by making out-of-scope changes you are making the review think more than they need to
<alexpott> hussainweb: array() vs [] doesn't need to change in the 8.x cycle
<hussainweb> alexpott, Besides, we are using both variants throughout the core. This helps in standardizing it.
<alexpott> hussainweb: well... we are a mishmash
<hussainweb> alexpott, yes, but since this is a minimal impact, we could as well get on with it.
<alexpott> hussainweb: and we have no policy to prefer one or the other

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.

hass’s picture

My 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.

bojanz’s picture

I'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.

jhodgdon’s picture

I 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.

Crell’s picture

Do 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.

jhodgdon’s picture

Right 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".

jcnventura’s picture

Issue tags: +revisit before release candidate

Knowing 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 :)

joelpittet’s picture

#2168893-84: Views filters groups adding and removing is broken in #70 @joelpittet said

Any changed/new diff hunks can get the short array syntax(don't go out of the hunks or it could cause needless rerolls on other patches and make the committers and review[er]s cross:P)

This is what I've been promoting in core, so we can move towards the goal without being overly disruptive.

dawehner’s picture

This is what I've been promoting in core, so we can move towards the goal without being overly disruptive.

I follow that rule with the addition of not blocking core patches, in case they don't do it.

xjm’s picture

Knowing 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.

This 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.)

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 :)

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:

Any changed/new diff hunks can get the short array syntax(don't go out of the hunks or it could cause needless rerolls on other patches and make the committers and review[er]s cross:P)

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:

I follow that rule with the addition of not blocking core patches, in case they don't do it.

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.

xjm’s picture

And, 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.

xjm’s picture

Related issues: +#1518116: [meta] Make Core pass Coder Review

Oh, 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.

bojanz’s picture

We 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.

klausi’s picture

For Rules 8.x-3.x we use https://github.com/thomasbachem/php-short-array-syntax-converter and regularly execute this:

find /path/to/drupal-8/modules/rules -name "*.php" -exec php "/home/klausi/workspace/php-short-array-syntax-converter/convert.php" -w "{}" \;

This will convert all array() statements to use [] instead. Have not seen false positives with this yet.

dawehner’s picture

Issue summary: View changes

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.

+1

Tried to summarize the current consensus as part of the issue summary.
Feel free to complain :)

The issue summary talks about

  • Patches are harder to backport to Drupal 7, which puts more work on the security team when working on multiple security patches.
  • I would argue this is simply not any harder/easier, given how much D7 and D8 diverged.

    jcnventura’s picture

    Issue summary: View changes
    Issue tags: -revisit before release candidate +revisit before stable release

    I'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.

    dawehner’s picture

    Well 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.

    drunken monkey’s picture

    @ bojanz:

    We need to at least communicate to contrib that array() is dead

    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.

    pfrenssen’s picture

    There 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:

    For the PHP built-in types, use the following names:

    • array (NOT "Array"). However, the [] syntax (see above) is preferred if appropriate.
    • ...

    We could use similar wording, expressing a preference for the shorthand notation, but still allowing the old style.

    jhodgdon’s picture

    RE #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".

    tizzo’s picture

    Project: Drupal Technical Working Group » Coding Standards
    Component: Code » Coding Standards
    kenorb’s picture

    My two cents, the syntax [ ] (instead of array())is much cleaner, why not to use it.

    Perignon’s picture

    I 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.

    xjm’s picture

    Issue tags: -revisit before stable release

    I 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).

    xjm’s picture

    Oh, 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 [] over array(), 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.

    pfrenssen’s picture

    I'm also in favor of adopting this for D8.

    Perignon’s picture

    I 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.

    alex.skrypnyk’s picture

    There 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.

    Perignon’s picture

    +1

    NerOcrO’s picture

    +1 for the short array syntax on D8 only!

    Perignon’s picture

    I 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.

    tizzo’s picture

    Issue tags: +final discussion
    droplet’s picture

    ["apples", "oranges", "bananas"]
    array("apples", "oranges", "bananas")
    

    Do 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:

    //bad
    ["apples", "oranges", "bananas"]
    
    //good
    ["apples",
     "oranges",
     "bananas"]
    
    //better
    [
     "apples",
     "oranges",
     "bananas",
    ]
    
    alexpott’s picture

    I'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.

    klausi’s picture

    Status: Active » Needs work

    Can 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.

    dawehner’s picture

    If 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.

    Chi’s picture

    Drupal 7 requires PHP 5.2 or higher but short array syntax is not supported in PHP 5.3 and lower.

    joelpittet’s picture

    To 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?

    dbt102’s picture

    Just 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 :-)

    Perignon’s picture

    +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.

    David_Rothstein’s picture

    Agreed, 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.

    There 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.

    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 than array() does not exactly qualify as compelling enough :)

    xjm’s picture

    I 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 our phpcs.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.

    xjm’s picture

    Title: [Policy, no patch] PHP 5.4 short array syntax coding standards » [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8
    joelpittet’s picture

    I 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?

    dawehner’s picture

    @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.

    effulgentsia’s picture

    I would dearly love to convert the entirety of D8 core to the short array syntax during the 8.2.x RC.

    The coding standards committee is meeting this Wednesday, and given the discussion on this issue to date, are inclined to approve this.

    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

    I opened #2788295: How to handle and document deprecated/versioned coding standards?. Feedback there would be appreciated.

    hussainweb’s picture

    That'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?

    klausi’s picture

    Coder rule: yes, just enable the following in your phpcs.xml.dist:

      <!-- We always want short array syntax only. -->
      <rule ref="Generic.Arrays.DisallowLongArraySyntax" />
    
    tizzo’s picture

    The 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.

    dawehner’s picture

    Issue summary: View changes

    I just copied my comment

    effulgentsia’s picture

    Thanks for #87, but:

    In Drupal 8 code, or code depending on 5.4 you can use the short array syntax.

    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.

    effulgentsia’s picture

    Issue summary: View changes

    Updated summary to use the IS template.

    tizzo’s picture

    Issue summary: View changes
    Status: Needs work » Needs review
    Issue tags: -Needs issue summary update

    Updating language with a concrete proposal requiring short array syntax where safe and updating the primary documentation and examples for the new requirement.

    drunken monkey’s picture

    Issue summary: View changes

    Thanks, tizzo, looks great to me!
    Just corrected "php" to "PHP" once and updated the "remaining tasks".
    +1 from me.

    tizzo’s picture

    Project: Coding Standards » Drupal core
    Version: » 8.3.x-dev
    Component: Coding Standards » other
    Status: Needs review » Reviewed & tested by the community
    Issue tags: -final discussion +Approved Coding Standard

    This 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.

    effulgentsia’s picture

    Issue tags: +needs documentation updates

    Tagging for documentation updates per step 6 of https://www.drupal.org/project/coding_standards, since that will need to happen once core complies.

    xjm’s picture

    Project: Drupal core » Coding Standards
    Version: 8.3.x-dev »
    Component: other » Coding Standards
    Issue tags: +Core Committer Approved

    Really 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.

    xjm’s picture

    Issue tags: +8.3.0 release notes
    xjm’s picture

    Core is patched for this now! Go forth and document. :)

    Chi’s picture

    This is a bit confusing me. What is the primary source of Drupal coding standards?

    • phphcs.xml in in Drupal core
    • rulset.xml in Coder module
    • drupal.org documentation
    alexpott’s picture

    @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.

    Chi’s picture

    @alexpott thanks, maybe we shouldn't have changed CS rulesets and applied new policies before updating the documentation.

    joelpittet’s picture

    @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.

    klausi’s picture

    Status: Reviewed & tested by the community » Fixed

    Updated 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!

    pfrenssen’s picture

    Issue tags: -needs documentation updates

    Awesome, thanks!

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.