Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent Squiz.Commenting.FunctionCommentThrowTag.Missing failing for exceptions caught in method #3685

Open
josemv92 opened this issue Oct 6, 2022 · 6 comments

Comments

@josemv92
Copy link

josemv92 commented Oct 6, 2022

Describe the bug
Using the Squiz.Commenting.FunctionCommentThrowTag.Missing to warn when a method is throwing an exception not mentioned in the PHPdoc is requiring users to add the @throw tag even when the exception is caught in the same scope of the method.

Code sample

class MyClass {

    /**
     * Test method.
     *
     * @return void
     */
    public function myMethod() {
        try {
            throw new Exception("Error");
        } catch (Exception $e) {
            return false;
        }
    }
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="Custom">
    <rule ref="Squiz.Commenting.FunctionCommentThrowTag.Missing" />
</ruleset>

To reproduce
Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above.
  2. Run phpcs test.php
  3. See error message displayed
FILE: /test.php
--------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------
 9 | ERROR | Missing @throws tag in function comment
   |       | (Squiz.Commenting.FunctionCommentThrowTag.Missing)
--------------------------------------------------------------------------------------------------------

Expected behavior
The rule shouldn't trigger for users to set the @throws tag on the method PHPdoc since in the end the method won't be throwing this exception outside of its scope.

Versions (please complete the following information):

  • OS:Ubuntu 20 (but it might be happening in any).
  • PHP: 7.4+
  • PHPCS: 3.7
  • Standard: Squiz

Additional context
This is conflicting with other inspections applied by some IDEs (like PHPStorm) which indicates the @throws tag is not necessary.

@ArneGockeln
Copy link

We are facing the same behaviour in PhpStorm 2022.2.3 on macOS Monterey 12.6 and also in the automated phpcs test pipelines on azure cloud.

@Daniel217D
Copy link

Any updates on this?

dmsnell added a commit to WordPress/gutenberg that referenced this issue Jul 28, 2023
See squizlabs/PHP_CodeSniffer#3685

PHP Code Sniffer mistakes the following code, thinking that it
throws an exception even though the exception is caught in place.

```php
try {
	throw new WP_HTML_Unsupported_Exception( 'The outside never sees me.' );
} catch ( WP_HTML_Unsupported_Excepted $e ) {
	return false;
}
```

This patch adds an exclude for the HTML Processor where PHPCS is confused.
Until that bug is fixed this is a pragmatic solution to avoiding the need
to change actual code around a bug in a linting tool. When that bug is
fixed and it no longer gets confused this exclusion should be removed.
dmsnell added a commit to WordPress/gutenberg that referenced this issue Aug 1, 2023
See squizlabs/PHP_CodeSniffer#3685

PHP Code Sniffer mistakes the following code, thinking that it
throws an exception even though the exception is caught in place.

```php
try {
	throw new WP_HTML_Unsupported_Exception( 'The outside never sees me.' );
} catch ( WP_HTML_Unsupported_Excepted $e ) {
	return false;
}
```

This patch adds an exclude for the HTML Processor where PHPCS is confused.
Until that bug is fixed this is a pragmatic solution to avoiding the need
to change actual code around a bug in a linting tool. When that bug is
fixed and it no longer gets confused this exclusion should be removed.
dmsnell added a commit to WordPress/gutenberg that referenced this issue Aug 8, 2023
See squizlabs/PHP_CodeSniffer#3685

PHP Code Sniffer mistakes the following code, thinking that it
throws an exception even though the exception is caught in place.

```php
try {
	throw new WP_HTML_Unsupported_Exception( 'The outside never sees me.' );
} catch ( WP_HTML_Unsupported_Excepted $e ) {
	return false;
}
```

This patch adds an exclude for the HTML Processor where PHPCS is confused.
Until that bug is fixed this is a pragmatic solution to avoiding the need
to change actual code around a bug in a linting tool. When that bug is
fixed and it no longer gets confused this exclusion should be removed.
@celorodovalho
Copy link

Any updates on this issue?

@jrfnl
Copy link
Contributor

jrfnl commented Aug 16, 2023

@celorodovalho As far as I can see, nobody has submitted a PR to fix this, so no.

@celorodovalho
Copy link

@jrfnl I could submit it by myself if I have time to learn how it works.

@jrfnl
Copy link
Contributor

jrfnl commented Nov 18, 2024

@celorodovalho If you do, please make sure you submit the PR to the new repo. This repo is abandoned and https://github.com/PHPCSStandards/PHP_CodeSniffer is its successor. See: #3932

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants