PSR2 forcing exact indent for function call opening statements

[lcobucci]

When running v3.1.1 and using PSR-2 as standard, this code doesn't show any error:

return (new Builder())->setA(1)
                      ->setB(2)
                      ->setC(
                          new Test(3)
                      );

On 3.2 (because of the changes in 9286141, I believe) PHPCS is not handling line alignments the way it used to do.

Since PSR-2 doesn't define rules for line alignments I believe that PHPCS shouldn't enforce anything, so maybe this is a BC-break.

v3.1.1:

$ phpcs aa.php 
. 1 / 1 (100%)


Time: 135ms; Memory: 6Mb

v3.2.0:

$ phpcs aa.php 
E 1 / 1 (100%)



FILE: aa.php
-----------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-----------------------------------------------------------------------------------------------------------------------
 4 | ERROR | [x] Opening statement of multi-line function call not indented correctly; expected 20 spaces but found 22
 5 | ERROR | [x] Multi-line function call not indented correctly; expected 24 spaces but found 26
 6 | ERROR | [x] Multi-line function call not indented correctly; expected 20 spaces but found 22
-----------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------

Time: 85ms; Memory: 6Mb

[gsherwood]

This is a tough one :(

The reason the sniff is enforcing that functions open at an exact tab stop is because it causes all sorts of scope indentation issues when they don't. But the intention was not to ban code like this. I've got a feeling this could be tricky to fix, so I'll need to investigate a lot more.

[lcobucci]

@gsherwood I imagine, please let me know if I can do anything to help on that.

[gsherwood]

I took a longer look at this and I think I need to keep this rule about the indent level for function call opening statements. But it shouldn't be reporting an error for PSR2, so I've muted that.

What the change does do is more correctly enforce the rule Argument lists MAY be split across multiple lines, where each subsequent line is indented once. from section 4.6. Previously, the fact that the function call didn't start on a tab stop meant you needed to indent the containing lines an additional 4 spaces from that position, meaning they also weren't at tab stops. That meant all other code blocks inside (if, foreach etc) had to also start at a non tab stop column. You didn't get any errors, but checks were not being enforced properly for the sample code you provided.

So now, with that sample code, you get these errors:

 5 | ERROR | [x] Multi-line function call not indented correctly; expected 24 spaces but found 26
 6 | ERROR | [x] Multi-line function call not indented correctly; expected 20 spaces but found 22

Which is PHPCS trying to ensure the rest of the function call body is indented to the correct tab stops.

But that second error is obviously not correct because it means the closing brace is not aligned with the opening statement. I still need to look into that specific error and see what can be done.

[gsherwood]

Actually, that second error was easy. I changed the sniff to ensure the closing brace is aligned with where the opening statement actually is instead of where the sniff thinks the opening statement should be. This allows the opening statement error to be muted without affecting the closing brace.

Thanks for letting me know about this.

[lcobucci]

@gsherwood I've just ran dev-master on my project and unfortunately it didn't work 100% (it shows less violations though).

This is the complete file:

<?php
declare(strict_types=1);

require __DIR__ . '/../vendor/autoload.php';

use Lcobucci\Chimera\Bus\Tactician\DependencyInjection as Bus;
use Lcobucci\Chimera\Routing\Expressive\DependencyInjection as Routing;
use Lcobucci\DependencyInjection\ContainerBuilder;
use Usabilla\HealthCheck\DependencyInjection\Package as HealthCheck;
use Usabilla\Logging\DependencyInjection\Package as Logging;
use Usabilla\Monitoring\DependencyInjection\Package as Monitoring;
use Usabilla\Serialization\DependencyInjection\Package as Serialization;

\Doctrine\Common\Annotations\AnnotationRegistry::registerLoader('class_exists');

$builder    = new ContainerBuilder();
$appMode    = getenv('APPLICATION_MODE') ?: 'prod';
$forceCache = $appMode === 'prod';

if (! $forceCache) {
    $builder->useDevelopmentMode();
}

$rootDir = dirname(__DIR__);

return $builder->setParameter('app.basedir', $rootDir . '/')
               ->setDumpDir($rootDir . '/tmp/cache')
               ->addFile(__DIR__ . '/container.xml')
               ->addFile(__DIR__ . '/modes/' . $appMode . '.xml')
               ->addFile($rootDir . '/src/services.xml')
               ->addPackage(HealthCheck::class)
               ->addPackage(Logging::class)
               ->addPackage(Monitoring::class)
               ->addPackage(Serialization::class)
               ->addDelayedPass(
                   Bus\RegisterServices::class,
                   [
                       'feedback-collector.command_bus',
                       'feedback-collector.query_bus',
                       ['message_creator' => 'jms_message_creator']
                   ]
               )->addDelayedPass(
                   Routing\RegisterServices::class,
                   [
                       'feedback-collector',
                       'feedback-collector.command_bus',
                       'feedback-collector.query_bus',
                       [
                           'result_converter' => 'result_converter.accept-header',
                           'router_config' => [
                               'cache_enabled' => $forceCache,
                               'cache_file' => $rootDir . '/tmp/cache/routing.php'
                           ]
                       ]
                   ]
               );

Output:

FILE: config/container_builder.php
---------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
---------------------------------------------------------------------------------------------------
 36 | ERROR | [x] Multi-line function call not indented correctly; expected 16 spaces but found 19
 37 | ERROR | [x] Multi-line function call not indented correctly; expected 16 spaces but found 19
 43 | ERROR | [x] Multi-line function call not indented correctly; expected 16 spaces but found 19
 44 | ERROR | [x] Multi-line function call not indented correctly; expected 16 spaces but found 19
---------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------

Time: 159ms; Memory: 6Mb

Fixed file (using phpcbf):

<?php
declare(strict_types=1);

require __DIR__ . '/../vendor/autoload.php';

use Lcobucci\Chimera\Bus\Tactician\DependencyInjection as Bus;
use Lcobucci\Chimera\Routing\Expressive\DependencyInjection as Routing;
use Lcobucci\DependencyInjection\ContainerBuilder;
use Usabilla\HealthCheck\DependencyInjection\Package as HealthCheck;
use Usabilla\Logging\DependencyInjection\Package as Logging;
use Usabilla\Monitoring\DependencyInjection\Package as Monitoring;
use Usabilla\Serialization\DependencyInjection\Package as Serialization;

\Doctrine\Common\Annotations\AnnotationRegistry::registerLoader('class_exists');

$builder    = new ContainerBuilder();
$appMode    = getenv('APPLICATION_MODE') ?: 'prod';
$forceCache = $appMode === 'prod';

if (! $forceCache) {
    $builder->useDevelopmentMode();
}

$rootDir = dirname(__DIR__);

return $builder->setParameter('app.basedir', $rootDir . '/')
               ->setDumpDir($rootDir . '/tmp/cache')
               ->addFile(__DIR__ . '/container.xml')
               ->addFile(__DIR__ . '/modes/' . $appMode . '.xml')
               ->addFile($rootDir . '/src/services.xml')
               ->addPackage(HealthCheck::class)
               ->addPackage(Logging::class)
               ->addPackage(Monitoring::class)
               ->addPackage(Serialization::class)
               ->addDelayedPass(
                Bus\RegisterServices::class,
                [
                       'feedback-collector.command_bus',
                       'feedback-collector.query_bus',
                       ['message_creator' => 'jms_message_creator']
                   ]
               )->addDelayedPass(
                Routing\RegisterServices::class,
                [
                       'feedback-collector',
                       'feedback-collector.command_bus',
                       'feedback-collector.query_bus',
                       [
                           'result_converter' => 'result_converter.accept-header',
                           'router_config' => [
                               'cache_enabled' => $forceCache,
                               'cache_file' => $rootDir . '/tmp/cache/routing.php'
                           ]
                       ]
                   ]
               );

You can see that the indentation is not well respect like it was with v3.1.1...

[lcobucci]

Also, I get this output when running with that first sample code I've sent:

FILE: aa.php
--------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------
 5 | ERROR | [x] Multi-line function call not indented correctly; expected 24 spaces but found 26
--------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------

[lcobucci]

my phpcs.xml.dist is:

<?xml version="1.0"?>
<ruleset>
    <arg name="basepath" value="." />
    <arg name="extensions" value="php" />
    <arg name="colors" />
    <arg name="cache" value=".phpcs.cache" />
    <arg value="p" />

    <file>config</file>
    <file>src</file>
    <file>public</file>
    <file>tests</file>

    <rule ref="PSR2"/>
</ruleset>

[gsherwood]

Those errors you get, and the one from the sample code, are described in my comment above: #1793 (comment)

Edit: The fact that not all code gets aligned while fixed is just a result of the auto fixer not having that capability, and the PSR2 standard not doing things like defining array indent rules (so PHPCBF can't fix and enforce any).

[lcobucci]

What I meant is that I expected that after the fix PHPCS wouldn't report errors, since the code is not wrong.

[gsherwood]

I've got another idea about how to revert this for PSR2 but not break everything else. I'll give it a go.

[gsherwood]

I've pushed up another change so your code no longer produces errors with PSR2. Thanks for being patient with me :)

[lcobucci]

@gsherwood it's perfect now, thanks for your help =)