Spacing of : for return declaration when including an argument

[GaryJones]

Related to #764.

When a function has a return declaration AND at least one parameter, then a warning is thrown unless there is a space before the :. My understanding is that WPCS doesn't define whether a space should be included here or not. The rest of the PHP world doesn't include a space, and WPCS ONLY flags the lack of space when a parameter is present.

Example:

<?php
/**
 * Foo
 *
 * @package   Test
 */

/**
 * Foo1
 *
 * @return string
 */
function prefix_foo1(): string {
	return '';
}

/**
 * Foo2
 *
 * @param string $input String.
 * @return string String
 */
function prefix_foo2( $input ): string {
	return '';
}

/**
 * Foo3
 *
 * @param string $input String.
 * @return string String
 */
function prefix_foo3( string $input ): string {
	return '';
}


/**
 * Foo4
 *
 * @param string $input String.
 * @return string String
 */
function prefix_foo4( string $input ) : string {
	return '';
}

/**
 * Foo5
 *
 * @param string $input String.
 * @return string String
 */
function prefix_foo5( $input ) : string {
	return '';
}

• Foo1: No argument, no warning.
• Foo2: Argument, warning.
• Foo3: Argument with type declaration, warning.
• Foo4: Argument, space before :, no warning.
• Foo5: Argument with type declaration, space before :, no warning.

Here are the warnings:

 23 | ERROR | [x] Space between opening control structure and closing parenthesis is required (WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterCloseParenthesis)
 33 | ERROR | [x] Space between opening control structure and closing parenthesis is required (WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterCloseParenthesis)

The general nature of the warning means it can't easily be excluded - either phpcs:disable and phpcs:enable lines need to be added, or the whole check needs to be severity 0, which then means other instances of this whitespace issue would not be reported.

[GaryJones]

For projects which use return types a lot, this can be annoying. Here's SatisPress for instance:

Adding the space before the : for some functions (with params), and not others would introduce inconsistency just for the sake of satisfying PHPCS.

[GaryJones]

The fixer for this also seems slightly off. With test lines of:

function returntype1(): string {} // Ok.
function returntype2( $input ): string {} // Ok.
function returntype3( $input ) : string {} // Ok.
function returntype4( string $input ): string {} // Ok.
function returntype5( string $input ) : string {} // Ok.

... the .fixed currently needs to be:

function returntype1(): string {} // Ok.
function returntype2( $input ): string                           {} // Ok.
function returntype3( $input ) : string {} // Ok.
function returntype4( string $input ): string                           {} // Ok.
function returntype5( string $input ) : string {} // Ok.

for it just to complain about the violations and not about the output as well.

[GaryJones]

The relevant part seems to be here:

https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/ecaa437623fddb11ed81b181a315b2f2b44ca803/WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php#L346-L355

Adding:

&& T_COLON !== $this->tokens[ ( $parenthesisCloser + 1 ) ]['code']

to the condition seems to work (including for the fixer), but I suspect that this should also be accompanied by a related condition that this is part of a function definition, and I'm not sure of the best way to do that.

[jrfnl]

@GaryJones I think this should be included in the discussion in #1101 instead of being a separate issue.
I also think it would be useful to discuss spacing around the nullable type operator.

I have a local WIP branch to address #1101, but want to get 1.0.0 out of the door before getting back to it.

[withinboredom]

fwiw, from core's best practices handbook

Similarly, there should be no space before the colon on return type declarations.

[GaryJones]

@withinboredom Ah, thank you - I think that was one of the newer additions which I forgot had been added.

@jrfnl Would it be possible to get this bug fixed, before 1.0.0? Is my code above sufficient?

[jrfnl]

I can't remember this handbook change being made ? Who changed this when ? And why wasn't an issue opened at the time ? (or is it so long ago, I don't even remember having seen it before ?)

Oh dear... a search yielded issue #547 which I believe this issue should be considered a duplicate of.

@jrfnl Would it be possible to get this bug fixed, before 1.0.0? Is my code above sufficient?

I haven't got time over the next few days to work on the sniffs for #1101 and I don't think this should be haphazardly fixed in a sniff in which it doesn't belong in the first place.

The only quick "fix" I can think of short term, is removing the functions related tokens from that sniff, but I'm not keen to do that while there is no replacement yet.

[ntwb]

It was added to the handbook per the request here in issue #547 (and companion open PR #1084).

[GaryJones]

I don't think this should be haphazardly fixed in a sniff in which it doesn't belong in the first place.

I'd disagree. Any bug fix, even if not ideal for the long-term, would be better for the 1.0.0 release, which is arguably our first LTS-like release.

Sure, #1101 may well remove it, but in the meantime, the quick fix is preferable IMO. My question was whether the above code looks sufficient, or whether there are test cases I can add that I can work with to narrow down when the : is excluded.

[jrfnl]

As I said above, I won't have time to look at this before the release. If someone else sends in a PR, I'll try and review it in time to include it.