-
-
Notifications
You must be signed in to change notification settings - Fork 490
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
base: develop
Are you sure you want to change the base?
Add documentation for WordPress.PHP.DevelopmentFunctions #2490
Conversation
There was a problem hiding this 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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()."> |
There was a problem hiding this comment.
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()
.
Related to #1722