WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 16 months ago

Last modified 16 months ago

#25495 closed defect (bug) (fixed)

Hook Docs: wp-includes/class-wp.php

Reported by: dougwollison Owned by: DrewAPicture
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Inline Docs Keywords: needs-patch
Focuses: Cc:

Description

Added patch has inline docs for do_parse_request, query_vars, request, parse_request, wp_headers, send_headers, query_string, and wp filters/actions found in wp-includes/class-wp.php.

Attachments (4)

class-wp.php (20.9 KB) - added by dougwollison 17 months ago.
WordPress environment setup class. (With inline docs for hooks)
25495-patch.diff (4.1 KB) - added by dougwollison 17 months ago.
RE-Updated diff file (Added missing period)
25495-patch.2.diff (4.1 KB) - added by dougwollison 17 months ago.
Updated "object" to "instance" when talking about $this. Also added () to function names when mentioning.
25495.patch (4.2 KB) - added by DrewAPicture 16 months ago.
Cleanup

Download all attachments as: .zip

Change History (18)

@dougwollison17 months ago

WordPress environment setup class. (With inline docs for hooks)

comment:1 @DrewAPicture17 months ago

  • Keywords needs-patch added

Hi, can you please re-upload your patch in either .diff or .patch format?

comment:2 @DrewAPicture17 months ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to DrewAPicture
  • Status changed from new to reviewing

comment:3 @DrewAPicture17 months ago

  • Keywords needs-patch added; has-patch removed

Hi, thanks for the new patch. Looks pretty good, just a couple of things to tweak.

Some notes on 25495-patch.diff:

  • All short descriptions and parameter descriptions should end with a period.
  • Add whitespacing per coding standards to the lines containing the apply_filters() or do_action() calls (a rare opportunity to do this!).

do_parse_request filter:

  • "Test if parse_request should be done." should follow the suggested language of "Filter <this thing>"
  • The first parameter needs a description with an ending period. With booleans, we often use language something like "Whether to <something>".
  • Move the @since line above the parameter lines.

query_vars filter:

  • In the long description, I'd suggest wrapping to a new line after the word "prior".

query_string filter:

  • Period missing on the short description
  • @depreciated should be @deprecated, wrap the hook names in single quotes on this line.

@dougwollison17 months ago

RE-Updated diff file (Added missing period)

comment:4 @DrewAPicture17 months ago

25495-patch.diff is looking much better, just a few things left:
— In the future, it's best not to overwrite the original patch because it makes it difficult track progress and context. If you name the file the same thing but don't check the "overwrite" box on upload, Trac will automatically increment the file name for you :)

General:

  • Since $this literally refers to the current instance, should probably replace "object" with "instance" in this frequently used blurb: "The WordPress environment object"

do_parse_request filter:

  • Add parenthesis to mentions of the parse_request() function

query_vars filter:

  • If you're going to reference $public_query_vars in the docblock, it should actually exist. So, for example, you could just do the following before the docblock:
    $public_query_vars = $this->public_query_vars;
    

request filter:

  • The long description is unnecessary as that's already the nature of a filter.
  • Assign $this->query_vars to $query_vars above the docblock, just like for the query_vars filter.

send_headers hook:

  • Add parenthesis to references to the send_headers() function.

query_string filter:

  • Assign $this->query_string to $query_string before the docblock.
Last edited 17 months ago by DrewAPicture (previous) (diff)

@dougwollison17 months ago

Updated "object" to "instance" when talking about $this. Also added () to function names when mentioning.

comment:5 follow-up: @dougwollison17 months ago

I've made the fixes. However, with regard to saying it's passed as an array, it's technically not; any callbacks that use the hook get the array elements as the separate arguments. @johnbillion brought this up on #25514 and agreed with me: http://core.trac.wordpress.org/ticket/25514#comment:2.

comment:6 in reply to: ↑ 5 @DrewAPicture17 months ago

Replying to dougwollison:

I've made the fixes. However, with regard to saying it's passed as an array, it's technically not; any callbacks that use the hook get the array elements as the separate arguments. @johnbillion brought this up on #25514 and agreed with me: http://core.trac.wordpress.org/ticket/25514#comment:2.

You are correct, and I agree.

@DrewAPicture16 months ago

Cleanup

comment:7 @DrewAPicture16 months ago

  • Keywords has-patch commit added; needs-patch removed
  • Milestone changed from Awaiting Review to 3.8

Did some language cleanup in 25495.patch.

comment:8 @kpdesign16 months ago

25495.patch looks good. Recommend commit.

comment:9 @DrewAPicture16 months ago

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

In 25940:

Inline documentation for hooks in wp-includes/class-wp.php.

Props dougwollison.
Fixes #25495.

comment:10 @c3mdigital16 months ago

#25719 was marked as a duplicate.

comment:11 @c3mdigital16 months ago

  • Keywords needs-patch added; has-patch commit removed

Reopening because there is a bug in the committed patch see: #25719

comment:12 @c3mdigital16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:13 @DrewAPicture16 months ago

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

In 25945:

Fix error introduced in [25940] where $public_query_vars should have only been used as a non-existent inline docs variable in the query_vars filter.

Restores $this->public_query_vars to the query_vars filter in wp-includes/class-wp.php.

Props mauryaratan.
Fixes #25495. See #25719.

comment:14 @DrewAPicture16 months ago

In 25946:

Revert another instance where a WP property was assigned to a one-time variable for inline docs purposes.

Referencing a non-existent variable only in the docs here would have been the better choice.

See #25495.

Note: See TracTickets for help on using tickets.