Php_codesniffer: Incorrect indentation for arguments of multiline function calls

[Levivb]

Hey,

The enforced indentation of the arguments of a multiline function call seems incorrect.

Given the following example was correct in 3.5.0:

        $componentType = $this->componentTypeRepository->findByType($this->identifier) ?:
            $this->componentTypeFactory->createForType(
                $this->identifier,
                $this->className,
                true,
                $this->isPrototypal
            );

Since 3.5.1 it gives an error and wants to fix it to:

        $componentType = $this->componentTypeRepository->findByType($this->identifier) ?:
            $this->componentTypeFactory->createForType(
            $this->identifier,
            $this->className,
            true,
            $this->isPrototypal
        );

162 | ERROR | [x] Multi-line function call not indented correctly expected 16 spaces but found 20

Also in arrays:

Correct in 3.5.0:

<?php

declare(strict_types=1);

return [
    'export-path' => 'exports/database/'
        . env(
            'APP_CUSTOMER',
            'not-configured'
        )
        . '/' . env(
            'APP_IDENTIFIER',
            'not-configured'
        ),

Since 3.5.1 it gives an error and wants to fix it to:

<?php

declare(strict_types=1);

return [
    'export-path' => 'exports/database/'
        . env(
        'APP_CUSTOMER',
        'not-configured'
    )
        . '/' . env(
        'APP_IDENTIFIER',
        'not-configured'
    ),

8 | ERROR | [x] Multi-line function call not indented correctly expected 8 spaces but found 12

This one even conflicts with Generic.Whitespace.ScopeIndent since after 'fixing' it, the closing brace of the env call is now indented four spaces, but Generic.Whitespace.ScopeIndent expects it to be eight.

Am I missing something or is the indentation check truly incorrect?

[phil-davis]

Similar here. My CI started failing last night, and I had to make this PR to "correct" the code style:
https://github.com/owncloud/core/pull/36286/files

Is the "new indent level" more correct than the previous behaviour?

[besanek]

@phil-davis the new behaviour is bug. Do not change your code for CI pass. Rather than that, add version _3.5.1_ to _composer.json_ as conflict.

  ...
  "conflict": {
    "squizlabs/php_codesniffer": "3.5.1"
  },
  ...

[phil-davis]

Thanks @besanek - that is a better idea than "fixing" the indent then having to "fix" it back again when 3.5.2 comes out.

[gsdevme]

Also seeing this issue.

[dereuromark]

I encounter the same issues, and we had to pin it down to 3.5.0

See e.g.

        $methods .=
            str_replace(
                array_keys($replacements),
                array_values($replacements),
                $methodTemplate
            )
            . PHP_EOL . PHP_EOL . str_repeat(' ', 4);

wrongly fixed to

        $methods .=
            str_replace(
            array_keys($replacements),
            array_values($replacements),
            $methodTemplate
        )
            . PHP_EOL . PHP_EOL . str_repeat(' ', 4);

Also:

        $rangeValues['min'] =
            $this->adjustLowerThreshold(
                $this->normalizeRatingForFilter($rangeValues['min'])
            );
    }

became

        $rangeValues['min'] =
            $this->adjustLowerThreshold(
            $this->normalizeRatingForFilter($rangeValues['min'])
        );

I kind of see that the first line could have first moved "up" and then it might not have run into those issues. But it shouldnt fix OKish code to this kind of broken indentation.

[dereuromark]

Also:

        $salesOrderThresholdTransfer->fromArray($salesOrderThresholdEntity->toArray(), true)
            ->setSalesOrderThresholdValue(
                $this->mapSalesOrderThresholdValueTransfer($salesOrderThresholdTransfer, $salesOrderThresholdEntity)
            )->setCurrency(
                (new CurrencyTransfer())->fromArray($salesOrderThresholdEntity->getCurrency()->toArray(), true)
            )->setStore(
                (new StoreTransfer())->fromArray($salesOrderThresholdEntity->getStore()->toArray(), true)
            );

becomes

        $salesOrderThresholdTransfer->fromArray($salesOrderThresholdEntity->toArray(), true)
            ->setSalesOrderThresholdValue(
                $this->mapSalesOrderThresholdValueTransfer($salesOrderThresholdTransfer, $salesOrderThresholdEntity)
            )->setCurrency(
            (new CurrencyTransfer())->fromArray($salesOrderThresholdEntity->getCurrency()->toArray(), true)
        )->setStore(
            (new StoreTransfer())->fromArray($salesOrderThresholdEntity->getStore()->toArray(), true)
        );

So chaining indentation seems to break for us.

[gsherwood]

Thanks to everyone who submitted code sample for this bug report - it made it a lot easier to improve the tests and confirm the fix.

I've added a fix for the issue into master. It changes the behaviour of one specific test case that was added in 3.5.1, but it's such an edge case that I think the new behaviour is just as acceptable as the 3.5.1 behaviour. The code snippet is:

class C
{

    public function m()
    {
        $a = [];
        $t =
            "SELECT * FROM t
WHERE f IN(" . implode(
                ",",
                $a
            ) . ")";
    }
}

And this is now seen as valid indentation. The lack of whitespace before WHERE makes this a hard one for the sniff to figure out as there isn't a lot to go on. It now uses the position of "SELECT to validate the indentation, which is probably ok.

The committed fix now allows all code sample pasted here to pass without error. If anyone has time to run dev-master over their code to check the fix, I'd be very grateful.

[dereuromark]

I can confirm on our 20000 file code base that now all works fine.

[Levivb]

Confirmed for 20+ separate repositories. Got no more unexpected line indent requirements

[phil-davis]

Works for me

[besanek]

It works now, thank you!

[gsherwood]

Thanks to everyone who tested this fix. I've got a couple more bugs to include, then I'll get a 3.5.2 release out.