Page MenuHomePhabricator
Create Task
Maniphest T182546

Require indentation of CASE statements in PHP code
Closed, ResolvedPublic
Actions

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";
}

Details

ProjectBranchLines +/-Subject
mediawiki/coremaster+12 -2Add comment to fall-through cases in switch
mediawiki/tools/codesniffermaster+1 -0Re-apply "Add PSR2.ControlStructures.SwitchDeclaration"
mediawiki/coreREL1_35+78 -79Fix switch/case indentation per mediawiki coding conventions
mediawiki/tools/codesniffermaster+1 -0Add PSR2.ControlStructures.SwitchDeclaration
mediawiki/coremaster+78 -79Fix switch/case indentation per mediawiki coding conventions
mediawiki/coremaster+1 K -1 KRequire indentation of CASE statements in PHP code
Customize query in gerrit

Related Objects

Event Timeline

Huji created this task.Dec 11 2017, 1:00 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 11 2017, 1:00 AM
Huji claimed this task.Dec 11 2017, 1:06 AM
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
gerritbot added a subscriber: gerritbot.Dec 11 2017, 3:08 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

gerritbot added a project: Patch-For-Review.Dec 11 2017, 3:08 AM
Huji added a comment.Dec 11 2017, 3:34 AM

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 updated the task description. (Show Details)Dec 11 2017, 9:15 AM
hashar edited projects, added MediaWiki-Codesniffer; removed Patch-For-Review, Continuous-Integration-Infrastructure.Dec 11 2017, 9:17 AM
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

Jdforrester-WMF added a subscriber: Jdforrester-WMF.Dec 11 2017, 6:14 PM
Huji added a comment.Dec 12 2017, 1:09 AM

@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?)

Legoktm added a subscriber: Legoktm.Dec 12 2017, 1:33 AM

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.

Krinkle added a subscriber: Krinkle.Dec 12 2017, 2:36 AM
In T182546#3829896, @Legoktm wrote:

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).

Legoktm added a comment.Dec 12 2017, 2:37 AM

Data was provided in the linked discussion: https://www.mediawiki.org/w/index.php?title=Topic:U39uajst25fusqgk

Krinkle added a comment.Dec 12 2017, 2:39 AM

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.

Huji added a comment.Dec 12 2017, 12:52 PM

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?

Esanders added a comment.Dec 12 2017, 4:24 PM

Sounds good to me.

Ricordisamoa added a subscriber: Ricordisamoa.Dec 17 2017, 6:35 PM
gerritbot added a comment.Dec 19 2017, 12:22 PM

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

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

Lucas_Werkmeister_WMDE added a subscriber: Lucas_Werkmeister_WMDE.Dec 19 2017, 12:55 PM
Huji added a comment.Dec 19 2017, 1:58 PM

@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 added a comment.Jan 26 2018, 6:40 PM
In T182546#3847948, @Huji wrote:

@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 :)

Krinkle moved this task from Untriaged to Proposed rule changes on the MediaWiki-Codesniffer board.Jul 16 2020, 10:34 PM
gerritbot added a comment.Jul 17 2020, 9:38 PM

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

gerritbot added a project: Patch-For-Review.Jul 17 2020, 9:38 PM
gerritbot added a comment.Jul 17 2020, 9:55 PM

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

Esanders added a comment.Jul 17 2020, 9:59 PM

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.

gerritbot added a comment.Jul 18 2020, 9:42 AM

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

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.1; 2020-07-21).Jul 18 2020, 10:00 AM
thiemowmde added a subscriber: thiemowmde.Jul 20 2020, 11:00 AM

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?

hashar removed a subscriber: hashar.Jul 20 2020, 1:04 PM
Huji added a comment.Jul 20 2020, 1:25 PM

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.

thiemowmde added a comment.Jul 20 2020, 3:45 PM

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.

gerritbot added a comment.Aug 29 2020, 12:06 AM

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

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

DannyS712 closed this task as Resolved.Sep 13 2020, 11:43 PM
DannyS712 assigned this task to Esanders.
DannyS712 removed a project: Patch-For-Review.
DannyS712 moved this task from Proposed rule changes to Accepted rule changes on the MediaWiki-Codesniffer board.
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:43 PM
Jdforrester-WMF added a comment.Oct 29 2020, 4:16 PM

Probably going to have to revert this. T266739: Autofix for switch indentation adds indentation to the wrong places.

Huji reopened this task as Open.Oct 29 2020, 5:21 PM

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

DannyS712 added a subscriber: DannyS712.Oct 29 2020, 6:22 PM

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

gerritbot added a comment.Nov 2 2020, 1:31 AM

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

gerritbot added a project: Patch-For-Review.Nov 2 2020, 1:31 AM
gerritbot added a comment.Nov 2 2020, 1:47 AM

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

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

ReleaseTaggerBot added a project: MW-1.35-notes.Nov 2 2020, 2:00 AM
Jdforrester-WMF removed Esanders as the assignee of this task.Jan 1 2021, 7:32 PM
Krinkle removed a subscriber: Krinkle.Jan 5 2021, 1:12 AM
Umherirrender mentioned this in T266739: Autofix for switch indentation adds indentation to the wrong places.Apr 9 2021, 4:51 PM
Umherirrender added a subscriber: Umherirrender.
In T182546#6589245, @Huji wrote:

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

Huji added a comment.Apr 10 2021, 12:08 AM

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

Umherirrender added a comment.Apr 10 2021, 6:43 PM
In T182546#6988212, @Huji wrote:

@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.

Huji added a comment.Apr 11 2021, 1:06 AM

So what is the next step? Revert the revert in https://gerrit.wikimedia.org/r/c/mediawiki/tools/codesniffer/+/637062 ?

gerritbot added a comment.Apr 11 2021, 1:08 AM

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

gerritbot added a comment.Apr 11 2021, 8:34 PM

Change 678357 merged by jenkins-bot:

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

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

gerritbot added a comment.Apr 12 2021, 9:22 PM

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

Umherirrender added a comment.Apr 12 2021, 9:23 PM

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
Jdforrester-WMF added a comment.Apr 12 2021, 9:28 PM
In T182546#6992361, @Umherirrender wrote:

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.

Umherirrender added a comment.Apr 12 2021, 10:08 PM
In T182546#6992361, @Umherirrender wrote:
  • − 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

gerritbot added a comment.Apr 15 2021, 6:45 PM

Change 678640 merged by jenkins-bot:

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

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

ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.3; 2021-04-27).Apr 15 2021, 7:00 PM
Umherirrender closed this task as Resolved.Apr 15 2021, 7:28 PM
In T182546#6992366, @Jdforrester-WMF wrote:
In T182546#6992361, @Umherirrender wrote:

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

Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL