Php_codesniffer: [3.2.3 BC break] EmptyStatement.DetectedCATCH renamed to EmptyStatement.DetectedCatch, breaking excludes

[Majkl578]

The changes in #1890 introduced a BC break in 3.2.3 bugfix release.

The following code did not produce any error in 3.2.2:

<?php


try {
    doSomething();
} catch (SomeException $e) {
    // ignore
}

It is now producing the following error in 3.2.3:

FILE: test.php
------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
 6 | ERROR | Empty CATCH statement detected (Generic.CodeAnalysis.EmptyStatement.DetectedCatch)
------------------------------------------------------------------------------------------------

[photodude]

I think you will find the issue is related to the Squiz.Commenting.EmptyCatchComment sniff
Or Generic.CodeAnalysis.EmptyStatement.DetectedCatch and underlying tokens.
I find it Unlikely that 1890 is related to the issues you are experiencing.

[Majkl578]

As you can see from the error message, the sniff error code is Generic.CodeAnalysis.EmptyStatement.DetectedCatch.

[Majkl578]

Ok, I found the culprit: it is caused by the change to the error code.
3.2.2: Generic.CodeAnalysis.EmptyStatement.DetectedCATCH
3.2.3: Generic.CodeAnalysis.EmptyStatement.DetectedCatch

We have an exclude for this specific error which is no longer working.
Rewording the issue, but keeping the BC break label.

[KacerCZ]

This BC break was introduced by commit https://github.com/squizlabs/PHP_CodeSniffer/commit/eefb31c68e4c9e4c390d08859163ca7e587ca46d

[photodude]

If you look at the unit test change you can see a finally block added after the try/catch. I suspect if you add just a try/catch to the unit test it should confirm the issue. (I.e. There probably should be a try/catch and a try/catch/finally in the unit test)

Should be easier to pin down what part of the change is the issue that way.

[jrfnl]

Should be easier to pin down what part of the change is the issue that way.

The change which is the issue is in line 90/91 of EmptyStatementSniff which now does a ucfirst(strtolower($name)) for the errorcode.

[gsherwood]

I have absolutely no idea why I made that change. It looks like a mistake where I was trying to change the message but didn't realise it was the error code.

I apologise for that. I'll update the release notes on Github to reflect this change

I think those exclude rules would benefit from being case insensitive as well.

[photodude]

@gsherwood unfortunately that change also made it over to 2.9.x with the backport in #1892

[gauthierm]

Should I update my ruleset to use the new EmptyStatement.DetectedCatch or should I wait for a new patch release of phpcs?

[gsherwood]

@gauthierm That change is now in the changelog, so I wont revert it. Please update your ruleset. Again, I apologise for that change.

[photodude]

@gsherwood I see the fix in #1914 for the Squiz ruleset Squiz.PHP.CommentedOutCode but no fix for the Generic.CodeAnalysis.EmptyStatement.DetectedCatch error noted here.

[gsherwood]

but no fix for the Generic.CodeAnalysis.EmptyStatement.DetectedCatch error noted here

The error isn't new. The error code changed. I have added that note to the changelog so it is now official and people can use the updated codes. I have not had time to try and make the rules case insensitive.

[gsherwood]

I took a look at making sniff codes case insensitive. It's very possible, but I'm really worried about missing something and breaking rulesets because I had to change a lot of places. I've created a new issue (#1955) for this but wont implement it until version 4.