PSR2.Methods.FunctionCallSignature.SpaceBeforeCloseBracket bug

[dereuromark]

Describe the bug
The rule breaks with an fail to fix.

Code sample

		$this->belongsTo('Users', [
				'className' => $userClass, 'foreignKey' => 'user_id',
			],
		);

shows

FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------------------
 53 | ERROR | [x] Expected 0 spaces before closing parenthesis; newline found
    |       |     (PSR2.Methods.FunctionCallSignature.SpaceBeforeCloseBracket)
-------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY

Trying to fix it (phpcbf):

FILE                                                                                                        FIXED  REMAINING
----------------------------------------------------------------------------------------------------------------------------
/.../cakephp-ratings/src/Model/Table/RatingsTable.php     FAILED TO FIX
----------------------------------------------------------------------------------------------------------------------------
A TOTAL OF -1 ERRORS WERE FIXED IN 1 FILE

It breaks here hard.

Note: The code uses tabs and is configured as such (<arg name="tab-width" value="4"/>).

Expected behavior
Auto fixing this , or not reporting it to be fixable.

I need to manually fix it to this for sniffer to work again and also fix the other things in this file:

		$this->belongsTo('Users', [
				'className' => $userClass, 'foreignKey' => 'user_id',
		]);

[gsherwood]

The problem here is the trailing comma after the last function argument. The fixer is getting into a loop where Generic.Functions.FunctionCallArgumentSpacing wants a space after that comma, but PSR2.Methods.FunctionCallSignature wants no space before the closing parenthesis.

There is nothing in the standard that forbids a trailing comma after the last argument, but you don't see this very often so it is assumed to never be there.

I could change Generic.Functions.FunctionCallArgumentSpacing to detect that this trailing comma exists and not to enforce spacing rules. You'll end up with this:

		$this->belongsTo('Users', [
				'className' => $userClass, 'foreignKey' => 'user_id',
		],);

But it's technically invalid PSR2 code because the standards says:

...there MUST NOT be a space before the closing parenthesis.

and also says:

In the argument list ...there MUST be one space after each comma.

The other option is to write a new sniff to enforce no trailing comma after the last argument. PSR2 doesn't explicitly state this this is not allowed, but the rule can be inferred from the code samples. This is probably cleaner.

Either way, it's a BC break. I may consider a change for version 3.7.0, so I'll add it to the roadmap.

[dereuromark]

Why not just also removing this trailing comma here? That would make most sense IMO, as the comma is not expected there as you said, so it should be safe to do that given that a closing brace follows after

	$this->belongsTo('Users', [
			'className' => $userClass, 'foreignKey' => 'user_id',
	]);

would be the result which is fine and prevents any such loop - and minimizes side effects.

If a sniff changes those two lines and merged them I think it is expected for this now wrong comma to be removed.

[gsherwood]

Why not just also removing this trailing comma here?

That's exactly what I said in my comment. I'd need a new sniff to do it because the other sniffs are not enforcing trailing commas.

But whatever I do, it's a BC break because PHPCS is working differently than before.

[dereuromark]

Interesting, I would have placed it in the same sniff as this one on its own (without others) is responsible for a safe and complete replace of the two lines into one.
So I do not think it should go into a different one as side effect.

[dereuromark]

But whatever I do, it's a BC break because PHPCS is working differently than before.

I dont think so, it is currently failing (breaking hard as it not further being able to fix the file at all) due to the loop issue.
So it would be a fix that seems without any BC breaks.

I see two options:

• Not (auto)fixing it (doesnt sound too appealing), basically skipping in this specific case.
• Fixing it with removing the side effect (wrong comma), sounds fully BC to me as no one would expect the comma to remain here after the merge. Also only needed in case it is there, which will not be many setups, either.

[gsherwood]

Interesting, I would have placed it in the same sniff as this one on its own (without others)

It's a potentially useful sniff all by itself, so some developers may want to include it in their standard without including all the side effects of the other sniffs that enforce specific formatting.

Fixing it with removing the side effect (wrong comma), sounds fully BC to me

It's not.

no one would expect the comma to remain here after the merge

You and I might not, but don't assume nobody does. If I make the change and someone cares, it's my time that's wasted doing a revert and re-release.

[jrfnl]

Just for context: PSR2 (and PSR12 for that matter) were published before trailing comma's in function calls were allowed (PHP 7.3), so the fact that the PSRs don't allow/forbid those specifically, actually doesn't mean anything. It just wasn't something for which a rule was needed yet.

[dereuromark]

You and I might not, but don't assume nobody does

Sure, but then they wouldnt use any of those sniffs in the first place.
Given the setup I mentioned, given all possible scenarios, the comma in such a merge case must be removed.
There is no point for the sniff to falsey continue to handle those lines IMO.

People either use that sniff as fully working, or they use sth else. But I dont see how keeping the sniff half-working is a fair option in this case.

I silenced the side effect issue for now, which works for us:

    <rule ref="Generic.Functions.FunctionCallArgumentSpacing.NoSpaceAfterComma">
        <severity>0</severity>
    </rule>

[dereuromark]

We managed to add spryker/code-sniffer#313 in order to remove the silencing again.
Maybe such a sniff should be part of those sniffs natively, but for us this works.
We can close then this ticket if you do not want to further follow up here.

[gsherwood]

I'll remove this from the 3.7 milestone as it's no longer a priority, but will leave open to see if other developers encounter this and it becomes a bigger issues.