Add documentation for WordPress.PHP.NoSilencedErrors#2495
Conversation
rodrigoprimo
left a comment
rodrigoprimo
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @gogdzl! I left some inline comments. As always, as I'm not a maintainer, those are just suggestions from someone who created sniff documentation in the past.
| <?xml version="1.0"?> | ||
| <documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd" | ||
| title="Discourage the use of the PHP error silencing operator" |
There was a problem hiding this comment.
| title="Discourage the use of the PHP error silencing operator" | |
| title="No Silenced Errors" |
AFAIK, the title should match the name of the sniff (with some rare exceptions).
| <code_comparison> | ||
| <code title="Valid: Using proper error checking when calling functions not in the allowed list."> | ||
| <![CDATA[ | ||
| $conn_id = ftp_connect( $ftp_server ); |
There was a problem hiding this comment.
| $conn_id = ftp_connect( $ftp_server ); | |
| $conn_id = <em></em>ftp_connect( $ftp_server ); |
| <![CDATA[ | ||
| Silencing errors is strongly discouraged. Use proper error checking instead. | ||
|
|
||
| Using the error operator with certain functions is allowed because no amount of error checking can fully prevent PHP from throwing errors when these functions are executed. The functions where this is permitted include, but are not limited to: |
There was a problem hiding this comment.
I agree that we should not include all the allowed functions, as the list is very long. But I'm unsure about adding just a few. I don't really know what criteria would be good for deciding which functions to include. What criteria did you use?
I'd be more inclined to just mention that some functions are allowed and point users to search for the $allowedFunctionsList property in https://github.com/WordPress/WordPress-Coding-Standards/blob/b0fc0c082a07da248fe767c10486a7fa27cc7c57/WordPress/Sniffs/PHP/NoSilencedErrorsSniff.php. That being said, I was not able to find other sniff documentation doing what I'm suggesting both in the WPCS and the PHPCS repository. So, I advise you to wait to hear what the maintainers think is the best approach here.
In the WPCS repository, there is one sniff documentation that uses an external link in a different context:
| ]]> | ||
| </standard> | ||
| <code_comparison> | ||
| <code title="Valid: Using proper error checking when calling functions not in the allowed list."> |
There was a problem hiding this comment.
To me, the valid message should be about what the sniff considers valid. The sniff doesn't check for proper error checking. Instead, it checks whether the @ operator is present.
For the same reason, I'm unsure if error handling should be included in the code sample. Maybe checking the documentation of other sniffs might help you decide which approach you want to take? Also, feel free to wait for the input of one of the maintainers regarding this.
|
@gogdzl, I was just wondering if you'll have a chance to finish this off in the near future. It would be great if this PR could be included in the next WPCS release. If you haven't got time or lost interest, please let us know and we'll see if we can find someone to take over. Thanks! |
|
@gogdzl, please let us know within a week if you are still interested in finishing this PR. If we don't hear back from you, we will presume you don't have time, and we will see if we can find someone else to take over and finish it. Thanks for your work so far! |
3 participants
Related to #1722