New "Generic.PHP.DiscourageGoto" sniff

[jrfnl]

Usage of goto is often seen as a typical code smell.

Includes docs file and unit tests.

Closes #1662

[michalbundyra]

I like it and I was thinking about the same recently, but maybe we should go a bit more 'crazy' and create a bit more generic sniff - I mean ForbiddenTokens (as we have Generic.PHP.ForbiddenFunctions) and allow customize forbidden tokens.

I've just created PR #1665 to allow 'better' definition arrays in ruleset.
Then we can have the following configuration:

<property name="forbiddenTokens" type="array">
    <element key="T_GOTO" value="Using the 'goto' language construct is discouraged"/>
    <element key="T_GOTO_LABEL" value="Using the goto labels is discouraged"/>
    ...
</property>

So we can define tags and custom error messages.

@jrfnl @gsherwood What do you think?

[gmponos]

If the current PR stays as is and you don't go in the road that @webimpress suggested I believe that you should add an option that you can configure if this will be an error or not. Same as ForbiddenFunction has.

[jrfnl]

If the current PR stays as is and you don't go in the road that @webimpress suggested I believe that you should add an option that you can configure if this will be an error or not. Same as ForbiddenFunction has.

@gmponos Actually, that option should be removed in the ForbiddenFunction sniff as this is already overrulable for every sniff from the ruleset without using custom properties:

<rule ref="Generic.PHP.DiscourageGoto">
	<type>error</type>
</rule>

The above is available in PHPCS 3.x. In PHPCS 2.x this had to be done per error code - see how I did this in the downstream PR to WPCS: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/1150/files#diff-9dbd3b63f5d2589b5eb1701e92893803R134

[jrfnl]

maybe we should go a bit more 'crazy' and create a bit more generic sniff

@webimpress I don't mind making the change, but would like @gsherwood's opinion about it first.

Personally, the only other token which I can think of which should probably be blanket forbidden for most projects is T_EVAL and there is already a dedicated sniff for that - Squiz.PHP.Eval.

Forbidding abstract classes or long arrays using a sniff like this seems to be overshooting. Those things can be auto-fixed and should have a dedicated sniff.

If a certain syntax is to be forbidden in a project because of PHP cross-version compatibility requirements, there is a better way and more comprehensive way to check for that available, i.e. the external PHPCompatibility standard: https://github.com/wimg/PHPCompatibility/

Could you give some examples of tokens you'd like to forbid using a more flexible sniff so I can understand the usecase better ?

[michalbundyra]

@jrfnl I do not have anything specific in mind at this time. I just thought that "generic" sniff could be really generic, and devs could set it up us they want. Then the same sniff could be used for goto and eval (btw I didn't know about the other sniff you mentioned 👍 ).
You have right, that some people can start using it incorrectly, instead of proper sniff which can fix the issue.
Let's see what @gsherwood thinks 😄

[gsherwood]

My feeling is that a specific sniff for gotos is probably a good thing.

I've (fairly) recently written a custom coding standard to ban a lot of PHP syntax and I ended up writing custom sniffs for each one. The main reason is that I wanted to be sure they were working.

Writing a single sniff to ban any combination of tokens sounds easier, but you end up with it either getting more and more complex and people's requirements change, or it's just not very well tested. I tried this with the pattern matching sniff in PHPCS (AbstractPatternSniff) but it didn't allow for really good error messages, and didn't allow for any sort of auto-fixing, so I've moved away from it for the Squiz standard now.

A really really generic token-based sniff might also be okay, but I think this goto sniff should be left as is.

[gsherwood]

Thanks for this sniff.

[jrfnl]

You're welcome 😄