squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

Bad indentation sniff on multiple array arguments #751

Closed soullivaneuh closed 8 years ago

soullivaneuh commented 8 years ago

With this code:

        $formMapper
            ->with('General')
                ->add('name')
                ->add('values', 'sonata_type_collection', [
                    'by_reference'      => false,
                    'mapped'            => true,
                ], [
                    'edit'              => 'inline',
                    'inline'            => 'table',
                    'link_parameters'   => ['context' => $context],
                ])
            ->end()
        ;

CS found an error on line with ], [:

Multi-line function call not indented correctly; expected 20 spaces but found 16

But I think this is wrong.

The only way to make phpcs happy is to make this following "correction":

        $formMapper
            ->with('General')
                ->add('name')
                ->add('values', 'sonata_type_collection', [
                    'by_reference'      => false,
                    'mapped'            => true,
                    ], [
                    'edit'              => 'inline',
                    'inline'            => 'table',
                    'link_parameters'   => ['context' => $context],
                    ])
            ->end()
        ;

But that makes no sense IMO.

JDGrimes commented 8 years ago

I think this is probably related to #721.

soullivaneuh commented 8 years ago

Maybe related, but in this case, the fixed code is just absolutely not correct.

gsherwood commented 8 years ago

The error and fix are correct as per the sniff's rules. It expects all lines within a function call to be indented 4 spaces from the start of the function call. This sniff is part of the PEAR standard, which is documented here: https://pear.php.net/manual/en/standards.funcalls.php

If that's not how you write your code, then you might not be able to use this sniff or standard.

Or, you can format the call with your arguments over more lines, which is also valid for this sniff:

$formMapper
    ->with('General')
        ->add('name')
        ->add(
            'values',
            'sonata_type_collection',
            [
                'by_reference'      => false,
                'mapped'            => true,
            ],
            [
                'edit'              => 'inline',
                'inline'            => 'table',
                'link_parameters'   => ['context' => $context],
            ]
        )
    ->end()
;

Your comments indicate you feel very strongly that the sniff is completely broken and it's indent rules should be ignored in your specific case, so it's possible I've misunderstood what is going on here. If that's the case, please reply with some more detail and I'll take a look again.

soullivaneuh commented 8 years ago

Thanks for responding.

I don't want to write multi-line call. My code it's an inline call, but with multi-line arrays.

For me, this is two different part.

I think this is not correct because my second past is the code corrected by phpcbf. I think you'll agree that it's not correct.

BTW, if I have a call with only one array like this:

->add('webServer', 'choice', [
    'choices'   => [
        'Apache'    => 'apache',
        'Nginx'     => 'nginx',
    ],
    'choices_as_values' => true,
])

It works, no warnings or bad correction.

gsherwood commented 8 years ago

I think this is not correct because my second past is the code corrected by phpcbf. I think you'll agree that it's not correct.

I don't agree, no. The fix looks fine to me. The line is properly indented 4 spaces from the start of the function call, which is what the PEAR coding standard requires and is exactly what the error message is saying. You might not like the way the array is indented, but that's not what this sniff is checking, and the PEAR standard doesn't actually say how arrays should be defined.

BTW, if I have a call with only one array like this: ... It works, no warnings or bad correction

This error has nothing to do with the number of arrays. The error is there because every line within the function call must be indented 4 spaces from the start of the call (except for the line with the closing parenthesis). In your original example, only one line violates the PEAR standard (the line with ] , [) and so that is the error you get. In your latest example (the comment above) all the lines are fine.

I can't see any problem with this sniff or the corrections it made. I think this is just a case of you not wanting to follow the rules of the PEAR standard, which is perfectly fine. Maybe you'd prefer following the PSR2 standard, which doesn't report any errors for your original call.

soullivaneuh commented 8 years ago

The error is there because every line within the function call must be indented 4 spaces from the start of the call (except for the line with the closing parenthesis)

See again the second past, the closing parenthesis is not indented correctly so.

And, is that a PEAR compliant to have closing arrays (]) as the same indent level as the content? I don't think so IMHO.

Maybe you'd prefer following the PSR2 standard, which doesn't report any errors for your original call.

Maybe. Does PSR-2 has fixer about array and function call indent for multi-line? Because I want this, but not with this way that I think it's wrong.

gsherwood commented 8 years ago

See again the second past, the closing parenthesis is not indented correctly so.

The line with the closing parenthesis must be indented to the same column as the start of the call.

And, is that a PEAR compliant to have closing arrays (]) as the same indent level as the content? I don't think so IMHO.

It's not a matter of opinion. The PEAR standard is documented (I linked to the relevant section above) and it even includes this example, where the closing array parenthesis is very clearly indented to the same level as the main content:

<?php

$this->someObject->subObject->callThisFunctionWithALongName(
    $this->someOtherFunc(
        $this->someEvenOtherFunc(
            'Help me!',
            array(
                'foo'  => 'bar',
                'spam' => 'eggs',
            ),
            23
        ),
        $this->someEvenOtherFunc()
    ),
    $this->wowowowowow(12)
);
?>

Maybe. Does PSR-2 has fixer about array and function call indent for multi-line? Because I want this, but not with this way that I think it's wrong.

The PSR-2 coding standard doesn't have any rules about array indentation, so no, it wont fix your arrays. It will fix your function calls to adhere to the PSR-2 standard: http://www.php-fig.org/psr/psr-2/ (see section 4.6). The standard is basically the same as PEAR, except that it doesn't consider your code a multi-line call because you are only using newlines for indenting arrays.

soullivaneuh commented 8 years ago

The line with the closing parenthesis must be indented to the same column as the start of the call.

So this:

->add('webServer', 'choice', [
    'choices'   => [
        'Apache'    => 'apache',
        'Nginx'     => 'nginx',
    ],
    'choices_as_values' => true,
])

Should not be ok, isn't it?

I'm sorry to insist, but it's hard to understand. It's like there is a conflict with CS here.

gsherwood commented 8 years ago

The indent of the final line is fine. It generates a different error from a different part of the standard. The error is: Closing parenthesis of a multi-line function call must be on a line by itself

soullivaneuh commented 8 years ago

The error is: Closing parenthesis of a multi-line function call must be on a line by itself

This sniff is deactivated for SF standard AFAIK. This is was I'm talking about CS conflicts.

gsherwood commented 8 years ago

I don't know what the SF standard is, but maybe you should be talking to whoever maintains that standard if you think it has a conflict as there isn't anything I can do to fix it.

Maybe the SF standard needs to use a different sniff, or even a custom sniff, if it doesn't want to use the same rules as the PEAR standard.