WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 8 weeks ago

#25514 closed defect (bug) (fixed)

Hook Docs (47): wp-includes/query.php

Reported by: dougwollison Owned by: DrewAPicture
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_query
  • parse_tax_query
  • pre_get_posts
  • posts_where
  • posts_join
  • comment_feed_join
  • comment_feed_where
  • comment_feed_groupby
  • comment_feed_orderby
  • comment_feed_limits
  • posts_where_paged
  • posts_groupby
  • posts_join_paged
  • posts_orderby
  • posts_distinct
  • post_limits
  • posts_fields
  • posts_clauses
  • posts_selection
  • posts_where_request
  • posts_groupby_request
  • posts_join_request
  • posts_orderby_request
  • posts_distinct_request
  • posts_fields_request
  • post_limits_request
  • posts_clauses_request
  • posts_request
  • split_the_query
  • posts_request_ids
  • posts_results
  • comment_feed_join
  • comment_feed_where
  • comment_feed_groupby
  • comment_feed_orderby
  • comment_feed_limits
  • the_preview
  • the_posts
  • found_posts_query
  • found_posts
  • loop_start
  • loop_end
  • comment_loop_start
  • the_post

Attachments (8)

WP_Query-hook-docs.diff (18.6 KB) - added by dougwollison 6 months ago.
Revised original diff; fixed consistency when mentioning "WP_Query object"
WP_Query-hook-docs-rev1.diff (22.4 KB) - added by dougwollison 6 months ago.
Updated revised diff; fixed consistency when mentioning "WP_Query object"
WP_Query-hook-docs.2.diff (18.7 KB) - added by dougwollison 6 months ago.
Updated "object" to "instance" when talking about $this.
WP_Query-hook-docs.3.diff (20.2 KB) - added by dougwollison 6 months ago.
Updated with requested changes.
WP_Query-hook-docs.4.diff (20.9 KB) - added by dougwollison 6 months ago.
Updated descriptions for clause related hooks.
WP_Query-hook-docs.5.diff (20.9 KB) - added by dougwollison 6 months ago.
Also fixed missing () on function names.
WP_Query-hook-docs.6.diff (21.1 KB) - added by dougwollison 6 months ago.
Spelling corrections; updated notes on pre_get_posts hook.
25514.diff (20.9 KB) - added by DrewAPicture 8 weeks ago.
Cleanup + fixed @since versions

Download all attachments as: .zip

Change History (34)

comment:1 johnbillion6 months ago

  • Type changed from defect (bug) to enhancement

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() and do_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

comment:2 dougwollison6 months 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.

Last edited 6 months ago by dougwollison (previous) (diff)

dougwollison6 months ago

Revised original diff; fixed consistency when mentioning "WP_Query object"

dougwollison6 months ago

Updated revised diff; fixed consistency when mentioning "WP_Query object"

comment:3 johnbillion6 months 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.

dougwollison6 months ago

Updated "object" to "instance" when talking about $this.

comment:4 DrewAPicture6 months 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 "before get_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 $selection before 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 $pieces before the doc. Again, avoid nonexistent variables.

posts_request filter:

  • Assign $this->request to $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->posts to $posts_results and change the referenced variable in the parameter doc

the_preview filter:

  • Assign $this->posts[0] to $post_preview and change the referenced variable in the parameter line (don't want to stomp the $post global, afterall

the_posts filter:

  • Assign $this->posts to $the_posts and change the referenced variable in the parameter doc

found_posts_query filter:

  • Assign 'SELECT FOUND_ROWS()' to $found_posts_query and change the referenced variable in the parameter doc

found_posts filter:

  • Assign $this->found_posts to $found_posts
Last edited 6 months ago by DrewAPicture (previous) (diff)

comment:5 DrewAPicture6 months ago

  • Owner set to DrewAPicture
  • Status changed from new to reviewing

dougwollison6 months ago

Updated with requested changes.

comment:6 follow-up: dougwollison6 months 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.

Version 0, edited 6 months ago by dougwollison (next)

comment:7 in reply to: ↑ 6 DrewAPicture6 months 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.

comment:8 DrewAPicture6 months 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.

dougwollison6 months ago

Updated descriptions for clause related hooks.

dougwollison6 months ago

Also fixed missing () on function names.

comment:9 follow-up: dougwollison6 months 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 );

comment:10 in reply to: ↑ 9 DrewAPicture6 months 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.

comment:11 DrewAPicture6 months 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?

comment:12 DrewAPicture6 months 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."

dougwollison6 months ago

Spelling corrections; updated notes on pre_get_posts hook.

comment:13 follow-up: dougwollison6 months 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."

comment:14 in reply to: ↑ 13 helen6 months 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.

comment:15 dougwollison6 months ago

Good point... just a mo.

comment:16 dougwollison6 months 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."

comment:17 kpdesign6 months ago

  • Type changed from enhancement to defect (bug)

comment:18 DrewAPicture4 months 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

comment:19 jeremyfelt3 months ago

  • Component changed from Inline Docs to Query
  • Focuses docs added

DrewAPicture8 weeks ago

Cleanup + fixed @since versions

comment:20 DrewAPicture8 weeks 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.

comment:21 DrewAPicture8 weeks ago

In 27206:

Inline documentation for various SQL clause hooks in wp-includes/query.php.

Covers documentation for general SQL clause hooks, including:

  • posts_where
  • posts_join
  • posts_where_paged
  • posts_groupby
  • posts_join_paged
  • posts_orderby
  • posts_distinct
  • post_limits
  • posts_fields
  • posts_clauses

Props dougwollison, DrewAPicture.
See #25514.

comment:22 DrewAPicture8 weeks ago

In 27207:

Inline documentation for various SQL clause hooks in wp-includes/query.php.

Covers documentation for SQL clause hooks specified for use by caching plugins, including:

  • posts_selection
  • posts_where_request
  • posts_groupby_request
  • posts_join_request
  • posts_orderby_request
  • posts_distinct_request
  • posts_fields_request
  • post_limits_request
  • posts_clauses_request

Props dougwollison, DrewAPicture.
See #25514.

comment:23 DrewAPicture8 weeks ago

In 27208:

Inline documentation for various SQL clause hooks in wp-includes/query.php.

Covers documentation for SQL clause hooks related to comment feeds, including:

  • comment_feed_join
  • comment_feed_where
  • comment_feed_groupby
  • comment_feed_orderby
  • comment_feed_limits

Props dougwollison, DrewAPicture.
See #25514.

comment:24 DrewAPicture8 weeks ago

In 27210:

Inline documentation hooks in wp-includes/query.php.

Covers documentation for the various remaining query hooks, notably including but not limited to:

  • parse_query
  • parse_tax_query
  • pre_get_posts
  • posts_results
  • the_posts
  • found_posts
  • the_post

Props dougwollison, DrewAPicture.
See #25514.

comment:25 DrewAPicture8 weeks ago

In 27211:

Remove now-unnecessary vanity spacing of various hooks in wp-includes/query.php.

See #25514.

comment:26 DrewAPicture8 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

I think we're done here.

Note: See TracTickets for help on using tickets.