WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#26252 closed defect (bug) (fixed)

Hook Docs: wp-admin/includes/update.php

Reported by: stevenkword Owned by: DrewAPicture
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Inline Docs Keywords: has-patch commit
Focuses: Cc:

Description

Attached patch has docs for the in_plugin_update_message-$file, in_theme_update_message-$theme_key actions in wp-admin/includes/update.php.

Attachments (7)

26252.diff (5.0 KB) - added by stevenkword 8 years ago.
26252.2.diff (5.3 KB) - added by stevenkword 8 years ago.
26252.3.diff (5.4 KB) - added by stevenkword 8 years ago.
Updates formatting
26252.4.diff (5.5 KB) - added by stevenkword 8 years ago.
Updates formatting
26252.5.diff (5.5 KB) - added by stevenkword 8 years ago.
More Formatting
26252.6.diff (5.5 KB) - added by stevenkword 8 years ago.
6th time's the charm
26252.7.diff (3.4 KB) - added by DrewAPicture 8 years ago.
Clean up

Download all attachments as: .zip

Change History (19)

@stevenkword
8 years ago

#1 @DrewAPicture
8 years ago

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

Hi, thanks for the patch. I'll have a review for you shortly.

#2 @DrewAPicture
8 years ago

  • Keywords needs-patch added; has-patch removed

26252.diff is a great start. Here are some notes:
Both hooks:

  • Use lowercase variables in the @param docs. They may be capitalized in the array keys, but we're aiming for consistency with the other hook docs, e.g. $Name becomes $name, $PluginURI becomes $pluginuri or $plugin_uri, and so on.
  • The hash notation indents should be four spaces, not tabs. This goes for the array descriptions and all inner @type lines.
  • The long descriptions should describe what the dynamic portion of the hook names, in this case $file refers to. Even a simple example for each would help.
  • Are these hooks Multisite specific? If not, we should avoid pigeon-holing them to the Network Admin in the long description

in_theme_update_message-{$file} hook:

  • Use the short description to describe where/when the hook fires, not why. Something like the following: "Fires at the end of each row in the theme updates table."

in_plugin_update_message-{$file} hook:

  • Same as the first hook. Try to describe where/when the hook fires, not why. "Fires at the end of each row in the plugin updates table."

#3 follow-up: @stevenkword
8 years ago

Hi Drew,
What is the preferred way of dealing with spaces found within array keys? Should they be converted to underscores?

UPDATE: This question can be ignored. I found my answer in the main thread.

Last edited 8 years ago by stevenkword (previous) (diff)

#4 in reply to: ↑ 3 @DrewAPicture
8 years ago

Replying to stevenkword:

Hi Drew,
What is the preferred way of dealing with spaces found within array keys? Should they be converted to underscores?

UPDATE: This question can be ignored if found my answer in the main thread.

For anyone wondering, the answer is that there's nothing wrong with using a docs-specific variable, even if it doesn't match up exactly with the array key it's referencing. Granted, that policy was put in place to handle non-standard values passed as parameters, e.g. $blog['site_id'] vs $blog_id but you get the idea.

@stevenkword
8 years ago

#5 @stevenkword
8 years ago

  • Uses lowercase @param variable names.
  • Uses spaces instead of tabs in hash notation.
  • Updates long descriptions for dynamic hooks and include examples.
  • Removes multisite-specific language from the long descriptions. While these hooks only fire in the network admin, they could be adapted for use elsewhere.
  • Updates short descriptions to describe where/when hooks fire rather than why.

@stevenkword
8 years ago

Updates formatting

#6 @stevenkword
8 years ago

Hi Drew,

I've made the suggested corrections and in an effort to improve readability, I've changed the tab formatting for the @params. I've edited the file in VIM and made sure I'm using real tabs instead of spaces. However, once I submit the patch to trac the formatting gets changed. What am I doing wrong?

UPDATE: I should have been using spaces for formatting instead of tabs

Last edited 8 years ago by stevenkword (previous) (diff)

@stevenkword
8 years ago

Updates formatting

@stevenkword
8 years ago

More Formatting

@stevenkword
8 years ago

6th time's the charm

#7 @stevenkword
8 years ago

  • Keywords has-patch added; needs-patch removed

#8 follow-up: @DrewAPicture
8 years ago

26252.6.diff looks pretty good. I'll get back to you with any changes soon.

#9 in reply to: ↑ 8 @stevenkword
8 years ago

Replying to DrewAPicture:

26252.6.diff looks pretty good. I'll get back to you with any changes soon.

Thanks Drew,

This is my first non-code patch to core and probably my first patch that will make its way into core. This means that I was both careful and clueless. Review away!

@DrewAPicture
8 years ago

Clean up

#10 follow-up: @DrewAPicture
8 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.8

I think we can get this one in under the wire. 26252.7.diff does some clean up from .6:

  • Fixed short descriptions for both hooks
  • The $theme parameter for the in_theme_update_message-{$theme_key} action is actually a WP_Theme object, and not an array
  • Shortened and clarified various parameter descriptions.

#11 @DrewAPicture
8 years ago

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

In 26540:

Inline documentation for hooks in wp-admin/includes/update.php.

Props stevenkword for the initial patches.
Fixes #26252.

#12 in reply to: ↑ 10 @stevenkword
8 years ago

Careful, but not careful enough! Good catch on the Theme object. Thanks drew!

Note: See TracTickets for help on using tickets.