Sniffs Squiz.Arrays.ArrayDeclaration.[No]KeySpecified are excluded in Core but included in Coder ruleset

Actually I may be mistaken, this may not be a new sniff. I thought the message above had just appeared in a regular branch test. But it was in new code in a PR.

This is not new. Just found #2908266: Fix 'Squiz.Arrays.ArrayDeclaration.NoKeySpecified' and 'Squiz.Arrays.ArrayDeclaration.KeySpecified' coding standard → where it was decided not to adopt this sniff in Drupal coding standards and not fix any core code.

Maybe we should exclude it in Coder's ruleset file, so that contrib developers do not have to write less-readable code, that core maintainers already decided was not good for Core.

Updated issue summary.
Added https://github.com/pfrenssen/coder/pull/206 to fix this

Thanks, but I'm not sure we should exclude this. Your example in the test is bad code and I would not consider it "good". When a developer writes code like that then it is super useful if Coder complains because you should not mix explicit array keys with implicit numeric array keys. That can introduce weird bugs in your code later and it is very hard to read.

Therefore I would argue that we keep this in Coder per default as good advice.

I also party agree with the conclusions in the Drupal core issue that fixing it now later is super hard and makes the code look ugly as well. Drupal core needs to keep the numeric keys for compatibility reasons. But even then I would argue that the hidden numeric keys should be made explicit, which is not nice but at least crystal clear.

If Drupal core does not want to fix that then the best course of action would be to explicitly make exceptions in the PHPCS config for all those places (or with phpcs:ignore comments). Then at least all new code like that introduced in Drupal core gets flagged by Coder - as it should be!

Any more opinions on this?

:-) yes the example I gave in the good.php file does not look like "good" code. I was just adding it there as I thought that if Core did not like the rule and was not going to fix the problem cases, then Coder should not include the sniff. But I see your point that Coder can have its own view of standards that we want to enforce even if Core maintainers decide against it. Are there many sniffs that are like this?

So, if the rule stays in Coder's ruleset, ie implicitly by not being excluded in /Drupal/ruleset.xml is there something that needs to be updated in the Coding Standards pages, to say "This is a good standard, use it in Contrib, however Core is not going to follow it (for acceptable reasons, etc) - with links" . That way, Contrib developers know what the deal is.

Or should we push for core to use your suggestion of phpcs:ignore Squiz.Arrays.ArrayDeclaration.NoKeySpecified where it is needed, and then enable the rule so that no new cases are created.

I don't think we have many sniffs that deviate from the explicitly defined Drupal coding standards. When new standards are being proposed we often already change Coder sniffs so that Coder is ready when the standard is formalized and finished.

I like your idea of changing the coding standards for arrays to forbid mixing of explicit keys and implicit keys. Can you open an issue in the coding standards queue for that?

If that gets approved then Drupal core can still do the changes from #2908266: Fix 'Squiz.Arrays.ArrayDeclaration.NoKeySpecified' and 'Squiz.Arrays.ArrayDeclaration.KeySpecified' coding standard → or make ignore exceptions

Just ran a check in D10.0 dev

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Squiz.Arrays.ArrayDeclaration.NoKeySpecified                     219
Squiz.Arrays.ArrayDeclaration.KeySpecified                       3
----------------------------------------------------------------------
A TOTAL OF 222 SNIFF VIOLATIONS WERE FOUND IN 2 SOURCES
----------------------------------------------------------------------

However, a large proportion are in Core/lib/Drupal/Component/Transliteration/data which is already excluded from some other sniffs, so can be excluded here too. Then we get the manageble list of 14 failures

PHP CODE SNIFFER REPORT SUMMARY
------------------------------------------------------------------------------------
FILE                                                                ERRORS  WARNINGS
------------------------------------------------------------------------------------
Core/modules/language/language.admin.inc                            1       0
...ate/src/Plugin/migrate/destination/EntityFieldStorageConfig.php  1       0
...ay/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php  4       0
...s/system/tests/src/Functional/FileTransfer/FileTransferTest.php  1       0
Core/modules/system/tests/src/Kernel/Theme/FunctionsTest.php        1       0
Core/modules/views/tests/src/Unit/PluginBaseTest.php                1       0
...e/tests/Drupal/KernelTests/Core/Menu/LocalActionManagerTest.php  1       0
...ore/tests/Drupal/Tests/Component/Serialization/YamlTestBase.php  2       0
.../tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php  1       0
Core/tests/Drupal/Tests/Core/Render/RendererTest.php                1       0
------------------------------------------------------------------------------------
A TOTAL OF 14 ERRORS AND 0 WARNINGS WERE FOUND IN 10 FILES
------------------------------------------------------------------------------------

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Squiz.Arrays.ArrayDeclaration.NoKeySpecified                     11
Squiz.Arrays.ArrayDeclaration.KeySpecified                       3
----------------------------------------------------------------------
A TOTAL OF 14 SNIFF VIOLATIONS WERE FOUND IN 2 SOURCES
----------------------------------------------------------------------

Nice, so I will close this as won't fix for now, we want to keep this sniff in Coder.