PHP/RestrictedPHPFunctions: add XML documentation#2693
PHP/RestrictedPHPFunctions: add XML documentation#2693jrfnl merged 5 commits intoWordPress:developfrom
Conversation
Some of these changes were suggested during the review of PR 2491, and others were decided while working on the new PR: - Make the standard description generic instead of mentioning create_function() specifically, following the pattern used by other docs like DeprecatedFunctionsStandard.xml. - Use "must not" instead of "should not" since the sniff produces an error. - Simplify the code examples by removing the add_action() wrapper. - Add <em> tags to the valid code example.
Following the suggestion in PR 2687, this commit improves the standard description to explain why these functions must not be used. The phrasing is kept generic instead of mentioning create_function() specifically, since the sniff name suggests it could be extended with more functions in the future.
|
Just noting here that, following what was suggested in #2687 (review), I added a new commit that expands the standard description to explain why the rule exists. |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
Thanks for working on this @rodrigoprimo!
Simplify the code examples by removing the
add_action()wrapper.
I'm not so sure that was the right call. The create_function() function was typically used as a callback for a WP hook-in, so while including that wrapper makes the code sample a little more complicated, it also makes it far more realistic and easier to recognize.
Reposition the
<em>tags to the valid code example.
I'd suggest ensuring that the <em> tags enclose the complete create_function() function call and it's equivalent closure.
That way it is more straight-forward for someone to see how to replace one with the other.
Follow up on the conversation about this phrasing in #2491 (comment) and #2693 (comment)
Follow up on the conversation about this phrasing in #2491 (comment) and #2693 (comment)
Follow up on the conversation about this phrasing in #2491 (comment) and #2693 (comment) Also related to: WordPress/wpcs-docs#158
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
rodrigoprimo
force-pushed
the
docs-restricted-php-functions
branch
from
3dcdd85 to
3c26be4
Compare
|
Thanks for your review, @jrfnl! I have applied the changes that you suggested, and this is now ready for another check. |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
All good from me. Thanks for working on this @rodrigoprimo !
|
For whomever does the second review: please squashmerge. |
| > | ||
| <standard> | ||
| <
There was a problem hiding this comment.
as they pose security risks
While https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/PHP/RestrictedPHPFunctionsSniff.php only contains the one restricted function (and that's because it's a security risk), there's nothing in the sniff name and comments as a whole, which limits the list of restricted functions to just those with security issues. As such, this XML documentation is a little misaligned with the sniff file comments.
If we were to add a new restricted PHP function because it had performance issues or wasn't cross-OS compatible, we'd also have to amend this XML description.
There was a problem hiding this comment.
Thanks for your review, @GaryJones. That is a good point. Should I update the description to be just:
Certain PHP functions must not be used. Use the recommended alternatives instead.
Or do you have a different suggestion?
There was a problem hiding this comment.
Yes, generalising the sniff description, but leaving comments in the rule code.
There was a problem hiding this comment.
Thanks, @GaryJones! I pushed a new commit implementing your suggestion.
5 participants
Description
This PR adds XML documentation for the
WordPress.PHP.RestrictedPHPFunctionssniff.It is based on the work started by @gogdzl in #2491. I squashed the original commits and, in a separate commit, reworked the documentation to address some of the changes suggested during the review of #2491 and a few more that I decided while working on this PR:
create_function()specifically, following the pattern used by other docs likeDeprecatedFunctionsStandard.xml.add_action()wrapper.<em>tags to the valid code example.I suggest squashing the two commits before merging. I'm opening the PR without doing that to make it easier to tell my changes apart from the original changes.
Suggested changelog entry
N/A
Related issues/external references
Related to: #1722
Supersedes: #2491
Closes #2491