Page MenuHomePhabricator

Require indentation of CASE statements in PHP code
Closed, ResolvedPublic

Description

Per this discussion, the goal of this task is to identify parts of code that do not have proper indentation for the CASE statement (inside of a SWITCH statement), and add the indentation. Then, make the indentation of CASE mandatory (currently, it is optional).

The proposal is to add a level indentation for each case statement. Thus instead of:

switch ( $var ) {
case 'a':
    // some thing
    break;
default:
    echo "not handled";
}

We would:

switch ( $var ) {
    case 'a':
        // some thing
        break;
    default:
        echo "not handled";
}

Event Timeline

Huji triaged this task as Lowest priority.
Huji added subscribers: Anomie, Esanders.
Huji renamed this task from Require indenting of CASE statements in PHP code to Require indentation of CASE statements in PHP code.Dec 11 2017, 1:22 AM

Change 397216 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/core@master] Require indentation of CASE statements in PHP code

https://gerrit.wikimedia.org/r/397216

Besides the patch I just submitted (for MW core), there are 94 more files that need to be dealt with. I will wait for this one to be merged before working on the files in MW extensions.

hashar added a subscriber: hashar.

That is better tracked via MediaWiki-Codesniffer.

Code style is enforced via PHP_CodeSniffer for which we have custom rules in a composer package: https://packagist.org/packages/mediawiki/mediawiki-codesniffer (build from mediawiki/tools/codesniffer.git in Gerrit).

There are some informations on: https://www.mediawiki.org/wiki/Continuous_integration/PHP_CodeSniffer

And the coding convention / style guide at: https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP

A change such as https://gerrit.wikimedia.org/r/397216 , should have the appropriate rule to enforce the layout.

I am not a fan of that extra level of indentation :D

@hashar: Currently MediaWiki-Codesniffer does not have a rule for this indentation, does it? If so, would you expect me to first submit a patch to that project to introduce the rule? Note that even then, nothing will change with 397216 (as there currently is no exclusion for the rule that doesn't exist anyway). So technically, 397216 should not have "the appropriate rule" (we don't include rules, we exclude them, right?)

I think it's fine to do both in parallel. I think hashar was just pointing out that if there's no codesniffer rule to enforce it, it will easily regress.

I think it's fine to do both in parallel. I think hashar was just pointing out that if there's no codesniffer rule to enforce it, it will easily regress.

Similarly, it is probably easier to add and release the updated ruleset first. Then, in the commit that bumps the mediawiki-codesniffer version, we can run the automatic fixer for any existing violations.

However, before we put too much effort in either of these steps, we should probably first ensure consensus. In particular, https://www.mediawiki.org/wiki/Manual:Coding_conventions has always explicitly stated that both ways are commonly used. Speaking from my own experience, I believe switch cases are most often written without the extra indentation for cases. Would be good to get some data and/or other opinions on this. Note though that counting raw matches in all code is not always a good measure for consensus because not every occurrence was written separately by someone (many matches will originate from the same source, and then copied-pasted as base; and/or adjusted over time to people's own preference or sense of consistency within a certain context).

Personally, I'm fine with either, with a soft preference towards not indenting (based on current habits).

I don't know if we're doing a poll of any kind, but based on Ed's assessment I'd now favour towards indenting (consistency with JS, and because it's a curly brace block of which every other type we already indent). I previously thought the JS side was still inconsistent as well, and I was thinking about the colon-block instead of the curly-block.

Okay, so plan of action is:

  1. Add this feature to CodeSniffer
  2. Modify the current submitted patch to also include a bump in the codesniffer version

Correct?

Change 397216 merged by jenkins-bot:
[mediawiki/core@master] Require indentation of CASE statements in PHP code

https://gerrit.wikimedia.org/r/397216

@Esanders you merged this patch. Is it because someone else added the feature to CodeSniffer and I am not aware of it? If not, I'll try to work on the patch this weekend.

Huji removed Huji as the assignee of this task.Jan 2 2018, 12:28 AM

@Esanders you merged this patch. Is it because someone else added the feature to CodeSniffer and I am not aware of it? If not, I'll try to work on the patch this weekend.

Not that I'm aware of. Looking forward to the CS patch :)

Change 613729 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/core@master] Fix switch/case indentation per [[mw:Coding conventions]]

https://gerrit.wikimedia.org/r/613729

Change 613734 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/tools/codesniffer@master] Add PSR2.ControlStructures.SwitchDeclaration

https://gerrit.wikimedia.org/r/613734

Unfortunately the above rule only has an error/fix for the indentation of the terminating statement (break/throw/return), despite what the documentation says (https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/PSR2/ruleset.xml#L205), but that will still let the user know they need more indentation.

Change 613729 merged by jenkins-bot:
[mediawiki/core@master] Fix switch/case indentation per mediawiki coding conventions

https://gerrit.wikimedia.org/r/613729

Personally, I disagree with making this a strict rule. There is nothing wrong with either of the two switch shown in the task description above. From my point of view, we do have a common code style for one and only one reason: To make code easier to read. And while I agree that the extra indention in the 2nd example does make the code a tiny little bit easier to read, it's not like the 1st example is unreadable.

I don't think I have ever seen somebody arguing about this in a code review. What's the problem with some switch being formatted like this?

I could say the same for how we indent the insides of a for loop or an if clause. The "there is nothing wrong" argument is non-falsifiable.

I don't have any strong feelings about this myself. I am just pointing out that we should not analyze this from the perspective of right and wrong. None of the other indentations are done because they are "right" or "necessary". They are perceived as useful, and I'm not sure if that is backed by hard evidence or not. What is proposed in this task is also based on perceptions of increased readability, not hard evidence.

I could say the same for how we indent the insides of a for loop or an if clause.

No, you can't. Removing the indention from a for or if leaves nothing, while the actual code in each case up there in the first example is still indented.

Change 613734 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Add PSR2.ControlStructures.SwitchDeclaration

https://gerrit.wikimedia.org/r/613734

DannyS712 assigned this task to Esanders.
DannyS712 removed a project: Patch-For-Review.

Reopening so the parties involved can decide if a new patch should be created, or the idea should be dropped altogether.

Is it possible to disable the autofix, but restore the rule, so that fixes will be left to human editors?

Change 637797 had a related patch set uploaded (by Reedy; owner: Esanders):
[mediawiki/core@REL1_35] Fix switch/case indentation per mediawiki coding conventions

https://gerrit.wikimedia.org/r/637797

Change 637797 merged by jenkins-bot:
[mediawiki/core@REL1_35] Fix switch/case indentation per mediawiki coding conventions

https://gerrit.wikimedia.org/r/637797

Umherirrender added a subscriber: Umherirrender.

Reopening so the parties involved can decide if a new patch should be created, or the idea should be dropped altogether.

The upstream issue is fixed. Not sure what now should happen here - see also T266739#6987856

@Umherirrender what about what @DannyS712 said in T182546#6589546?

That should no longer apply after the fix of the sniff.
On the other side I do not know a way to disable the autofix but keep the sniff running.

Change 678357 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/tools/codesniffer@master] Re-apply "Add PSR2.ControlStructures.SwitchDeclaration"

https://gerrit.wikimedia.org/r/678357

Change 678357 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Re-apply "Add PSR2.ControlStructures.SwitchDeclaration"

https://gerrit.wikimedia.org/r/678357

Change 678640 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] Add comment and fix fall-through cases in switch

https://gerrit.wikimedia.org/r/678640

Do we need the following error? I will upload a patch set with the fixes for core to have a look at

20:46:30 FILE: /src/includes/poolcounter/PoolCounterWork.php
20:46:30 ----------------------------------------------------------------------
20:46:30 FOUND 1 ERROR AFFECTING 1 LINE
20:46:30 ----------------------------------------------------------------------
20:46:30  160 | ERROR | There must be a comment when fall-through is
20:46:30      |       | intentional in a non-empty case body
20:46:30      |       | (PSR2.ControlStructures.SwitchDeclaration.TerminatingComment)
20:46:30 ----------------------------------------------------------------------
  • It has problems like phan with non-returning functions (see T240141)
  • A @codeCoverageIgnore comment is okay for the sniff
  • It seems to have problems with try/finally statements (looks like a upstream bug, not sure)
  • Cannot detect unreachable missing default cases of switch
  • phpcs ignore comments treated as content of a case body (looks like a upstream bug)
  • + But it seems it found a bug in FormatMetadata

Do we need the following error? I will upload a patch set with the fixes for core to have a look at

Thanks; it looks a little ugly, but I appreciate it.

20:46:30 FILE: /src/includes/poolcounter/PoolCounterWork.php
20:46:30 ----------------------------------------------------------------------
20:46:30 FOUND 1 ERROR AFFECTING 1 LINE
20:46:30 ----------------------------------------------------------------------
20:46:30  160 | ERROR | There must be a comment when fall-through is
20:46:30      |       | intentional in a non-empty case body
20:46:30      |       | (PSR2.ControlStructures.SwitchDeclaration.TerminatingComment)
20:46:30 ----------------------------------------------------------------------

In general, yes.

  • It has problems like phan with non-returning functions (see T240141)
  • A @codeCoverageIgnore comment is okay for the sniff
  • It seems to have problems with try/finally statements (looks like a upstream bug, not sure)
  • Cannot detect unreachable missing default cases of switch
  • phpcs ignore comments treated as content of a case body (looks like a upstream bug)

Sounds like these are things that might be fixed upstream over time. None of them catastrophic.

  • + But it seems it found a bug in FormatMetadata

Neat.

  • It seems to have problems with try/finally statements (looks like a upstream bug, not sure)

https://github.com/squizlabs/PHP_CodeSniffer/issues/3297

  • phpcs ignore comments treated as content of a case body (looks like a upstream bug)

https://github.com/squizlabs/PHP_CodeSniffer/issues/3296

Change 678640 merged by jenkins-bot:

[mediawiki/core@master] Add comment to fall-through cases in switch

https://gerrit.wikimedia.org/r/678640

Do we need the following error? I will upload a patch set with the fixes for core to have a look at

Thanks; it looks a little ugly, but I appreciate it.

20:46:30 FILE: /src/includes/poolcounter/PoolCounterWork.php
20:46:30 ----------------------------------------------------------------------
20:46:30 FOUND 1 ERROR AFFECTING 1 LINE
20:46:30 ----------------------------------------------------------------------
20:46:30  160 | ERROR | There must be a comment when fall-through is
20:46:30      |       | intentional in a non-empty case body
20:46:30      |       | (PSR2.ControlStructures.SwitchDeclaration.TerminatingComment)
20:46:30 ----------------------------------------------------------------------

In general, yes.

Okay, than this task is resolved