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

Generic.Formatting.MultipleStatementAlignment skipping assignments within closures #1925

Closed
jrfnl opened this issue Mar 6, 2018 · 3 comments
Milestone

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Mar 6, 2018

Following up on #1870, #1848 and open PR #1924.

The fixes in the commits related to the above issues, all skip past closures and arrays.

While working on #1924, I realized however, that as the sniff returns a stackPtr to where the assignment block ended, this means that assignments within closures/anonymous classes will never be examined nor trigger an error.

For arrays, I'm not too concerned as the double arrow alignment is often covered by an array focused sniff anyway.

Code sample demonstrating the issue:

$loggerResult = $util->setLogger(new class {
    public function log($msg)
    {
        $a = $msg;
        $foobar = $msg;
        $bar = $msg;
    }
});
$foo          = 5;

The assignments within the closure are not aligned, but will not trigger an error either.

@jrfnl jrfnl changed the title Generic/MultipleStatementAlignment Generic/MultipleStatementAlignment: assignment alignment within closures Mar 6, 2018
@gsherwood gsherwood added this to the 3.3.0 milestone Mar 7, 2018
@gsherwood gsherwood changed the title Generic/MultipleStatementAlignment: assignment alignment within closures Generic.Formatting.MultipleStatementAlignment skipping assignments within closures Mar 7, 2018
gsherwood added a commit that referenced this issue Mar 7, 2018
…ing assignments within closures

I had to revert the patch to fix #1922 because it wouldn't allow for the sniff to do recursion, which is what it now does when it finds a closure or anon class.
@gsherwood
Copy link
Member

Thanks for picking this up - it was a pain to fix, but I think I got it.

I had to revert the simpler fix for #1922 so that all the tokens would be checked, which allowed the sniff to recurse and check inside anon classes and closures. It's not very efficient, but the tests are all passing.

@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 7, 2018

@gsherwood You're a star!

gsherwood added a commit that referenced this issue Mar 8, 2018
Performance was terrible for files with heaps of closures + the array skipping code meant that closures used inside array values were being skipped as well. Now, instead of looking for closures and anon classes, we check token levels and process assignments inside nested blocks before returning to the main block. Blank lines are skipped inside arrays by keeping track of if we are in one.
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 14, 2018

@gsherwood I think I'm seeing undesired side-effects of the fix.

Given this code:

$abc = 'something';
if ( $something ) {
    // do something
} else {
   // do something else
}
$defghi = 'longer something;'

The sniff now appears to expects the assignment operators for the $abc and the $defghi variables to be aligned.

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

2 participants