Opened 12 years ago
Closed 12 years ago
#25514 closed defect (bug) (fixed)
Hook Docs (47): wp-includes/query.php
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 3.9 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Query | Keywords: | has-patch commit |
| Focuses: | docs | Cc: |
Description
Added hook documentation for the following actions/filters found in wp-includes/query.php:
parse_queryparse_tax_querypre_get_postsposts_whereposts_joincomment_feed_joincomment_feed_wherecomment_feed_groupbycomment_feed_orderbycomment_feed_limitsposts_where_pagedposts_groupbyposts_join_pagedposts_orderbyposts_distinctpost_limitsposts_fieldsposts_clausesposts_selectionposts_where_requestposts_groupby_requestposts_join_requestposts_orderby_requestposts_distinct_requestposts_fields_requestpost_limits_requestposts_clauses_requestposts_requestsplit_the_queryposts_request_idsposts_resultscomment_feed_joincomment_feed_wherecomment_feed_groupbycomment_feed_orderbycomment_feed_limitsthe_previewthe_postsfound_posts_queryfound_postsloop_startloop_endcomment_loop_startthe_post
Attachments (8)
Change History (34)
#2
@
12 years ago
Attached revised version, but I thought these were to be documented for how the hooks should be used; any hook callbacks using these won't be passed an array, but the individual items within the array (e.g. "posts_where" receives the arguments $where and $wp_query; not array($where, $wp_query)).
Also, my other patch (#25495) didn't use hashes and nobody suggested it.
#3
@
12 years ago
Interesting point Doug, and I think I agree. We should document how the filter arguments get passed to callbacks. So your first patch would be preferred.
#4
@
12 years ago
WP_Query-hook-docs.2.diff:
Overall:
- Any function references in descriptions need parenthesis, e.g. s/parse_query/parse_query()
- I'm not a big fan of this wording: "Fires actions to be run after ...". It is an action. "Fires after ..." says the same thing without ambiguity, and is consistent with the suggested language for actions.
- "refrence" is misspelled many times, probably from copy/pasta
- s/portion/clause in many short descriptions
- There's a lot of copy pasta in here. The point of documenting the individual hooks is to both differentiate them from each other and provide valuable, unique information about each specific hook. It's also important to provide context. View each hook as independent of the other hooks, even independent of the file it's in.
pre_get_posts hook:
- The description is very literal, and kind of incorrect. If
$this->parse_query()has already run, it's hardly "beforeget_posts()actually begins. - Worth noting in a long description that many/most conditional tags aren't available yet for this hook. For instance,
is_main_query()/#23329
posts_where_paged, posts_groupby, posts_join_paged, posts_orderby, posts_distinct, post_limits, posts_fields, posts_clauses filters:
- Need to specify that these are specifically for use when "paged" in each hook's description
posts_selection hook:
- Assign the concatenated variables to
$selectionbefore the doc starts. We need to be very careful about not documenting variables that don't exist. - Since the parameter is a concatenation of the clauses, we need to spell out which clauses comprise the parameter, either in the parameter description, or a long description for the hook doc.
posts_where_request, posts_groupby_request, posts_join_request, posts_orderby_request, posts_distinct_request, posts_fields_request, post_limits_request, posts_clauses_request filters:
- Specify that these filters are for use by caching plugins in each hook's description
posts_clauses_request filter:
- Assign
compact( $pieces )to$piecesbefore the doc. Again, avoid nonexistent variables.
posts_request filter:
- Assign
$this->requestto$request.
split_the_query filter:
- Description is pretty vague. Perhaps explain what splitting the query actually means.
posts_request_ids filter:
- Should specify that it's a SQL request
posts_results filter:
- Assign
$this->poststo$posts_resultsand change the referenced variable in the parameter doc
the_preview filter:
- Assign
$this->posts[0]to$post_previewand change the referenced variable in the parameter line (don't want to stomp the$postglobal, afterall
the_posts filter:
- Assign
$this->poststo$the_postsand change the referenced variable in the parameter doc
found_posts_query filter:
- Assign
'SELECT FOUND_ROWS()'to$found_posts_queryand change the referenced variable in the parameter doc
found_posts filter:
- Assign
$this->found_poststo$found_posts
#6
follow-up:
↓ 7
@
12 years ago
Done, however in terms of documenting variables that "don't exist", it still goes back to the fact that we're documenting how the hook is used; the exact name of the parameter doesn't matter, as it's getting passed to another function that may use it's only naming for it.
We almost don't need to name the variables at all; just the type and a description. It's more like a @return tag than a @param tag in some ways. PHPDoc wasn't really written with this particular use in mind.
#7
in reply to:
↑ 6
@
12 years ago
Replying to dougwollison:
Done, however in terms of documenting variables that "don't exist", it still goes back to the fact that we're documenting how the hook is used; the exact name of the parameter doesn't matter, as it's getting passed to another function that may use it's only naming for it.
We almost don't need to name the variables at all; just the type and a description. It's more like a @return tag than a @param tag in some ways.
Unfortunately, we can't just look at this from the perspective of "<this thing> will be parsed, so it doesn't really matter."
As pointed out by @tivnet in another ticket (can't find the number), we need be careful about 1) declaring types for nonexistent variables, 2) redeclaring new types for existing variables in doc blocks, even if they are for hooks, because they can cause validation errors.
We're never going to be able to circumvent all of these cases, but taking a proactive approach to avoiding them is probably for the best.
PHPDoc wasn't really written with this particular use in mind.
This is correct. Unfortunately, WordPress has an action and filter architecture, and this is the best method we've been able to come up with for documenting hooks and filters.
#8
@
12 years ago
WP_Query-hook-docs.3.diff is looking much better. More changes:
- Any function references in descriptions need parenthesis, e.g. s/parse_query/parse_query()
posts_clauses filter:
- Specify which clauses this covers
posts_where_request, posts_groupby_request, posts_join_request, posts_orderby_request, posts_distinct_request, posts_fields_request, post_limits_request, posts_clauses_request filters:
- Specify that these filters are for use by caching plugins in each hook's description
posts_clauses_request filter:
- Specify which clauses this covers
split_the_query filter:
- Specify the default (true or false) at the end of the parameter description.
#9
follow-up:
↓ 10
@
12 years ago
Done, although I can't specify a default value for $split_the_query since it's a calculated value, unlike most of those flags:
$split_the_query = ( $old_request == $this->request && "$wpdb->posts.*" == $fields && !empty( $limits ) && $q['posts_per_page'] < 500 );
#10
in reply to:
↑ 9
@
12 years ago
Replying to dougwollison:
Done, although I can't specify a default value for $split_the_query since it's a calculated value, unlike most of those flags:
$split_the_query = ( $old_request == $this->request && "$wpdb->posts.*" == $fields && !empty( $limits ) && $q['posts_per_page'] < 500 );
Good call. I'll take a look at the new patch.
#11
@
12 years ago
@dougwoolison: You do good work :)
WP_Query-hook-docs.5.diff:
the_post filter:
- "retreived"
pre_get_posts hook:
The long description note isn't really correct and that's my badness. It's not that they aren't available, it's that conditional tags are testing the $wp_query global and not the main query, which is what is passed to this hook.
After some discussion with @helen, I'd like to change that to something like:
"Note: WP_Query member methods should be used to test the main query here, rather than global conditional functions. For instance, use WP_Query::is_main_query() instead of is_main_query()."
Have a better suggested wording?
#12
@
12 years ago
Or maybe:
"Note: global conditional tags like is_main_query() shouldn't be used with this hook as they test against the $wp_query global, not the main query."
#13
follow-up:
↓ 14
@
12 years ago
"Note: If using conditional tags, use the method versions within the passed instance
(e.g. $this->is_main_query() instead of is_main_query()). This is because the functions
like is_main_query() test against the global $wp_query instance, not the passed one."
#14
in reply to:
↑ 13
@
12 years ago
Replying to dougwollison:
"Note: If using conditional tags, use the method versions within the passed instance
(e.g. $this->is_main_query() instead of is_main_query()). This is because the functions
like is_main_query() test against the global $wp_query instance, not the passed one."
Works for me in the context of is_main_query(), but I don't think it globally applies to all conditionals. Maybe you want to alter a nav menu item query when you're on a date archive or something.
#16
@
12 years ago
How's about...
"Note: keep in mind the context of any conditional tags you use. For example, to test if THIS instance is the main query, use $this->is_main_query(). This is because the global function versions, such as is_main_query(), are run on the global $wp_query instance."
#18
@
12 years ago
- Keywords has-patch needs-refresh added; needs-patch removed
- Summary changed from Hook Docs: wp-includes/query.php to Hook Docs (47): wp-includes/query.php
#20
@
12 years ago
- Keywords commit added; needs-refresh removed
- Milestone changed from Awaiting Review to 3.9
25514.diff refreshes the last patch. I also double-checked and fixed @since versions, tightened up several descriptions, etc.
I'm confident we can get this in for 3.9 (finally!). I think I'm probably going to incrementally commit these hook docs instead of all at once. This should allow more focused review.
The formatting for the arguments documented in WP_Query-hook-docs.diff isn't quite correct.
Arguments in array format (ie. those passed to
apply_filters_ref_array()anddo_action_ref_array()should use hash notation. See http://make.wordpress.org/core/handbook/inline-documentation-standards/php-documentation-standards/#4-hooks-actions-and-filters