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.DevelopmentFunctions #2490

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

gogdzl
Copy link

@gogdzl gogdzl commented Sep 17, 2024

Related to #1722

@jrfnl jrfnl mentioned this pull request Sep 17, 2024
61 tasks
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 working on this PR, @gogdzl! I'm not a maintainer of the project so take my review with a grain of salt. That being said, I left some comments based on my own experience creating PHPCS and WPCS sniffs.

<![CDATA[
Debug code should not normally be used in production.

Typically, this rule verifies if function calls to the PHP native `error_log()`, `var_dump()`, `var_export()`, `print_r()`, `trigger_error()`, `set_error_handler()`, `debug_backtrace`, `debug_print_backtrace` and `wp_debug_backtrace_summary()` functions are present in the code.
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
Typically, this rule verifies if function calls to the PHP native `error_log()`, `var_dump()`, `var_export()`, `print_r()`, `trigger_error()`, `set_error_handler()`, `debug_backtrace`, `debug_print_backtrace` and `wp_debug_backtrace_summary()` functions are present in the code.
This rule verifies if calls to the `error_log()`, `var_dump()`, `var_export()`, `print_r()`, `trigger_error()`, `set_error_handler()`, `debug_backtrace()`, `debug_print_backtrace()` and `wp_debug_backtrace_summary()` functions are present in the code.

I'm suggesting the removal of the PHP native part as wp_debug_backtrace_summary() is not a PHP native function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe instead of verifies, we could use warns to more accurately describe what the rule does? Just thinking out loud here.

>
<standard>
<![CDATA[
Debug code should not normally be used in production.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that this is the warning message displayed by the sniff, but I believe that the documentation (and maybe also the sniff messages) usually only uses should/should not when describing an error. Since this is a warning, maybe we could say that it is recommended or something like that?

]]>
</standard>
<code_comparison>
<code title="Valid: (Not) using var_dump().">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not is in between parenthesis?

Also, I wonder if the message should be more generic and mention debug code/functions instead of singling out var_dump()? I'm ok with a single example with just var_dump().

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