Since we are now using PHP 7+ we can use List (array destructuring) assignment using the array syntax. Requires PHP >= 7.1. In the related issue it was suggested to make a new issue for this change.

-list($sample) = $array;
+[$sample] = $array;

How to reproduce

php-cs-fixer

Added a config this time so it will do everything at once.

git clone --branch '9.3.x' https://git.drupalcode.org/project/drupal.git
cd drupal
mkdir --parents tools/php-cs-fixer
composer require --working-dir=tools/php-cs-fixer friendsofphp/php-cs-fixer

Now create the config file at tools/php-cs-fixer/.php-cs-fixer.dist.php.

<?php
/*
 * This document has been generated with
 * https://mlocati.github.io/php-cs-fixer-configurator/#version:3.0.0|configurator
 * you can change this configuration by importing this file.
 */
$config = new PhpCsFixer\Config();
$finder = PhpCsFixer\Finder::create()
  ->in(['core', 'composer'])
  ->exclude('vendor')
  ->name('*.php')
  ->name('*.sh')
  ->name('*.theme')
  ->name('*.module')
  ->name('*.inc');

return $config
    ->setRules([
        'list_syntax' => true,
    ])
    ->setFinder($finder);

Run the fixer

tools/php-cs-fixer/vendor/bin/php-cs-fixer fix . -v --diff --config tools/php-cs-fixer/.php-cs-fixer.dist.php --path-mode=intersection
git add core

There is some manual work to fix some style issues like: ([$var,] and indentation faults).

Issue fork drupal-3222769

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bbrala created an issue. See original summary.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

MR is incoming in a bit.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Big +1 to this as PHPStorm loves to make this change for you, that you then have to manually revert before posting patches.

bbrala opened merge request !912

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Symfony ships a php-cs-fixer config, I wonder if we should consider doing the same and running it as part of our commit checks: https://github.com/symfony/symfony/blob/5.4/.php-cs-fixer.dist.php

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

Drupal uses php codesniffer right now for that. But yeah, Symfony does use php-cs-config.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

To be honest, extending the ruleset of the Drupal PHP CodeSniffer with these rules should the way to go, otherwise its quite the change we are looking at.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Nothing to stop us using both, I think they both have value and do slightly different things.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

Yeah but would mean 2 different tools would start scanning the whole codebase, which might do conflicting things. But that is 'but but'-me talking. :)

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented
Issue summary: View changes
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented
Title: Replace all list (array destructuring) assignment to the array syntax » [November 8, 2021] Replace all list (array destructuring) assignment to the array syntax
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

Ping :)

What would you need here?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Postponed » Active

Let's at least roll a patch that fixes all cases in core, not sure we should introduce PHP CS fixer here as well, that should probably be a separate issue.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented
Version: 9.3.x-dev » 9.4.x-dev

i'll update the mr tonight, reproducing is easy enough ;)

Guessing target should be 9.4.x?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

It will need to be committed to both branches, hopefully the same MR will apply cleanly to both.

bbrala opened merge request !1393

bbrala closed merge request !912

bbrala opened merge request !1394

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

I'll update the branches with proper codestyle later. Those are currently failing.

bbrala closed merge request !1394

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented

Seems 9.3.x diff is literally the same as the 9.4.x diff, so closing the separate MR

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Status: Active » Needs review

Everything is green, changes where automated for 95% setting to needs review. ^^

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

This looks close but there are a few more cases we should fix up:

$ rg '\Wlist\('
core/scripts/run-tests.sh
470:    list($php) = explode(' ', $sudo, 2);
827:      list($class_name, $method) = explode('::', $test_class, 2);
1051:        list($class_name) = explode('::', $test_class, 2);
1267:        list($class, $name) = explode('->', $result->function, 2);

composer/Plugin/Scaffold/Interpolator.php
151:      list($sourceText, $key) = $matchSet;

core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
143:          //   list($field_name, $langcode, $values) = $units[$key]; causes

alexpott made their first commit to this issue’s fork.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review

I ran the following regex on core's codebase and the composer directory (\W)list\(([^\)]*)\) and replace $1\[$2\]

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

No instances of \Wlist\( left in core so to me this is complete.

Matroskeen’s picture

Matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
CreditAttribution: Matroskeen at DevBranch, Evolving Web, Drupal Ukraine Community commented
Status: Reviewed & tested by the community » Needs review

I already asked in Slack, but I want to document it here:
1) Should we create a follow-up before it's committed?
2) Is it fine to introduce this change before having the actual sniff that will catch new changes coming with the old style? (I'm just wondering about the policy so I can proceed in the same way here #2960522: Use root-namespaced native functions for better opcache'd performance)
Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

I don't think we should go out of our way to have a rule that says [$var] = explode() and not list($var) = explode(). At best it is preference. PHPStorm complicates the issue because it automatically converts this on save so you get unrelated change which why this definitely should go in and any discussion about PHPCS sniffs should go later.

If we want we could add a sniff that extends \PHP_CodeSniffer\Standards\Squiz\Sniffs\PHP\DiscouragedFunctionsSniff and uses the same logic to report usage of the list() function.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Reviewed & tested by the community

Discussed #28 and #29 with @Matroskeen on slack and he said

Yeah, it makes sense, thanks! I asked in d.org issue too, so feel free to move back to RTBC :slightly_smiling_face:

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
  1. I've created #3248483: Codestyle check for list (array destructuring) assignment to the array syntax.
  2. I do remember some discussion on this on Slack faguely, which seemed to be okay with delaying that at first since this change needs to be close to a beta release. Also an argument can be made that introducing a sniff right away will break a lot of builds in this phase, which could be problematic. Perhaps making sure having a sniff available around release is better. But for that i'd kinda like to defer to core committers. :)

In order to get that reviewed by some of the core ppl, a RTBC is a good status to be in ;) Perhaps setting it back is in order?

Edit: this comment was posted before reading the above on slack and here.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Issue summary: View changes

Update config and command to invlude .sh scripts and also the composer directory. This will minimize any fix work after the command is run. (for future reference).

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Saving issue credits

  • larowlan committed 6d39cde on 9.4.x 
    Issue #3222769 by bbrala, alexpott, longwave, Matroskeen: [November 8,...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Reviewed this locally using git diff --color-words

Whilst it didn't apply cleanly after #3222251: [November 8, 2021] Replace all isset constructs with the null coalescing operator, it did apply with a three way merge.

Got to say it was much nicer to review than #3222251: [November 8, 2021] Replace all isset constructs with the null coalescing operator.

Committed 6d39cde and pushed to 9.4.x. Thanks!

To maximise the possibility of backporting applying cleanly, backported to 9.3.x

Wrote and published a change record.

  • larowlan committed e125ce2 on 9.3.x 
    Issue #3222769 by bbrala, alexpott, longwave, Matroskeen: [November 8,...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Related issues: +#3010032: Add dependency on slevomat/coding-standard

We should add a phpcs rule enforcing this style choice.

Per @alexpott #3010032: Add dependency on slevomat/coding-standard includes a rule for this change.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented

Nice one xjm :)

Status: Fixed » Closed (fixed)

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