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" />
Comment | File | Size | Author |
---|---|---|---|
#46 | 2790597-46-8.3.x-not-modules.patch | 3.34 MB | joelpittet |
#46 | 2790597-46-8.3.x-modules.patch | 8.78 MB | joelpittet |
#37 | 2776975-not_modules-37.patch | 3.34 MB | tim.plunkett |
#37 | 2776975-modules-37.patch | 8.77 MB | tim.plunkett |
Comments
Comment #2
joelpittetCurious to see how long it took to do this and if it would work
Comment #3
geerlingguy CreditAttribution: geerlingguy as a volunteer commentedDo you win the contest for largest patch ever? :)
Comment #4
joelpittetI did one last week that was bigger by accidentally diffing 8.0.x from habit;)
Comment #5
dawehnerThis 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.
Comment #7
dawehnerUpdated 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.Comment #8
dawehnerComment #9
klausiI 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.
Comment #10
klausiTriggering the testbot.
Comment #11
dawehnerWell, 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.
Comment #14
pfrenssen#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.
Comment #15
cilefen CreditAttribution: cilefen as a volunteer commentedComment #16
xjmThanks 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.
Comment #17
xjmComment #18
xjmComment #19
xjmComment #21
dawehnerFrom a coordination point of view it would be important IMHO to run this after the browsertestbase conversion.
Comment #22
xjmToday the core committers agreed to schedule this for March 3, 48 hours or so after RC1 is created.
Comment #23
dawehnerNice!
Comment #24
hussainwebAwesome! 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?Comment #25
xjmThe 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.
Comment #26
xjmAlrighty! It's time.
Comment #27
joelpittetNot 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:
engine
to the extensions because it looks like it's missing.<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.Comment #29
eojthebraveAre 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?
Comment #30
xjm@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? :)
Comment #31
xjmWe'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!
Comment #32
xjmAlso posted https://groups.drupal.org/node/516354.
Comment #33
xjmWe probably need two docs followups, for:
Comment #34
xjmOnce we have the updated patches we need uploaded here, we'll freeze commits until it tests and is committed.
Comment #35
xjmOh also, I would like explicit patches uploaded as in #27, so that they get tested and have peer review. Thanks!
Comment #36
tim.plunkettGenerating a new patch
Comment #37
tim.plunkettNice tricks @joelpittet, way faster.
Comment #40
xjmSo after applying the two patches above to 8.4.x, the following files still have some non-docs
array(
use in them:In some of these, the
array(
is a string literal; inContainerAwareEventDispatcherTest.php
:// @codingStandardsIgnoreFile
I wonder if we can force-override the ignore file directive while just using the one rule trick?
Comment #41
xjmAlso 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.
Comment #42
xjmTLDR, let's fix whatever the rule does here, and then follow up for subsequent additions to the standard. :)
Comment #43
joelpittetOne 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:
Likely mis-categorized them and could use some flushing out, please feel free to update them or start working on them.
Comment #44
xjm@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.Comment #45
xjmRe-saving issue credit again.
Comment #46
joelpittet@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 afterphpcbf
phpcs --standard=core/phpcs.xml.dist core
Just for explicitness this is the commands I ran to set the standard:
Comment #47
xjm8.4.x commit looks good. Proving the rule is running also:
:)
Comment #48
xjmy u no run, tests :(
Comment #49
xjmFWIW 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.
Comment #50
xjmThis is what confirms that no evil is done:
So @joelpittet is also not trying to sneak in anything sneaky. :D Now just waiting for tests to run.
Comment #51
xjmAll green! Committing shortly as I have one more check to do.
Comment #53
xjmVCS integration doesn't believe it yet, but the commits are there.
Enjoy!!
Comment #54
denutkarsh CreditAttribution: denutkarsh at Google Code-In commentedComment #55
LendudeAnd 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'?
Comment #56
joelpittet@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?
Comment #57
Lendude@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 generates fatal "Column not found: 1054 Unknown column" SQL error when using multi-column Fields, lets see how badly that fatals :)
Comment #58
LendudeQuick 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.
Comment #59
alexpottThis patch introduced a number of coding standards fails. Fixed in #2857822: Fix coding standards issues introduced mostly by array syntax conversion
Comment #60
klausiOpened #2857830: Enforce short array syntax for Drupal 8 code for Coder.
Comment #61
xjmFor 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.Comment #62
xjmAh, 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:
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.
Comment #63
xjmI'd rather just provide the DA with the patches. We tend to cause problems when we abuse the issue queue with scripts. :)
Comment #64
joelpittetThanks 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.
Comment #65
joelpittetHere's my WIP:
https://gist.github.com/joelpittet/dc7b71e1445650a187fa3f191713fa0c
Comment #66
droplet CreditAttribution: droplet commentedSlightly improve to #58,
(** Obviously cloned repo to 2 diff folders will save you a lot of time on `git checkout`)
Comment #67
LendudeCreated 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
Comment #68
LendudeAlso 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.
Comment #69
droplet CreditAttribution: droplet commented#68,
that case reroll with
git diff -W
will work. @see #66 :pComment #70
joelpittetI've updated the script I was writing in the gist.
https://gist.github.com/joelpittet/dc7b71e1445650a187fa3f191713fa0c
It does the following so far:
phpcbf
against the files that the patch changes.Now I just need to avoid the conflict that I've created in the diff...
Comment #71
Lendude@droplet thanks for the extra steps!
Comment #72
joelpittetUpdated 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:
array(
in itI 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
Comment #73
Gábor HojtsyIt 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.