Regression in 2.9.0: False positive using ShortVariable for $e in catch() within a foreach()

[Doqnach]

• PHPMD version: 2.9.0-2.9.1
• PHP Version: 7.3.19
• Installation type: composer
• Operating System / Distribution & Version: MacOS 10.15.6 / Alpine 3.11

Current Behavior

False positive for ShortVariable, regression since earlier versions:

$ src/vendor/bin/phpmd src/src/proof.php text phpmd.xml
src/src/proof.php:18	Avoid variables with short names like $e. Configured minimum length is 2.

Expected Behavior

No errors.

Steps To Reproduce:

Proof of concept: https://github.com/Doqnach/phpmd-shortvariable-error

The problem seems to be related to PR #807 (2.9.0). Works like expected in 2.8.2.

Checks before submitting

• Be sure that there isn't already an issue about this. See: Issues list
• Be sure that there isn't already a pull request about this. See: Pull requests
• I have added every step to reproduce the bug.
• If possible I added relevant code examples.
• This issue is about 1 bug and nothing more.
• The issue has a descriptive title. For example: "JSON rendering failed on Windows for filenames with space".

[kylekatarnls]

Hello,

Thanks for raising this.

I can't see why the expected behavior would be "no errors", for me the bug in proof.php is that PHPMD does not rise an error line 31.

[kylekatarnls]

OK, Indeed, the code is supposed to filter short variable created via catch. I personally recommend to consider catch () like any other statement or function call and give meaningful names to your $exception variable. Bu I agree to not change this for now and try to restore the catch-detection in foreach.

[Doqnach]

@kylekatarnls I kind of disagree with you on that, primarily because I think that the type/classname of the caught Exception should already describe the intention of the variable enough. At most people would start using $ex, $exception or similar, since doing catch (InvalidArgumentException $invalidArgumentException) is more verbose than need be.

Using $e in a try { ... } catch() { ... } is about as common as using $i in a for(), which would also be my guess as to why that exception was made in the code to begin with?

See https://github.com/phpmd/phpmd/blob/2.8.2/src/main/php/PHPMD/Rule/Naming/ShortVariable.php#L173

[ravage84]

I personally recommend to consider catch () like any other statement or function call and give meaningful names to your $exception variable. Bu I agree to not change this for now and try to restore the catch-detection in foreach.

I agree on both statements. Using $e for exception has been an unhealthy habit for too long in PHP. Same for $i.

[ravage84]

Thank you, @Doqnach for the detailed issue report.