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.ControlStructures.SwitchDeclaration does not handle if branches with returns #834

Closed
Majkl578 opened this issue Dec 28, 2015 · 4 comments

Comments

@Majkl578
Copy link
Contributor

Consider the following code as an example:

<?php

switch ($foo) {
    case 1:
        if ($bar === 1) {
            return 1;
        } elseif ($bar === 2) {
            return 2;
        } else {
            return 3;
        }
    case 2:
        return 4;
}

Although the case 1 body always returns and thus is not fall-through, the error is generated: There must be a comment when fall-through is intentional in a non-empty case body.

@gsherwood
Copy link
Member

I'm not sure how hard this will be to fix. I might be able to just look for ELSE statements directly inside the case and search for returns in there, but I would also have to recursively check other nested ELSE statements inside as well.

In the meantime, the most obvious suggestion would be to make the return explicit, which also adheres to the common no-else-return rule (or even the no-else and return-early rules) because the ELSE is technically unnecessary:

switch ($foo) {
    case 1:
        if ($bar === 1) {
            return 1;
        } elseif ($bar === 2) {
            return 2;
        }

        return 3;
    case 2:
        return 4;
}

@Majkl578
Copy link
Contributor Author

I understand this is not easy to fix, one has to check all branches recursively. Explicit return is of course a solution, and also more clean. :)

By the way, this problem also applies to throw.

@dereuromark
Copy link
Contributor

@Majkl578 Or continue; ;)
But yeah, one should probably avoid this kind of nesting, better to refactor into a sub-method here.

fabacino added a commit to fabacino/PHP_CodeSniffer that referenced this issue Feb 19, 2017
fabacino added a commit to fabacino/PHP_CodeSniffer that referenced this issue Apr 5, 2017
gsherwood added a commit that referenced this issue Apr 13, 2017
@gsherwood
Copy link
Member

Fixed by PR #1363

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