-
-
Notifications
You must be signed in to change notification settings - Fork 522
DB/RestrictedFunctions: add XML documentation #2676
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
base: develop
Are you sure you want to change the base?
DB/RestrictedFunctions: add XML documentation #2676
Conversation
jrfnl
left a comment
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 finishing this one off @rodrigoprimo !
I've reviewed this PR. The first and second commit look good, but I don't understand the reason for the third commit.
The second code comparison shows off the use of the $wpdb object, which is mentioned in the generic description, so, to me, this code sample feels like a good addition. I see no reason to remove the second code comparison.
Care to explain what your reasoning for this is ?
|
Thanks for your review, @jrfnl! I opted to remove the second code comparison block to simplify the documentation. The However, you're right that showing I added a commit illustrating what I have in mind. Let me know your thoughts. |
To me the code samples felt sufficiently different, as the one was about fetching data from the database, the other about inserting data into the database. And considering the security aspects of "manual" database queries versus using the WP abstraction layer, I don't think having some extra examples is a bad thing in this case.
That could be fixed by using a different function from the list of functions the sniff detects, which also includes functions from other extensions than
I'm not a fan as the "valid" vs "invalid" examples are not comparable now. I'd still vote for removing the last two commits (and possibly making some small tweaks to the second code sample, but I'd be fine leaving it as-is too). N.B.: I've not tested the code samples for validity. |
5a6eee6 to
c9e123e
Compare
Ok, I removed the last two commits and added a new one, making a few minor changes to the second code comparison block.
I did check, and they look good to me after a small fix that I made. This PR is ready for another review. |
jrfnl
left a comment
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.
LGTM! Thanks.
c9e123e to
86ea20d
Compare
|
Just documenting that I squashed all the commits into one without changes. |
Description
This PR adds XML documentation for the
WordPress.DB.RestrictedFunctionssniff.The documentation is based on the work started by @paulgibbs in #2453. I used the original commit, and then made subsequent changes, mostly based on the review left in #2453.
I suggest squashing those 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: #2453