Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#25495 closed defect (bug) (fixed)

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

Reported by: dougwollison's profile dougwollison Owned by: drewapicture's profile 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 10 years ago.
WordPress environment setup class. (With inline docs for hooks)
25495-patch.diff (4.1 KB) - added by dougwollison 10 years ago.
RE-Updated diff file (Added missing period)
25495-patch.2.diff (4.1 KB) - added by dougwollison 10 years ago.
Updated "object" to "instance" when talking about $this. Also added () to function names when mentioning.
25495.patch (4.2 KB) - added by DrewAPicture 10 years ago.
Cleanup

Download all attachments as: .zip

Change History (18)

@dougwollison
10 years ago

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

#1 @DrewAPicture
10 years ago

  • Keywords needs-patch added

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

#2 @DrewAPicture
10 years ago

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

#3 @DrewAPicture
10 years 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.

@dougwollison
10 years ago

RE-Updated diff file (Added missing period)

#4 @DrewAPicture
10 years 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.

parse_request hook:

  • The parameter is passed via an array so that should be the type assigned in the doc.

send_headers hook:

  • Add parenthesis to references to the send_headers() function.
  • &$this is passed in an array, same as the parse_request hook.

query_string filter:

  • Assign $this->query_string to $query_string before the docblock.

wp hook:

  • &$this is passed in array
Version 2, edited 10 years ago by DrewAPicture (previous) (next) (diff)

@dougwollison
10 years ago

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

#5 follow-up: @dougwollison
10 years 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.

#6 in reply to: ↑ 5 @DrewAPicture
10 years 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.

@DrewAPicture
10 years ago

Cleanup

#7 @DrewAPicture
10 years ago

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

Did some language cleanup in 25495.patch.

#8 @kpdesign
10 years ago

25495.patch looks good. Recommend commit.

#9 @DrewAPicture
10 years 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.

#10 @c3mdigital
10 years ago

#25719 was marked as a duplicate.

#11 @c3mdigital
10 years ago

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

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

#12 @c3mdigital
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#13 @DrewAPicture
10 years 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.

#14 @DrewAPicture
10 years 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.