Use new array syntax in PHP files outside of /core.

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

Let's see

+++ b/core/phpcs.xml.dist
@@ -2,6 +2,8 @@
+  <file>../sites/default/default.settings.php</file>

Probably this tells us to move phpcs.xml.dist and phpcs.xml files one level up because now they are covering not only core/

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

+++ b/sites/default/default.settings.php
@@ -464,14 +464,14 @@
 /*
 if ($settings['hash_salt']) {
-  $prefix = 'drupal.' . hash('sha256', 'drupal.' . $settings['hash_salt']);
-  $apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader($prefix, $class_loader);
-  unset($prefix);
-  $class_loader->unregister();
-  $apc_loader->register();
-  $class_loader = $apc_loader;
+$prefix = 'drupal.' . hash('sha256', 'drupal.' . $settings['hash_salt']);
+$apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader($prefix, $class_loader);
+unset($prefix);
+$class_loader->unregister();
+$apc_loader->register();
+$class_loader = $apc_loader;
 }
-*/
+ */

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.

This change isn't right. Should maybe be in a coding standards ignore section

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

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

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

Edit: wrong issue, sorry.

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

Did you script this? If so could you share that in the IS?

I didn't @joelpittet, did it by hand...

Ok 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

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

• core/modules/contextual/src/Element/ContextualLinks.php
• core/modules/field_ui/src/Form/EntityDisplayFormBase.php
• core/modules/locale/locale.bulk.inc
• core/modules/migrate/src/Plugin/Migration.php
• core/modules/node/node.api.php
• core/modules/node/src/Plugin/Search/NodeSearch.php
• core/modules/rest/src/Plugin/ResourceInterface.php
• core/modules/simpletest/simpletest.module
• core/modules/simpletest/src/BlockCreationTrait.php
• core/modules/simpletest/src/NodeCreationTrait.php
• core/modules/simpletest/src/TestBase.php
• core/modules/simpletest/src/TestDiscovery.php
• core/modules/simpletest/src/WebTestBase.php
• core/modules/user/user.module
• core/modules/views/src/Annotation/ViewsDisplay.php
• core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php
• core/modules/views/src/Plugin/views/join/JoinPluginBase.php
• core/modules/views/views.api.php
• core/modules/views_ui/admin.inc
• core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php

PLENTY left to do, just run the command grep -r --exclude-dir='[Tt]ests' --exclude='*Test.php' '[^_]array(' core > arrays.txt, open up arrays.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:

core/modules/hal/src/Normalizer/ContentEntityNormalizer.php:   *   The type array(s) (value of the 'type' attribute of the incoming data).
core/modules/views/src/ViewExecutable.php:   * @var array()
core/modules/views/src/Plugin/views/wizard/WizardPluginBase.php:   * @var array()

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

I've change the array() -> [] in some more files.
I didn't change on @code docblocks per comment #17

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

Can @benjifisher or @faline move the @code block changes to a follow-up as mentioned in #17?

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

--- a/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
+++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
@@ -77,12 +77,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
 
     /**
      * Filter groups is an array that contains:
-     * array(
+     * [
      *   'operator' => 'and' || 'or',
-     *   'groups' => array(
+     *   'groups' => [
      *     $group_id => 'and' || 'or',
-     *   ),
-     * );
+     *   ],
+     * ];
      */
 
     $grouping = count(array_keys($groups['groups'])) > 1;

I am also leaving in this, which may be out of scope:

--- a/core/modules/locale/locale.bulk.inc
+++ b/core/modules/locale/locale.bulk.inc
@@ -205,7 +205,7 @@ function locale_translate_batch_import($file, array $options, &$context) {
           'seek' => 0,
         ];
       }
-      // Update the seek and the number of items in the $options array().
+      // Update the seek and the number of items in the $options array.
       $options['seek'] = $context['sandbox']['parse_state']['seek'];
       $options['items'] = $context['sandbox']['parse_state']['chunk_size'];
       $report = Gettext::fileToDatabase($file, $options);

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?

@@ -462,16 +462,14 @@
  * example, to use Symfony's APC class loader without automatic detection,
  * uncomment the code below.
  */
-/*
-if ($settings['hash_salt']) {
-  $prefix = 'drupal.' . hash('sha256', 'drupal.' . $settings['hash_salt']);
-  $apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader($prefix, $class_loader);
-  unset($prefix);
-  $class_loader->unregister();
-  $apc_loader->register();
-  $class_loader = $apc_loader;
-}
-*/
+# if ($settings['hash_salt']) {
+#   $prefix = 'drupal.' . hash('sha256', 'drupal.' . $settings['hash_salt']);
+#   $apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader($prefix, $class_loader);
+#   unset($prefix);
+#   $class_loader->unregister();
+#   $apc_loader->register();
+#   $class_loader = $apc_loader;
+# }

I am feeling a little bleary-eyed, so these patch files could use another look!

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

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

$ grep -rn --exclude-dir='core' --exclude-dir='.git' --exclude-dir='vendor' --exclude="./sites/default/files/*" --exclude="./modules/*" '[^_]array(' .

Running the search over core as well gave 10700(approx) occurrences of "array(" over 180 files spread over about 50 modules and subsystems.

Those are probably #2874067: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard.

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

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

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

$databases['default']['default'] = array ();
$settings['install_profile'] = 'standard';
$config_directories['sync'] = '';

@Maheshwaran.j, I am not sure what you are asking. The file sites/default/default.settings.php currently has the code

$databases = array();

and the current patch changes that to

$databases = [];

I do not see the line

$databases['default']['default'] = array ();

at all, although I do see

 * @code
 * $databases['default']['default'] = array (
 * // ...
 * );
 * @endcode

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.

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

Can 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 the sites directory ?

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

This patch will successfully fix this issue keeping the requirements in #36

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

@MaskyS Ok

Providing interdiff :)

I was just reading the issue queue.Ignore this comment. It's a mistake.

All places except doc-comments are fixed with #41
The rest will be done in #2874067: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard

Committed bd9008c and pushed to 8.6.x. Thanks!