Skip to content

Conversation

@rodrigoprimo
Copy link
Collaborator

Description

This PR adds XML documentation for the WordPress.DB.RestrictedFunctions sniff.

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

Copy link
Member

@jrfnl jrfnl 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 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 ?

@rodrigoprimo
Copy link
Collaborator Author

Thanks for your review, @jrfnl!

I opted to remove the second code comparison block to simplify the documentation. The wp_insert_post() example feels redundant, since get_posts() already demonstrates the use of WordPress wrapper functions in the first code comparison block. The valid and invalid titles for both blocks were also very similar.

However, you're right that showing $wpdb usage is valuable. What do you think about adding the $wpdb->insert() example to the first code comparison block (with or without a corresponding invalid example) instead of restoring the second block? This would keep the documentation concise while still illustrating both valid approaches.

I added a commit illustrating what I have in mind. Let me know your thoughts.

@jrfnl
Copy link
Member

jrfnl commented Dec 23, 2025

I opted to remove the second code comparison block to simplify the documentation. The wp_insert_post() example feels redundant, since get_posts() already demonstrates the use of WordPress wrapper functions in the first code comparison block.

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.

The valid and invalid titles for both blocks were also very similar.

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 mysqli (though in practice it will be far less common to see these in the wild), but even a different mysqli function call could make the code samples more varied.

However, you're right that showing $wpdb usage is valuable. What do you think about adding the $wpdb->insert() example to the first code comparison block (with or without a corresponding invalid example) instead of restoring the second block? This would keep the documentation concise while still illustrating both valid approaches.

I added a commit illustrating what I have in mind. Let me know your thoughts.

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.

@rodrigoprimo
Copy link
Collaborator Author

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

Ok, I removed the last two commits and added a new one, making a few minor changes to the second code comparison block.

N.B.: I've not tested the code samples for validity.

I did check, and they look good to me after a small fix that I made. This PR is ready for another review.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@jrfnl jrfnl added this to the 3.3.x milestone Dec 24, 2025
@rodrigoprimo rodrigoprimo force-pushed the docs-restricted-functions branch from c9e123e to 86ea20d Compare December 24, 2025 11:58
@rodrigoprimo
Copy link
Collaborator Author

Just documenting that I squashed all the commits into one without changes.

@jrfnl jrfnl mentioned this pull request Dec 25, 2025
61 tasks
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.

3 participants