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

Add documentation for WordPress.PHP.NoSilencedErrors #2495

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

Conversation

gogdzl
Copy link

@gogdzl gogdzl commented Sep 17, 2024

Related to #1722

Copy link
Collaborator

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

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"
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).

<code_comparison>
<code title="Valid: Using proper error checking when calling functions not in the allowed list.">
<![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 );

<![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:

<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>

]]>
</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.

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

Successfully merging this pull request may close these issues.

2 participants