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

Incorrect indent for closures as method argument. #2078

Open
watari opened this issue Jun 30, 2018 · 6 comments · May be fixed by PHPCSStandards/PHP_CodeSniffer#38
Open

Incorrect indent for closures as method argument. #2078

watari opened this issue Jun 30, 2018 · 6 comments · May be fixed by PHPCSStandards/PHP_CodeSniffer#38

Comments

@watari
Copy link

watari commented Jun 30, 2018

Today encountered problem with codesniffer on PSR1/PSR2 rules set.
In my class I have next part of code:

protected function getCategoryFeatureRepositoryMock(
    array $map,
    int $getExistingIdsCalls
): CategoryFeatureRepositoryInterface {
    /** @var MockObject|CategoryFeatureRepository $repository */
    $repository = $this->getMockBuilder(CategoryFeatureRepository::class)
                       ->disableOriginalConstructor()
                       ->setMethods(['getCategoryFeatureIds'])
                       ->getMock();

    $repository->expects(self::exactly($getExistingIdsCalls))
               ->method('getCategoryFeatureIds')
               ->willReturnCallback(
                   function (string $providerId, int $categoryId, array $idsList) use ($map) { \\ line 91
                       self::assertEquals($this->providerId, $providerId); \\ line 92
                       self::assertEquals($this->categoryId, $categoryId); \\ line 93
                       \\ line 94
                       return \array_intersect($map, $idsList); \\ line 95
                   } \\ line 96
               );

    return $repository;
}

It is well formatted by PhpStorm but code sniffer throw next errors for this code part:

 ----------------------------------------------------------------------------------------
 FOUND 5 ERRORS AFFECTING 5 LINES
 ----------------------------------------------------------------------------------------
  91 | ERROR | [x] Line indented incorrectly; expected at least 24 spaces, found 23
  92 | ERROR | [x] Line indented incorrectly; expected at least 28 spaces, found 27
  93 | ERROR | [x] Line indented incorrectly; expected at least 28 spaces, found 27
  95 | ERROR | [x] Line indented incorrectly; expected at least 28 spaces, found 27
  96 | ERROR | [x] Line indented incorrectly; expected 24 spaces, found 23

Firstly I decided that there is something with my IDE formatting settings and manually edited as required - I added 1 space for each marked line. But code sniffer again throw bad formatting error:

 FOUND 1 ERROR AFFECTING 1 LINE
 ---------------------------------------------------------------------------------------------------
  91 | ERROR | [x] Multi-line function call not indented correctly; expected 23 spaces but found 24
 ---------------------------------------------------------------------------------------------------

What should I do in this case?

P.S. As temporary bypass I edited my code part by splitting chained method calls:

protected function getCategoryFeatureRepositoryMock(
    array $map,
    int $getExistingIdsCalls
): CategoryFeatureRepositoryInterface {
    /** @var MockObject|CategoryFeatureRepository $repository */
    $repository = $this->getMockBuilder(CategoryFeatureRepository::class)
                       ->disableOriginalConstructor()
                       ->setMethods(['getCategoryFeatureIds'])
                       ->getMock();

    $repository->expects(self::exactly($getExistingIdsCalls))
               ->method('getCategoryFeatureIds');
    $repository->willReturnCallback( \\ <----------- this is take away code sniffer errors.
        function (string $providerId, int $categoryId, array $idsList) use ($map) {
            self::assertEquals($this->providerId, $providerId);
            self::assertEquals($this->categoryId, $categoryId);

            return \array_intersect($map, $idsList);
        }
    );

    return $repository;
}
@gsherwood
Copy link
Member

This is caused by a conflict between Generic.WhiteSpace.ScopeIndent and PSR2.Methods.FunctionCallSignature. It's best seen on a simple example:

<?php
$repository->foo()
           ->bar(
               function () {
                   return true;
               }
           );

When running the fixer over it using the PSR2 standard, you get these first few loops:

*** START FILE FIXING ***
---START FILE CONTENT---
1|<?php
2|$repository->foo()
3|           ->bar(
4|               function () {
5|                   return true;
6|               }
7|           );
8|
--- END FILE CONTENT ---
	E: [Line 4] Line indented incorrectly; expected at least 16 spaces, found 15 (Generic.WhiteSpace.ScopeIndent.Incorrect)
	PHP_CodeSniffer\Standards\Generic\Sniffs\WhiteSpace\ScopeIndentSniff (line 1345) replaced token 12 (T_WHITESPACE) "···············function" => "················function"
	PHP_CodeSniffer\Standards\Generic\Sniffs\WhiteSpace\ScopeIndentSniff (line 1345) replaced token 20 (T_WHITESPACE) "···················return" => "····················return"
	E: [Line 6] Line indented incorrectly; expected 16 spaces, found 15 (Generic.WhiteSpace.ScopeIndent.IncorrectExact)
	PHP_CodeSniffer\Standards\Generic\Sniffs\WhiteSpace\ScopeIndentSniff (line 1345) replaced token 26 (T_WHITESPACE) "···············}" => "················}"
	* fixed 3 violations, starting loop 2 *
---START FILE CONTENT---
1|<?php
2|$repository->foo()
3|           ->bar(
4|                function () {
5|                    return true;
6|                }
7|           );
8|
--- END FILE CONTENT ---
	E: [Line 4] Multi-line function call not indented correctly; expected 15 spaces but found 16 (PSR2.Methods.FunctionCallSignature.Indent)
	**** PHP_CodeSniffer\Standards\PEAR\Sniffs\Functions\FunctionCallSignatureSniff (line 531) has possible conflict with another sniff on loop 0; caused by the following change ****
	**** replaced token 12 (T_WHITESPACE) "················function" => "···············function" ****
	**** ignoring all changes until next loop ****
	* fixed 0 violations, starting loop 3 *
---START FILE CONTENT---
1|<?php
2|$repository->foo()
3|           ->bar(
4|                function () {
5|                    return true;
6|                }
7|           );
8|
--- END FILE CONTENT ---

The ScopeIndent sniff notices that a code block is not starting at a tab stop (assuming a 4-space indent) so it wants that function keyword (and closing brace) to be moved to a 16-space indent instead of a 15-space indent.

But then the FunctionCallSignature sniff notices that the multi-line function call is now indented 5 spaces from the ->bar( instead of the 4 it is expecting, so it moves it back. Now the sniff conflict.

I think the solution here is to let the ScopeIndent sniff win so that code lines fall at the correct tab-stops. That's what a few other sniffs do as well, and it's the only way I can think of changing things that wont break any of the standards. Plus, it will allow the closure to have other code blocks like IF statements aligned to tab-stops as well without having to make further nested exceptions for them.

This change would mean that your code would look like this:

<?php
protected function getCategoryFeatureRepositoryMock(
    array $map,
    int $getExistingIdsCalls
): CategoryFeatureRepositoryInterface {
    /** @var MockObject|CategoryFeatureRepository $repository */
    $repository = $this->getMockBuilder(CategoryFeatureRepository::class)
                       ->disableOriginalConstructor()
                       ->setMethods(['getCategoryFeatureIds'])
                       ->getMock();

    $repository->expects(self::exactly($getExistingIdsCalls))
               ->method('getCategoryFeatureIds')
               ->willReturnCallback(
                    function (string $providerId, int $categoryId, array $idsList) use ($map) {
                        self::assertEquals($this->providerId, $providerId);
                        self::assertEquals($this->categoryId, $categoryId);

                        return \array_intersect($map, $idsList);
                    }
               );

    return $repository;
}

I have to fix this conflict anyway, but I'm keen to see if you think that fix would work for you.

@gsherwood gsherwood added this to the 3.3.1 milestone Jul 1, 2018
@gsherwood gsherwood added this to Backlog in PHPCS v3 Development via automation Jul 1, 2018
@gsherwood gsherwood moved this from Backlog to In Progress in PHPCS v3 Development Jul 1, 2018
@gsherwood
Copy link
Member

The code for this is actually in the PEAR sniff, which already has some code to deal with functions that don't begin at tab-stops, so I'll need to look at that code and see if it can be improved.

@gsherwood
Copy link
Member

which already has some code to deal with functions that don't begin at tab-stops

Ah yes. It has code to help fix this situation, but the PSR-2 standard specifically blocks it due to issue #1793. This is going to be pretty tricky to fix.

@gsherwood
Copy link
Member

Should have probably also commented (esp if I can't find a fix for this) that the other way I've seen people indent chained method calls works fine with PSR-2 and is probably easier to align given it uses tab stops:

<?php
$repository
    ->expects(self::exactly($getExistingIdsCalls))
    ->method('getCategoryFeatureIds')
    ->willReturnCallback(
        function (string $providerId, int $categoryId, array $idsList) use ($map) {
            self::assertEquals($this->providerId, $providerId);
            self::assertEquals($this->categoryId, $categoryId);

            return \array_intersect($map, $idsList);
        }
    );

I'm not suggesting this is the solution, but it might be worth considering something along those lines if you're already splitting up your chained calls to get PHPCS to stop reporting errors.

@watari
Copy link
Author

watari commented Jul 2, 2018

@gsherwood , thanks for fast reply. I simply forgot about second way of formatting that you described. This formatting satisfy code sniffer.
About proposed possible solution - I think it can work for my case.
Is there anything that I can do, right now, from y side to help you?

@gsherwood
Copy link
Member

Is there anything that I can do, right now, from y side to help you?

I don't think so, but I'll let you know if I need some help testing or someone to run ideas past. I'm still not really sure where to go with this, but I did get distracted with other work today.

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