Follow-up to #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8

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

A script to automate this should be used. The converter works well from
https://github.com/thomasbachem/php-short-array-syntax-converter

find -E core/ -regex '.*\.(php|engine|inc|install|make|module|profile)' -exec php "convert.php" -w "{}" \;

Note: takes a while to run.

In addition to the core conversions, we will need to enable the rule in the core coding standards:

 <!-- We always want short array syntax only. -->
  <rule ref="Generic.Arrays.DisallowLongArraySyntax" />
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Status: Active » Needs review
FileSize
13.96 MB

Curious to see how long it took to do this and if it would work

geerlingguy’s picture

Do you win the contest for largest patch ever? :)

joelpittet’s picture

I did one last week that was bigger by accidentally diffing 8.0.x from habit;)

dawehner’s picture

Status: Needs review » Needs work

This has to add a phpcs additional. I cannot find that. In order to prevent regressions, we really need that entry.

I skimmed until \Drupal\Core started but then realized, PHPCS would just do the same for us.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Updated the patch against 8.2.x and 8.3.x together with the phpcs file entry.

I am using phpcbf --standard=~/www/d8/core/phpcs.xml.dist core for it.

dawehner’s picture

klausi’s picture

I think it does not make sense to post conversion patches here. Just add the entry to phpcs.xml.dist in the patch and let a core committer run phpcbf locally.

klausi’s picture

Status: Needs work » Needs review

Triggering the testbot.

dawehner’s picture

Well, sure, I just wanted to prove that using the phpcs file is the way to go.

Sadly this is basically blocked on #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 going through the process.

The last submitted patch, 7: 2776975-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2776975-83.patch, failed testing.

pfrenssen’s picture

#2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 is now one step closer to completion, it is pending approval from a core committer now. This means we can start working on this.

The thing is though that the impact of this is huge since it will cause most patches to be rerolled. A good moment might be right after 8.3.0-rc1 is released.

cilefen’s picture

Title: Convert core to array syntax coding standards for Drupal 8.2.x » Convert core to array syntax coding standards for Drupal 8.3.x
xjm’s picture

Title: Convert core to array syntax coding standards for Drupal 8.3.x » Feb. 27, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RCphase
Issue summary: View changes
Status: Needs work » Postponed

Thanks everyone! Let's schedule this change for 8.3.0-RC1, the start of the RC phase.

The patch or a followup should also include enabling the coder rule. I've added it to the summary here.

xjm’s picture

Title: Feb. 27, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RCphase » Feb. 27, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase
xjm’s picture

Issue tags: +8.3.0 release notes
xjm’s picture

Issue tags: +rc target

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

From a coordination point of view it would be important IMHO to run this after the browsertestbase conversion.

xjm’s picture

Title: Feb. 27, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase » March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase

Today the core committers agreed to schedule this for March 3, 48 hours or so after RC1 is created.

dawehner’s picture

Nice!

hussainweb’s picture

Awesome! No more array( all over the code.

I can't seem to find the link to documentation for this change for contrib. Is it now part of Drupal coding standard or is the legacy array() syntax acceptable?

xjm’s picture

The Technical Working Group formally adopted the standard, so it should also apply to contrib for 8.x. #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 will take care of the needed documentation updates once core is compliant.

xjm’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Postponed » Needs work

Alrighty! It's time.

joelpittet’s picture

Not sure if this is helpful or not, but I just wanted to try @dawehner's approach.

I changed two things in the phpcs.xml.dist before running:

  1. Added engine to the extensions because it looks like it's missing.
  2. Removed all other rules besides the added <rule ref="Generic.Arrays.DisallowLongArraySyntax" /> rule so that I hopefully didn't exceed the scope of this issue and to make it maybe faster (took about 5 min)

Apparently the patch is too large to upload to d.o so I split it between two, those with core/modules and those outside.

The last submitted patch, 27: convert_core_to_array-no-modules-2776975-27.patch, failed testing.

eojthebrave’s picture

Are we going to update all the code in docblock @code comments? There's quite a bit that uses array() syntax. Example https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

I realize that it might be out of scope for this issue specifically, but wanted to make sure that it was pointed out. I think my argument for having them all be updated is that these code examples should be suitable for people to copy/paste and make use of. This makes them more useful as a learning tool. And, our example code should probably follow current best practices.

Is this something that can be automated with a tool like phpcbf?

xjm’s picture

@eojthebrave, yep, I definitely we should have a followup for documentation usages and see if it's achievable during this RC phase as well. Edit: want to file it? :)

xjm’s picture

Status: Needs review » Needs work

We're going to need both 8.4.x and 8.3.x patches at the same time I think; this won't apply cleanly or be a complete fix across branches. Thanks @joelpittet for getting this moving again!

xjm’s picture

xjm’s picture

Issue tags: +Needs followup

We probably need two docs followups, for:

  1. API docs.
  2. The handbook.
xjm’s picture

Once we have the updated patches we need uploaded here, we'll freeze commits until it tests and is committed.

xjm’s picture

Oh also, I would like explicit patches uploaded as in #27, so that they get tested and have peer review. Thanks!

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Generating a new patch

tim.plunkett’s picture

Nice tricks @joelpittet, way faster.

The last submitted patch, 37: 2776975-modules-37.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 37: 2776975-not_modules-37.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

So after applying the two patches above to 8.4.x, the following files still have some non-docs array( use in them:

core/lib/Drupal/Component/Utility/Variable.php
core/lib/Drupal/Core/Archiver/ArchiveTar.php
core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
core/lib/Drupal/Core/Template/TwigNodeTrans.php
core/modules/migrate_drupal/tests/fixtures/drupal6.php
core/modules/migrate_drupal/tests/fixtures/drupal7.php
core/modules/system/src/Tests/Form/FormTest.php
core/scripts/generate-d6-content.sh
core/scripts/generate-d7-content.sh
core/scripts/run-tests.sh
core/scripts/transliteration_data.php.txt
core/scripts/update-countries.sh
core/tests/Drupal/KernelTests/Core/Plugin/Discovery/DiscoveryTestBase.php
core/tests/Drupal/KernelTests/Core/Site/SettingsRewriteTest.php
core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php
core/tests/Drupal/Tests/Component/Utility/VariableTest.php
core/tests/Drupal/Tests/ComposerIntegrationTest.php
sites/default/default.settings.php

In some of these, the array( is a string literal; in ContainerAwareEventDispatcherTest.php:
// @codingStandardsIgnoreFile

I wonder if we can force-override the ignore file directive while just using the one rule trick?

xjm’s picture

Also we probably need to add .sh to the extension since we use that extension on PHP files.... or fix those in a followup is fine too, since anything that coder doesn't check, coder doesn't check.

I'm wrong about overriding the ignore; we shouldn't do that in this issue at least. Some of those are ignored for other reasons, but some like Archiver are faux-vendor.

xjm’s picture

TLDR, let's fix whatever the rule does here, and then follow up for subsequent additions to the standard. :)

joelpittet’s picture

Issue tags: -Needs followup

One thing my patch in #27 is missing that @tim.plunkett correctly included in his patch and I forgot to mention was the <rule ref="Generic.Arrays.DisallowLongArraySyntax" />

Not sure if drupalci is picking up that... I'll post a new set of patches for 8.3.x with that addition as well in a few minutes.

Created 2 follow-ups:

  1. #2857771: Convert core to array syntax coding standards - API docs
  2. #2857772: Convert core to array syntax coding standards - The D8 handbook

Likely mis-categorized them and could use some flushing out, please feel free to update them or start working on them.

xjm’s picture

@joelpittet, @tim.plunkett also just said he'll roll the 8.3.x patches, so. :) I will check for the rule.

We need one more followup, postponed on the API docs one, to look at remaining /\barray(/ and determine if they should be fixed or not.

xjm’s picture

Re-saving issue credit again.

joelpittet’s picture

@tim.plunkett let me know i'm ok to do the 8.3.x patch, there was one more addition that slipped in since last night anyways on datetime. EDIT: interdiff is from #27
I ran phpcs as well to double check after phpcbf
phpcs --standard=core/phpcs.xml.dist core

Just for explicitness this is the commands I ran to set the standard:

# Ensure Code sniffer was up-to-date
composer global update
# Ensure my config is set
phpcs --config-set installed_paths ~/.composer/vendor/drupal/coder/coder_sniffer
# Fix all the things.
phpcbf --standard=core/phpcs.xml.dist core
xjm’s picture

8.4.x commit looks good. Proving the rule is running also:

[ibnsina:maintainer | Fri 19:20:59] $ git diff
diff --git a/core/modules/node/node.module b/core/modules/node/node.module
index 40e5b4e293..1928547562 100644
--- a/core/modules/node/node.module
+++ b/core/modules/node/node.module
@@ -1376,6 +1376,7 @@ function node_comment_update($comment) {
 function node_comment_delete($comment) {
   // Reindex the node when comments are deleted.
   if ($comment->getCommentedEntityTypeId() == 'node') {
+    $array_of_evil = array();
     node_reindex_node_search($comment->getCommentedEntityId());
   }
 }

[ibnsina:maintainer | Fri 19:21:10] $ git commit -am "EVIL ARRAY"

FILE: /Users/xjm/git/maintainer/core/modules/node/node.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1379 | ERROR | [x] Short array syntax must be used to define arrays
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 319ms; Memory: 12Mb

:)

xjm’s picture

y u no run, tests :(

xjm’s picture

FWIW I also just used a porcelain diff to confirm @tim.plunkett's patch contains nothing other than array fixes. ;) Will do the same on the 8.3.x shortly.

xjm’s picture

This is what confirms that no evil is done:


$file = file('./porcelain.txt');

foreach ($file as $key => $line) {
  if ((substr($line, 0, 5) == '--- a') || (substr($line, 0, 5) == '+++ b')) {
    continue;
  }
  if ($line[0] == '+') {
    // Added lines should consist only of some number of square brackets.       
    // Not bothering with making sure anything matches because php -l will      
    // catch it if they don't.                                                  
    if (!preg_match('/^\+(\s)*[\[\]]+$/', $line)) {
      print $line;
    }
  }
  elseif ($line[0] == '-') {
    // Removed lines should consist only of 'array(' followed by some parens.   
    if (!preg_match('/^-(\s)*(array(\s)*\()+[\(\)]*$/', $line) && !preg_match('/^-(\s)*[\)]*$/', $line)) {
      print $line;
    }
  }

}

git diff --word-diff=porcelain --staged > ~/porcelain.txt
[ibnsina:~ | Fri 20:26:10] $ php porcelain.php
-Array(
-Array(
-array((array
-array( )
+[ ]
-      array(array(), array()),
+      [[], []],
+<rule ref="Generic.Arrays.DisallowLongArraySyntax" />

So @joelpittet is also not trying to sneak in anything sneaky. :D Now just waiting for tests to run.

xjm’s picture

Assigned: Unassigned » xjm
Status: Needs review » Reviewed & tested by the community

All green! Committing shortly as I have one more check to do.

xjm’s picture

Status: Patch (to be ported) » Fixed

VCS integration doesn't believe it yet, but the commits are there.

Enjoy!!

denutkarsh’s picture

Assigned: xjm » Unassigned
Lendude’s picture

And now the only thing to do is reroll every single patch in the queue.....

Any way to let people know this was done and mark everything that 'needs review' now probably 'needs reroll'?

joelpittet’s picture

@lendude we can maybe automate that, it's scriptable if the patch applied before the this one.

@alexpott has a tool to help and we could use that and I saw another one that helps create patches recently.

Just don't want all the patches owned by me so maybe I can convince a bot user to do it?

Lendude’s picture

@joelpittet using the tool you pointed to, adding a <?php tag to the top of your patch file and running convert seems the work

tried it on #2815881: Switching on aggregation on generates fatal " Column not found: 1054 Unknown column" SQL error when using multi-column Fields, lets see how badly that fatals :)

Lendude’s picture

Quick blog post on what I'm doing at the moment:
https://www.dx-experts.nl/converting-patches-use-correct-array-notation

Interested to see if we run into any problems using that setup.

alexpott’s picture

This patch introduced a number of coding standards fails. Fixed in #2857822: Fix coding standards issues introduced mostly by array syntax conversion

klausi’s picture

xjm’s picture

For some previous disruptive changes (maybe the patches to clean up t() on test assertions), I had a script that downloaded every NR patch in the queue to provide a reroll. Might still have that kicking around somewhere. I'll check.

xjm’s picture

Ah, my script was written using the old D6 d.o web services, so won't work even if I do find it. However, it's simple in concept:

  1. Use REST data to curl down the latest patches from the RTBC queue first and NR queue second, 8.3.x first and 8.4.x second. Save them in a directory, each named with the issue ID.
  2. Run the array-fixing script on all patches in the directory, saving the fixed patches to a new directory with a suffix.
  3. Loop over all the patches in the new directory and check that they apply to HEAD by trying a git apply. If the apply passes, move the patch to a directory of safe patches; if it fails; move it to a separate directory of patches that need other reroll.

The one problem part is bulk-uploading the working ones; my script didn't handle that because it was just to prove that my changes weren't breaking most things (to address objections) and providing rerolls for the few things they did. Maybe the DA could help with that step.

xjm’s picture

Just don't want all the patches owned by me so maybe I can convince a bot user to do it?

I'd rather just provide the DA with the patches. We tend to cause problems when we abuse the issue queue with scripts. :)

joelpittet’s picture

Thanks for the suggestion @xjm, if I get something going I'll see how I can get patches to the DA, I'm experimenting a bit here this afternoon, anybody is welcome to contribute snippets to this cause.

@xjm if there is part of your scripts that could help this cause please do post them here or on a gist/pastebin/etc.

joelpittet’s picture

droplet’s picture

Slightly improve to #58,

# Checkout the commit right before Array changes
git checkout 3aba956a39a4852f42cad

git apply --index old.patch

######################################
############# It does the trick!!! #############
######################################
git diff -W --cached > new.patch

# Add <?php to the first line of the patch

php convert.php -w new.patch

# Optional
git checkout 8.4.x
git apply --index new.patch
git diff --cached > new-small.patch

(** Obviously cloned repo to 2 diff folders will save you a lot of time on `git checkout`)

Lendude’s picture

Created a fork of convert https://github.com/Lendude/php-short-array-syntax-converter that adds a -p switch that handles most of the manual steps needed to run convert on a patch file

Lendude’s picture

Also ran into the first situation where using convert on the patch file won't work: if only the trailing ) of the array() is in the patch file (see: #2820347: Exposed filter reset redirects user to 404 page on AJAX view when placed as a block). Funky stuff will probably also happen if only the leading array( is in the original patch.

droplet’s picture

#68,
that case reroll with git diff -W will work. @see #66 :p

joelpittet’s picture

I've updated the script I was writing in the gist.

https://gist.github.com/joelpittet/dc7b71e1445650a187fa3f191713fa0c

It does the following so far:

  1. Downloads all the latest patches from 8.3.x/8.4.x RTBC/NR in the issue queue.
  2. Applies the patches against the commit before the array syntax to ensure they applied before.
  3. Runs phpcbf against the files that the patch changes.

Now I just need to avoid the conflict that I've created in the diff...

Lendude’s picture

@droplet thanks for the extra steps!

joelpittet’s picture

Updated one more time: https://gist.github.com/joelpittet/dc7b71e1445650a187fa3f191713fa0c

I opted to compare the files changed against the short array syntax commit to generate the diff then checked that.

It does the following so far:

  1. Downloads all the latest patches from 8.3.x/8.4.x RTBC/NR in the issue queue: 1792 patches downloaded.
  2. Applies the patches against the commit before the array syntax to ensure they applied before.
  3. Double check that the patch has a array( in it
  4. Runs phpcbf against the files that the patch changes.
  5. Creates a new patch based on the diff between those files changed and the short array syntax commit.
  6. Check the new patch applies against HEAD and store the new patch in a good or bad folder

I think this covers all the things, @xjm who can I fire this off to at the DA? It seems to create about 300-400 "good" patches. Probably could use a few more eyes to make sure I didn't slip. Tried to comment it out well enough.

EDIT: 605 are good, 4 are bad out of 609 applied and have long array

Gábor Hojtsy’s picture

Issue tags: +Needs change record

It would be *really* useful if coding standards changes like this would get a core change record because then contrib/core developers would only need to review https://www.drupal.org/list-changes/drupal/published?to_branch=8.3.x and get a complete list of changes including this one.

Not 100% sure where it should be attached to, but this is the core issue executing it, so putting the tag here.

Status: Fixed » Closed (fixed)

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