Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase array syntax in core was converted to be consistent. It seems like at least default.settings.php was still left into an inconsistent state.
Proposed resolution
Convert all usages of the old array syntax to new. This issue only deals with files outside the core/
directory (per #17).
Remaining tasks
- Convert all remaining usages of the old array syntax
- Ensure that there are no old array syntax usages left
Find remaining instances of old array syntax.
git grep -l " array(" -- './*' ':!^core/'
User interface changes
-
API changes
-
Data model changes
-
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff_30_41.txt | 3.61 KB | mohit1604 |
#41 | array-syntax-2868078-41.patch | 2.22 KB | ankitjain28may |
Comments
Comment #2
alexpottdefault.settings.php is not covered by phpcs automated testing - this is how it slips through the net. Same with run-tests.sh. Perhaps this issue could work out how to scan those files too.
Comment #3
claudiu.cristeaLet's see
Comment #4
claudiu.cristeaProbably this tells us to move phpcs.xml.dist and phpcs.xml files one level up because now they are covering not only core/
Comment #5
alexpott@claudiu.cristea I don't think we should be move phpcs.xml.dist up a level - those are core's standards we don't want to people to have to merge them or do whatever thay have to do to root files when they change.
This change isn't right. Should maybe be in a coding standards ignore section. Or perhaps we just shouldn't bother with automated tests on this file. It's really nice to bring run-tests.sh in though.
Comment #6
claudiu.cristeaI agree. I ran phpcbf to see all changes. But I think also that this should be fixed in coder. I reopened #2635594: Line indentation is checked incorrectly for commented code for this reason.
Comment #7
claudiu.cristea@alexpott what about this?
It seems that coder doesn't check the hash comments. This would also align with the other commented code chunks in the file.
Comment #8
pfrenssenI wouldn't prefix it with hashes just for the sake of making the coding standards check pass. This makes it harder to uncomment this section.This is not something that should be fixed in coder since committing commented out code is bad practice. This is like the only valid case we will ever have for this in core. This warrants an exception in my opinion.
I would surround this section with@codingStandardsIgnoreStart
and@codingStandardsIgnoreEnd
.Edit: I just discussed this with @claudiu.cristea on chat, actually the use of hashes to comment out sections of code is used everywhere in settings.php, so any system administrator working on a copy of the file is already uncommenting by removing hashes.
I changed my mind, this is a good solution after all.
Comment #9
pfrenssenComment #10
pfrenssenEdit: wrong issue, sorry.
Comment #11
lauriiiThere are more PHP files under the core/scripts that are not cleaned up yet
Comment #12
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedI was just preparing a patch updating
default.settings.php
comments to short array syntax and found this issue. Adding my changes to previous patch, hope this is the right place to do this =)Comment #13
joelpittetDid you script this? If so could you share that in the IS?
Comment #14
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedI didn't @joelpittet, did it by hand...
Comment #15
joelpittetOk here's maybe a helper script to find others that are outside tests. Feel free to iterate on this:
grep -r --exclude-dir='[Tt]ests' --exclude='*Test.php' '[^_]array(' core
Comment #16
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedOK, I'll bite... this is quite painful but writing a script to handle this would be tricky for the closing parenthesis... I guess Rome was not built in one day right? :D
Cleaned up:
PLENTY left to do, just run the command
grep -r --exclude-dir='[Tt]ests' --exclude='*Test.php' '[^_]array(' core > arrays.txt
, open uparrays.txt
and start working on the files listed.Note that some are legitimate, so mindless search & replace wont work - for example these three I've encountered so far:
Comment #17
alexpottHmmm... think the direction this patch has taken is getting out-of-scope. Let's wind this back to being about just applying the already agreed coding standards to PHP files outside /core.
The question of coding standards of code inside @code docblocks is another issue entirely.
Comment #18
faline CreditAttribution: faline at CI&T commentedI've change the array() -> [] in some more files.
I didn't change on @code docblocks per comment #17
Comment #19
benjifisher@faline, I am guessing that you just forgot to move the issue back to "Needs Review". If you had a reason not to do that, then I apologize.
Is the goal to get 100% of the remaining
array()
s out of core, or if we get 80% to 90% there, can we commit it and then do the rest in a follow-up? This seems like the sort of patch that will lead to a lot of re-rolls if it stays open too long.Comment #20
benjifisherComment #21
joelpittetCan @benjifisher or @faline move the @code block changes to a follow-up as mentioned in #17?
Comment #22
benjifisherLet's see, I think I can do this. The interdiff for this should be the same as the first patch on the follow-up issue!
I am going through the patch and reverting changes to
@code
blocks. I am leaving in changes to@param
lines (and other lines) in doc blocks. I am not sure whether we want to update those (as part of this issue): for now, I am just leaving in such changes from the previous patch.Then there is this, which I am also leaving in:
I am also leaving in this, which may be out of scope:
There are also some changes in
core/scripts/run-tests.sh
that look like unrelated fixes.Why are we commenting out this section of
settings.php
?I am feeling a little bleary-eyed, so these patch files could use another look!
Comment #23
benjifisherI am updating the issue summary to reflect the scope stated in #17: this issue only deals with files outside the
core/
directory.I am also updating the
grep
command in the issue summary to reflect the scope.Comment #24
John Cook CreditAttribution: John Cook at Creode commentedI've had a check of the patch in comment #22 and it does do what the issue intends to do.
But the patch does touch files inside the core directory. New issues need to be created as necessary (one per module / sub-system) and the changes in this patch need to be moved there. Maybe separate documentation tickets also need to be created where needed.
When running the check before I get 16 occurrences of "array(" and 11 afterwards. These 11 remaining occurrences are all in @code sections of comments.
For reference, the command I used was:
Running the search over core as well gave 10700(approx) occurrences of "array(" over 180 files spread over about 50 modules and subsystems.
Comment #25
cilefen CreditAttribution: cilefen commentedThose are probably #2874067: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard.
Comment #26
vegantriathleteComment #27
Mile23If you find that you're correcting an array in a file that has
@codingStandardsIgnoreStart
or@codingStandardsIgnoreFile
annotation, and you can remove that annotation, you should.You would not remove that annotation if it leads to out-of-scope phpcs errors.
I doubt it will be possible, but it's worth mentioning.
Comment #28
vegantriathleteComment #30
gigiabba CreditAttribution: gigiabba at Ibuildings commentedAs per comments #17, #23 and #24, I did a new patch that affects only files outside
/core
directory.Thank you guys for the useful hints about using the
grep
command: @joelpittet, @Manuel Garcia, @benjifisher and @John CookComment #32
Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #33
MaskyS CreditAttribution: MaskyS at Google Code-In commentedComment #34
Maheshwaran.j CreditAttribution: Maheshwaran.j as a volunteer and at DrupalPartners for Innoppl Technologies Pvt. Ltd commentedComment #35
Maheshwaran.j CreditAttribution: Maheshwaran.j as a volunteer and at DrupalPartners for Innoppl Technologies Pvt. Ltd commentedPatch is good to go.
My doubt is when after installed databases creates array, is there any separate issue for that?or do we need to work on this.
Comment #36
benjifisher@Maheshwaran.j, I am not sure what you are asking. The file
sites/default/default.settings.php
currently has the codeand the current patch changes that to
I do not see the line
at all, although I do see
which should be handled in the related issue #2874067: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard.
I do not see how the other lines you quote relate to this issue.
Comment #38
Maheshwaran.j CreditAttribution: Maheshwaran.j as a volunteer and at DrupalPartners for Innoppl Technologies Pvt. Ltd commented@benjifisher
When we freshly install a new site, settings.php will create the database array, will that change too?
I might not have asked the question properly but I got the answer thanks :)
Comment #39
ankitjain28may CreditAttribution: ankitjain28may as a volunteer and at Google Summer of Code commentedCan you please let me know, which files need to be changed because the most of the files in the
core
directory need to be changed so should i make a patch for that or should i make the patch only for thesites
directory ?Comment #40
cilefen CreditAttribution: cilefen as a volunteer commented@ankitjain28may There may be PHP files in /core using array() that are not covered in #2874067: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard or in #2857771: Convert core to array syntax coding standards - API docs. If you find any, open a new child issue of #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase, because it has been decided that fixing /core is not a goal of this issue.
By the way, 'git grep' takes ~15 ms and naturally excludes non-project files:
git grep -l " array(" -- './*' ':!^core/'
So as to avoid confusion, I've retitled this issue accurately.
This needs work for #36 because it is duplicating work in #2874067: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard as mentioned in #36. @ankitjain28may: Perhaps you would like to sort that out in a new patch.
Thanks, everyone.
Comment #41
ankitjain28may CreditAttribution: ankitjain28may as a volunteer and at Google Summer of Code commentedThis patch will successfully fix this issue keeping the requirements in #36
Comment #42
MaskyS CreditAttribution: MaskyS at Google Code-In commented@ankitjain28, I know you're new here, but it's considered a good practice to attach an interdiff along with your patch. Makes reviewing easier :)
Comment #43
ankitjain28may CreditAttribution: ankitjain28may as a volunteer and at Google Summer of Code commented@MaskyS Ok
Comment #44
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedProviding interdiff :)
Comment #45
Maheshwaran.j CreditAttribution: Maheshwaran.j as a volunteer and at DrupalPartners for Innoppl Technologies Pvt. Ltd commentedI was just reading the issue queue.Ignore this comment. It's a mistake.
Comment #46
andypostAll places except doc-comments are fixed with #41
The rest will be done in #2874067: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard
Comment #48
catchCommitted bd9008c and pushed to 8.6.x. Thanks!