DB/SlowDBQuery: add XML documentation#2699
Conversation
- Rewrite standard text to mention all four flagged keys, clarify that both array assignments and query-parameter strings are detected. - Replace code examples with examples that stay within 48-character limit and use `<em>` tags. - Remove second code comparison that incorrectly showed `tax_query` and `meta_key` as valid when the sniff warns on all four keys.
There was a problem hiding this comment.
@rodrigoprimo Thanks for working on this!
The code samples are confusing to me as the description and the sniff name point to this being about database queries, but none of the code samples are querying the database.
While I realize that the sniff doesn't actually check the keys are used in the context of a database query, I think the code samples in the docs should demonstrate what we're concerned about.
To that end, I've left some suggestions inline, but there may be more things which can be improved ?
1288e0e to
6c1dd8d
Compare
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
6c1dd8d to
63ff8ec
Compare
|
Thanks for the review, @jrfnl! Good point about the examples showing database query context. I applied your suggestions for the I'm unsure whether to make a similar change for the string example. I considered the example below. The code works, but I don't think it's a good idea to use an undocumented pattern in the valid example. The documentation doesn't mention that $posts = get_posts(
'post_type=post&orderby=date'
);Do you think the documentation is good as is, or should we think a bit more about the string example? |
jrfnl
left a comment
There was a problem hiding this comment.
I'm unsure whether to make a similar change for the string example. I don't think it's a good idea to use an undocumented pattern in the valid example. The documentation doesn't mention that
get_posts()accepts a string.
To me, this means that a PR to the WP codebase to fix the function documentation may be in order.
Do you think the documentation is good as is, or should we think a bit more about the string example?
I'm okay with the documentation as-is. I do think it would be good to improve the string example, but I don't see that as a blocker for this PR. That can be done in a future iteration when someone comes up with a better code sample.
For now, IMO, this will work fine.
Thanks @rodrigoprimo !
|
For whomever merged this, please squash-merge this PR. |
Here is the core ticket: https://core.trac.wordpress.org/ticket/64813 |
Description
This PR adds XML documentation for the
WordPress.DB.SlowDBQuerysniff.The documentation is based on the work started by @petitphp in #2464. I used the original commit and, in a separate commit, made the following changes to the original version of the documentation:
<em>tags.tax_queryandmeta_keyas valid when the sniff warns on all four keys.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: #2464
Closes #2464