Make Generic.Functions.OpeningFunctionBraceKernighanRitchie use different error codes for closures

[Daimona]

Is your feature request related to a problem?

The Generic.Functions.OpeningFunctionBraceKernighanRitchie accepts a checkClosures property, false by default, that determines whether the sniff should also check closures. Because closures, unlike functions, support an additional use clause, sometimes formatting and indentation can be trickier to get right. For example, in:

$closure = function ( string $firstArgument, string|bool|null $secondArgument = null )
    use ( $firstUsedVariable, $secondUsedVariable, $thirdUsedVariable, $fourthUsedVariable )
    {
        echo 'Function body starts here';
};

Here, changing the brace style allows better separation between the parameter and use list and the function body.

Of course we could argue on the best™ way to indent the use and the rest, and perhaps there's a way that works better than mine. However, that's beyond the point here. Instead:

Describe the solution you'd like

It would be nice if the sniff used different error codes for closures. This way, individual coding standards would have more flexibility, and potentially disable problematic checks for closures. In the example above, one would disable Generic.Functions.OpeningFunctionBraceKernighanRitchie.BraceOnNewLine to allow that style while still benefitting from the other checks. However, this currently uses the same code for functions and closures, so it's not possible to do this selectively for closures.

For example, the sniff could use:

• Generic.Functions.OpeningFunctionBraceKernighanRitchie.X for functions
• Generic.Functions.OpeningFunctionBraceKernighanRitchie.ClosureX for closures

Additional context (optional)

Strictly speaking, the sniff also has a checkFunctions option to toggle functions checks, enabled by default. However, assuming that functions are more common than closures, and considering the default states of the two toggles, as well as reducing backward compatibility breaks, I think it makes sense to keep using unprefixed error codes for functions, and add a prefix for closures only.

• I have read the Contribution Guidelines and this is not a support question.
• I intend to create a pull request to implement this feature.

NB, I'll first wait to hear whether this proposal would be accepted before working on an implementation.

[jrfnl]

@Daimona I'm not keen on changing the error codes for this sniff (or even these sniffs as there is a sister-sniff), as it is quite widely used and might break more than intended.

However, this codebase already has a sniff which differentiates the brace style between named functions and closures. That sniff may not be what you are looking for, as it does more than just the brace style, but the logic used in that sniff might give you some inspiration on how to solve the underlying problem you are trying to solve ?

PHP_CodeSniffer/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php

Lines 248 to 252 in c4adbbc

	if ($tokens[$stackPtr]['code'] === T_CLOSURE) {
	$sniff = new OpeningFunctionBraceKernighanRitchieSniff();
	} else {
	$sniff = new OpeningFunctionBraceBsdAllmanSniff();
	}

[jrfnl]

Oh and just for the record - if that sniff would be written for PHP_CodeSniffer itself nowadays, then yes, the properties would not exist and there would be error code differentiation instead. I totally agree, that's how it should be.

These are however some of the oldest sniffs in the codebase, so we are unfortunately left with the legacy of the past.

[Daimona]

@Daimona I'm not keen on changing the error codes for this sniff (or even these sniffs as there is a sister-sniff), as it is quite widely used and might break more than intended.

Hmmmm yeah, I think that makes sense. Could we maybe put the error code differentiation behind a new config option that is default false? This is also not how it should be, but with legacy code sometimes there isn't much choice.

However, this codebase already has a sniff which differentiates the brace style between named functions and closures. That sniff may not be what you are looking for, as it does more than just the brace style, but the logic used in that sniff might give you some inspiration on how to solve the underlying problem you are trying to solve ?

Thanks for the pointer! I am separately working on enabling that sniff in our MediaWiki standard. However, I'm not sure if it would help much here: because OpeningFunctionBraceKernighanRitchieSniff provides no way to selectively turn off only parts of it, I think I would still have to duplicate a decent amount of code to get that working.

[Daimona]

Or maybe, how about:

• deprecating both checkClosures and checkFunctions
• changing the error code for closures UNLESS checkClosures is set to true

That way:

• Individual issue categories can be disabled by suppressing the relevant issue codes
• Everyone using checkClosures will see a deprecation warning, but will still get the previous issue code. This way, they can migrate from checkClosures to the new issue code at once
• Everyone not using checkClosures will start getting new warnings about closures, which they can disable while upgrading by excluding the issue codes from their config
• If someone is unsetting checkFunctions (possibly quite rare?), they will start getting warnings about functions that can also be disabled in the config

The advantage being that the deprecated properties will eventually be dropped, and the sniffs will end up having different error codes for functions and closures, no properties, and with minimal disruption.