[Php71] Add MultiDimensionalArrayToArrayDestructRector

[nikolicaleksa]

No description provided.

[TomasVotruba]

Hi, thanks for the proposal.
I personally find there very hard to read and modify later. Why do you think this rule should be part of generic set?

[nikolicaleksa]

Hi, thanks for the proposal. I personally find there very hard to read and modify later. Why do you think this rule should be part of generic set?

Hi, are you talking about the MR code being hard to read or the modified code that rector produces with this rule?

I think this rule should be part of a generic set as it's quite similar to the existing rule ListToArrayDestructRector which is already part of the set, the only difference is that this one works with associative arrays.

I personally find this PHP feature neat to use when you have multidimensional associative arrays as you only have couple of options

• Reference value by foreach-var and accessor all the time ($item['id'], $item['name']....)
• Introduce a local variable so you don't have to repeat writing the array accessor ($id = $item['id'];, $name = $item['name'];)
• Use array destructing to introduce local variables and save couple of lines

[ruudk]

Interesting rule. I never knew it was possible to destruct multi dimensional arrays like this. Only did it for values. TIL!

I think this rule should be part of a generic set as it's quite similar to the existing rule ListToArrayDestructRector which is already part of the set, the only difference is that this one works with associative arrays.

This is something completely different. The ListToArrayDestruct is converts an older syntax list($id1, $name1) = $array to a newer syntax [$id1, $name1] = $array. It's shorter and more common. Also, it changes a single line.

The rule you are proposing is a preference that can potentially create a lot of changes (everything in the loop) and is not something every developers wants.

[nikolicaleksa]

Interesting rule. I never knew it was possible to destruct multi dimensional arrays like this. Only did it for values. TIL!

I think this rule should be part of a generic set as it's quite similar to the existing rule ListToArrayDestructRector which is already part of the set, the only difference is that this one works with associative arrays.

This is something completely different. The ListToArrayDestruct is converts an older syntax list($id1, $name1) = $array to a newer syntax [$id1, $name1] = $array. It's shorter and more common. Also, it changes a single line.

The rule you are proposing is a preference that can potentially create a lot of changes (everything in the loop) and is not something every developers wants.

In that case, developer can disable the rule in the configuration or we could have this as part of some other set. In my opinion, even though it changes more than a single line, this is something that will contribute to making the code cleaner and shorter by remove the need for constant foreach variable reference.

[samsonasik]

I think this should not be included in config/sets, so user can manually enable it theirself

[nikolicaleksa]

I think this should not be included in config/sets, so user can manually enable it theirself

Also a possibility. Just let me know what you decide so that I can adjust.

[samsonasik]

I think not registered in any set is the way it become easy merge, as it may be personal preference syntax

[nikolicaleksa]

I think not registered in any set is the way it become easy merge, as it may be personal preference syntax

Done

[nikolicaleksa]

@samsonasik Seems like PHPStan is complaining now that it must be registered in a set. How should I fix this?

[samsonasik]

It seems you can ignore at phpstan.neon

rector-src/phpstan.neon

Lines 238 to 239 in d68e8fa

	# optional as changes behavior, should be used explicitly outside PHP upgrade
	- '#Register "Rector\\Php73\\Rector\\FuncCall\\JsonThrowOnErrorRector" service to "php73\.php" config set#'

[ruudk]

I think these changes need to be reverted.

[samsonasik]

the documentation is generated weekly, you can revert the doc change, and it will be regenerated later by CI

[ruudk]

Would be great if manual changes to these files are rejected :D

[TomasVotruba]

@nikolicaleksa I think better than PHP 7.1, where rules are features most devs want, is better to target "coding style" namespace, where it depends on taste. PHPStan error will be fixed :)

[nikolicaleksa]

@nikolicaleksa I think better than PHP 7.1, where rules are features most devs want, is better to target "coding style" namespace, where it depends on taste. PHPStan error will be fixed :)

Should I move it to "coding style" set and leave it enabled for that set or just move it there but not register?

[TomasVotruba]

@nikolicaleksa Only move to the namespace. No registrations as that would automatically run for everyone using the set.

[samsonasik]

PHPStan notice seems only on php set, on other namespace, that seems working ok without registering in config set.

[nikolicaleksa]

All done 🤝

[TomasVotruba]

Awesome! Thank you for fast work 🎉

[ruudk]

Just discussed this with a team member, and they gave this feedback:

I knew already, but never really used it as the foreach line becomes too long pretty quickly. You need to split into multiple lines, and then it becomes less readable often.
So doing it more like this:
foreach ($users as $user) {
  ['firstName' => $firstName, 'lastName' => $lastName] = $user;
  
  // .. more fancy stuff
}

Did you consider an alternative syntax as soon as the array shape line becomes too long?

[samsonasik]

It seems cause downgrade error:

---------------------------------------------------------
Parse error: rector-prefixed-downgraded/rules/CodingStyle/Rector/Foreach/MultiDimensionalArrayToArrayDestructRector.php:4
    2| 
    3| declare (strict_types=1);
  > 4| namespace Rector\CodingStyle\Rector\Foreach;

Foreach should be Foreach_

[samsonasik]

downgrade error should be resolved at #6045

[samsonasik]

@ruudk that possibly an improvement area to make rule configurable with "limit" variable config...