Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Squiz.Arrays.ArrayDeclaration fixer sometimes puts a comma in front of the last array value #1982

Closed
TysonAndre opened this issue Apr 5, 2018 · 2 comments
Milestone

Comments

@TysonAndre
Copy link
Contributor

TysonAndre commented Apr 5, 2018

This happens both in 3.2.3 and master. I didn't see any similar issues after a quick search for ArrayDeclaration or commas. It looks like it's related to the length of the keys/values found in the array.

Expected: Either a valid AST is created, or the fix is not saved.
Observed: The fix suggested by phpcbf for ArrayDeclaration is invalid PHP code

The input file:

<?php
$x = [
    'xxxx'   => ['aaaaaaaaaa' => 'ccccccccccc',
                 'bbbbbbbb'   => false],
];

The invalid output (Note that =>,false is generated)

<?php
$x = [
    'xxxx' => [
'aaaaaaaaaa' => 'ccccccccccc',
                 'bbbbbbbb'   =>,false
],
];

The below config can be used to reproduce this issue

<?xml version="1.0"?>
<ruleset name="myset">
    <description>A coding standard</description>

    <rule ref="Squiz.Arrays.ArrayDeclaration">
        <exclude name="Squiz.Arrays.ArrayDeclaration.MultiLineNotAllowed" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed" />

        <exclude name="Squiz.Arrays.ArrayDeclaration.KeyNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.KeyIncorrect" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.CloseBraceNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.ValueNotAligned" />
    </rule>
</ruleset>
@gsherwood
Copy link
Member

Simplified test case:

<?php
$x = [
    'a' => false];

Debug output using phpcs temp.php --standard=Squiz --sniffs=Squiz.Arrays.ArrayDeclaration --report=diff -vvv

	*** START FILE FIXING ***
---START FILE CONTENT---
1|<?php
2|$x = [
3|    'a' => false];
4|
--- END FILE CONTENT ---
	E: [Line 3] Each line in an array declaration must end in a comma (Squiz.Arrays.ArrayDeclaration.NoComma)
	PHP_CodeSniffer\Standards\Squiz\Sniffs\Arrays\ArrayDeclarationSniff (line 843) replaced token 12 (T_FALSE) "false" => ",false"
	* fixed 1 violations, starting loop 2 *
---START FILE CONTENT---
1|<?php
2|$x = [
3|    'a' => ,false];
4|
--- END FILE CONTENT ---
	E: [Line 3] Expected 0 spaces between "=>" and comma; 1 found (Squiz.Arrays.ArrayDeclaration.SpaceBeforeComma)
	PHP_CodeSniffer\Standards\Squiz\Sniffs\Arrays\ArrayDeclarationSniff (line 862) replaced token 11 (T_WHITESPACE) "·," => ","
	* fixed 1 violations, starting loop 3 *
---START FILE CONTENT---
1|<?php
2|$x = [
3|    'a' =>,false];
4|
--- END FILE CONTENT ---
	*** END FILE FIXING ***
--- temp.php
+++ PHP_CodeSniffer
@@ -1,3 +1,3 @@
 <?php
 $x = [
-    'a' => false];
+    'a' =>,false];

@gsherwood gsherwood added this to the 3.3.0 milestone Apr 5, 2018
@gsherwood gsherwood changed the title phpcbf fix for Squiz.Arrays.ArrayDeclaration produces invalid PHP AST. The comma is in an invalid position. Squiz.Arrays.ArrayDeclaration fixer sometimes puts a comma in front of the last array value Apr 5, 2018
gsherwood added a commit that referenced this issue Apr 5, 2018
…a comma in front of the last array value
@gsherwood
Copy link
Member

gsherwood commented Apr 5, 2018

Thanks for reporting this. The fix has now been corrected and the comma is placed after the array value. This looks to have only happened when the array end brace came immediately after the last array value. This fix will be in the 3.3.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants