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.

Sign up for GitHub

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

Jump to bottom

Add documentation for WordPress.PHP.NoSilencedErrors #2495

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add documentation for WordPress.PHP.NoSilencedErrors #2495

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions WordPress/Docs/PHP/NoSilencedErrorsStandard.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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).

>
<standard>
<![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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

WordPress-Coding-Standards/WordPress/Docs/NamingConventions/PrefixAllGlobalsStandard.xml

Lines 6 to 16 in cdb29b5

<standard>
<![CDATA[
All globals terms must be prefixed with a theme/plugin specific term. Global terms include Namespace names, Class/Interface/Trait/Enum names (when not namespaced), Functions (when not namespaced or in an OO structure), Constants/Variable names declared in the global namespace, and Hook names.
A prefix must be distinct and unique to the plugin/theme, in order to prevent potential conflicts with other plugins/themes and with WordPress itself.
The prefix used for a plugin/theme may be chosen by the developers and should be defined in a custom PHPCS ruleset to allow for this sniff to verify that the prefix is consistently used.
Prefixes will be treated in a case-insensitive manner.
https://github.com/WordPress/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#naming-conventions-prefix-everything-in-the-global-namespace
]]>
</standard>


- `file_exists()`
- `file_get_contents()`
- `filesize()`
- `filetype()`
- `fopen()`
- `ftp_login()`
- `ftp_rename()`
]]>
</standard>
<code_comparison>
<code title="Valid: Using proper error checking when calling functions not in the allowed list.">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<![CDATA[
$conn_id = ftp_connect( $ftp_server );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$conn_id = ftp_connect( $ftp_server );
$conn_id = <em></em>ftp_connect( $ftp_server );

if ( ! $conn_id ) {
// Handle it as needed.
}
]]>
</code>
<code title="Invalid: Using the error operator to silence errors.">
<![CDATA[
$conn_id = <em>@ftp_connect( $ftp_server );</em>
]]>
</code>
</code_comparison>
</documentation>
Loading