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

PSR2 standard can't auto fix multi-line function call inside a string concat statement #2506

Closed
stdex opened this issue May 17, 2019 · 5 comments
Milestone

Comments

@stdex
Copy link

stdex commented May 17, 2019

Hello.

PHP_CodeSniffer version 3.4.2 (stable)

Call:
phpcbf.bat --report=full -s -vv --standard=psr2

Problem with code:

<?php

class C
{

    public function m()
    {
        $t = '
	' . (empty(true) ? '
	' . f(
                    '1',
                    '2',
                    '3',
                    '4'
                ) . '
	' : '');
    }

}
=> Fixing file: 4/7 violations remaining [made 50 passes]... 	* fixed 4 violations, starting loop 51 *
	*** Reached maximum number of loops with 4 violations left unfixed ***
ERROR in 1.62 secs

A TOTAL OF 3 ERRORS WERE FIXED IN 1 FILE
----------------------------------------------------------------------
PHPCBF FAILED TO FIX 1 FILE
----------------------------------------------------------------------

Problem with code:

<?php

class C
{

    public function m()
    {
        $a = [];
        $t =
            "SELECT * FROM t
WHERE f IN(" . implode(
                ",",
                $a
            ) . ")";
    }
}
=> Fixing file: 3/4 violations remaining [made 50 passes]... 	* fixed 3 violations, starting loop 51 *
	*** Reached maximum number of loops with 3 violations left unfixed ***
ERROR in 1.29 secs

A TOTAL OF 1 ERROR WERE FIXED IN 1 FILE
----------------------------------------------------------------------
PHPCBF FAILED TO FIX 1 FILE
----------------------------------------------------------------------
@jrfnl
Copy link
Contributor

jrfnl commented May 18, 2019

This is a fixer conflict between the Generic.WhiteSpace.ScopeIndent and the PEAR.Functions.FunctionCallSignature sniffs.

The problem (in both cases) is caused by the function call being concatenated onto a text string spanning multiple lines.

Looking at the code of the first example, IMO the correct fix would be to indent the code like below where the indent of the function call parameters is based on the indent of the $t = + 4 spaces, which would come down to 12 spaces.

Based on an initial check, it looks like both sniffs get it wrong.

The PEAR.Functions.FunctionCallSignature tries to fix to 4 spaces, i.e. start position of the line containing the function call (= 0 indent as it is the second line in a multi-line text string) + 4 spaces, while the Generic.WhiteSpace.ScopeIndent tries to set the indent to 8 spaces, i.e. the same as the start position of the $t.

<?php

class C
{

    public function m()
    {
        $t = '
    ' . (empty(true) ? '
    ' . f(
            '1',
            '2',
            '3',
            '4'
        ) . '
    ' : '');
    }
}

@stdex
Copy link
Author

stdex commented May 19, 2019

Does this mean that I have to manually resolve such conflicts?

@jrfnl
Copy link
Contributor

jrfnl commented May 19, 2019

@stdex No, sorry, I wasn't clear. My post was just to add additional information to the issue in hopes that a solution can be found.

@gsherwood gsherwood added this to the 3.5.1 milestone Oct 11, 2019
@gsherwood gsherwood changed the title Reached maximum number of loops with 4 violations left unfixed PSR2 standard can't auto fix multi-line function call inside a string concat statement Oct 11, 2019
gsherwood added a commit that referenced this issue Oct 11, 2019
@gsherwood
Copy link
Member

I've added a fix for the first case.

The PSR2 standard will now fix this code:

<?php

class C
{

    public function m()
    {
        $t = '
        ' . (empty(true) ? '
        ' . f(
                        '1',
                        '2',
                        '3',
                        '4'
                    ) . '
        ' : '');
    }

}

So that it looks like this:

<?php

class C
{

    public function m()
    {
        $t = '
        ' . (empty(true) ? '
        ' . f(
            '1',
            '2',
            '3',
            '4'
        ) . '
        ' : '');
    }
}

The second case is something slightly different and needs to be fixed separately.

gsherwood added a commit that referenced this issue Oct 11, 2019
Slightly different fix for the second code snippet reported there. Uses startOfStatement instead of line unless it detects method chaining.
@gsherwood
Copy link
Member

gsherwood commented Oct 11, 2019

I've added another fix into the same sniff for the second case.

The PSR2 standard will now fix this code:

<?php

class C
{

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

So that it looks like this:

<?php

class C
{

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

Thanks for the bug report.

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

3 participants