WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years 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 2 years ago.
WordPress environment setup class. (With inline docs for hooks)
25495-patch.diff (4.1 KB) - added by dougwollison 2 years ago.
RE-Updated diff file (Added missing period)
25495-patch.2.diff (4.1 KB) - added by dougwollison 2 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 2 years ago.
Cleanup

Download all attachments as: .zip

Change History (18)

@dougwollison
2 years ago

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

#1 @DrewAPicture
2 years ago

  • Keywords needs-patch added

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

#2 @DrewAPicture
2 years ago

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

#3 @DrewAPicture
2 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
2 years ago

RE-Updated diff file (Added missing period)

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

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 2 years ago by DrewAPicture (previous) (diff)

@dougwollison
2 years ago

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

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

Cleanup

#7 @DrewAPicture
2 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
2 years ago

25495.patch looks good. Recommend commit.

#9 @DrewAPicture
2 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
2 years ago

#25719 was marked as a duplicate.

#11 @c3mdigital
2 years ago

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

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

#12 @c3mdigital
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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