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.

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.
grep -r --exclude-dir='[Tt]ests' --exclude='*Test.php' '[^_]array(' core

User interface changes

-

API changes

-

Data model changes

-

Comments

lauriii created an issue. See original summary.

alexpott’s picture

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.

claudiu.cristea’s picture

Status: Active » Needs review
FileSize
13.6 KB

Let's see

claudiu.cristea’s picture

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

alexpott’s picture

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

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

FileSize
1023 bytes
13.76 KB

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

pfrenssen’s picture

Title: Ensure that core array syntax is conistent » Ensure that core array syntax is consistent

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.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community
pfrenssen’s picture

Edit: wrong issue, sorry.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

There are more PHP files under the core/scripts that are not cleaned up yet

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
17.92 KB
4.44 KB

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

joelpittet’s picture

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

Manuel Garcia’s picture

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

joelpittet’s picture

Issue summary: View changes

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

Manuel Garcia’s picture

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()
alexpott’s picture

Status: Needs review » Needs work

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.

faline’s picture

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