WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#26869 closed task (blessed) (fixed)

Hook Docs clean up

Reported by: DrewAPicture Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: docs Cc:

Description (last modified by DrewAPicture)

We'll use this ticket as a clearing house fixes to the list of files found on make/docs: http://make.wordpress.org/docs/closed-hook-tickets-review/

Changes may include cleaning up any discrepancies in terms of spacing, code that was unnecessarily moved (such as in the case of filterable arrays, in-line @ tags, etc.

Attachments (18)

26869-25249.diff (2.0 KB) - added by DrewAPicture 4 years ago.
wp-comments-post.php [25249]
26869-25252.diff (2.2 KB) - added by DrewAPicture 4 years ago.
wp-admin/admin-footer.php [25252]
26869-25281.diff (859 bytes) - added by DrewAPicture 4 years ago.
xmlrpc.php [25281]
26869-25284.diff (939 bytes) - added by DrewAPicture 4 years ago.
wp-includes/wp-db.php [25284]
26869-25410.diff (4.4 KB) - added by DrewAPicture 4 years ago.
wp-includes/nav-menu-template.php [25410]
26869-hook-docs-audit-fixes.diff (10.6 KB) - added by kpdesign 3 years ago.
Hook docs audit recommended fixes
26869-audit.diff (10.0 KB) - added by DrewAPicture 3 years ago.
props kpdesign for most of this
26869-misc.php.diff (1.4 KB) - added by GaryJ 3 years ago.
load-back-compat.diff (629 bytes) - added by DrewAPicture 3 years ago.
Clarify comment for back-compat load-* hooks.
26869.diff (7.8 KB) - added by DrewAPicture 3 years ago.
signup_url.diff (2.0 KB) - added by DrewAPicture 3 years ago.
admin_post_actions.diff (1.3 KB) - added by DrewAPicture 3 years ago.
$action
do_feed.diff (918 bytes) - added by DrewAPicture 3 years ago.
$hook
perpage.diff (1.4 KB) - added by Otto42 3 years ago.
$per_page
screenid.diff (550 bytes) - added by Otto42 3 years ago.
manage_{$screen->id}_columns
perpage.2.diff (1.9 KB) - added by DrewAPicture 3 years ago.
edit_{$post_type}_per_page round 2.
admin_post_actions.2.diff (1.3 KB) - added by DrewAPicture 3 years ago.
admin_post_* actions round two.
26869.2.diff (2.2 KB) - added by DrewAPicture 3 years ago.
Four hooks for admin_post

Download all attachments as: .zip

Change History (60)

This ticket was mentioned in IRC in #wordpress-dev by jeremyfelt. View the logs.


4 years ago

#2 @jeremyfelt
4 years ago

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

@DrewAPicture
4 years ago

wp-comments-post.php [25249]

#3 @DrewAPicture
4 years ago

In 27144:

Fixes for inline documentation for hooks in wp-comments-post.php.

Adds missing @since versions, spacing, and language tweaks.

See #26869, #25229, [25249].

#4 @DrewAPicture
4 years ago

  • Description modified (diff)

@DrewAPicture
4 years ago

wp-admin/admin-footer.php [25252]

#5 @DrewAPicture
4 years ago

In 27145:

Fixes for hooks documentation in wp-admin/admin-footer.php.

Adds some new lines, some hook-line spacing, docs-specific variables, and other language tweaks.

See #26869, #25229, [25252].

@DrewAPicture
4 years ago

xmlrpc.php [25281]

#6 @DrewAPicture
4 years ago

In 27146:

Fixes for hooks documentation on xmlrpc.php.

Adds spacing, changes an @see to @link, adds a docs-specific variable for a parameter.

See #26869, #25229 and [25281].

@DrewAPicture
4 years ago

wp-includes/wp-db.php [25284]

#7 @DrewAPicture
4 years ago

In 27147:

Fixes for hooks documentation in wp-includes/wp-db.php.

See #26869, #25229 and [25284].

#8 @SergeyBiryukov
4 years ago

In 27192:

Correct @since value. see #26869.

@DrewAPicture
4 years ago

wp-includes/nav-menu-template.php [25410]

#9 @DrewAPicture
4 years ago

In 27201:

Fixes for hooks documentation wp-includes/nav-menu-template.php.

See #26869, #25229 and [25410].

@kpdesign
3 years ago

Hook docs audit recommended fixes

@DrewAPicture
3 years ago

props kpdesign for most of this

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


3 years ago

#11 @nacin
3 years ago

audit patch is good.

#12 @DrewAPicture
3 years ago

In 28083:

Priority fixes for various existing hook documentation.

Props kpdesign.
See #26869

@GaryJ
3 years ago

#13 @GaryJ
3 years ago

Small hook docs tweaks for wp-admin/includes/misc.php - order of tags, spacing between tags, and clearer English.

@DrewAPicture
3 years ago

Clarify comment for back-compat load-* hooks.

#14 @DrewAPicture
3 years ago

Added load-back-compat.diff which clarifies a comment on back-compat hooks of the load-* variety.

Notes and todo items from @kpdesign's audit(s):

Missing or incomplete docs for the following hooks:

  • wp_header_image_attachment_metadata
  • wp_create_file_in_uploads (duplicate)
  • Long description for do_mu_upgrade filter - "it is automatically fired" - what is fired?
  • get_attached_media

Suggested changes by file:

wp-admin/includes/menu.php:

  • Fix erroneous phpdoc tag for 3 hooks: _network_admin_menu, _user_admin_menu, _admin_menu.

wp-admin/includes/plugin-install.php:

  • Add missing docs variable for plugins_api hook.

wp-admin/network/admin.php:

  • Fix incorrect docs variable for redirect_network_admin_request hook (copy/pasta error).

wp-admin/options-discussion.php:

  • Change docs variable for thread_comments_depth_max hook to indicate what that number represents $max_depth.

wp-admin/options-writing.php:

  • Fix short description and add missing docs variable for enable_update_services_configuration hook.

wp-admin/options.php:

  • Fix short description and add missing docs variable for enable_post_by_email_configuration hook.

wp-admin/user/admin.php:

  • Fix short description, remove long description for redirect_user_admin_request hook.

wp-includes/class-wp-embed.php:

  • Add missing @since value for embed_oembed_discover hook.

wp-includes/class-wp-xmlrpc-server.php:

  • Document http_headers_useragent hook, added in r27872 (other hook docs originally added in r27730).

wp-includes/comment-template.php:

  • get_comment_excerpt missing docs
  • pings_open missing docs
  • the_permalink (twice as duplicates)
  • Add missing @since values for comments_open and get_comments_link hooks.

wp-includes/l10n.php:

  • Fix duplicate hook lines for locale and plugin_locale hooks.
  • Fix typo in short description for override_unload_textdomain hook.
  • Add missing docs variables for override_load_textdomain and override_unload_textdomain hooks, fix alignment due to change.
Last edited 3 years ago by kpdesign (previous) (diff)

@DrewAPicture
3 years ago

#15 @DrewAPicture
3 years ago

In 28207:

Ensure the nav_menu_meta_box_object filter hook is only documented once.

See #26869.

#16 @DrewAPicture
3 years ago

In 28209:

Ensure the style_loader_tag filter hook is only documented once.

See #26869.

#17 @DrewAPicture
3 years ago

In 28210:

Ensure the submitlink_box action hook is only documented once.

See #26869.

#18 @DrewAPicture
3 years ago

In 28211:

Ensure adjacency of a duplicate hook comment for a the_category filter call.

See #26869.

#19 @DrewAPicture
3 years ago

In 28212:

Ensure all duplicate calls of the wp_create_file_in_uploads filter are labeled as such.

See #26869.

#20 @DrewAPicture
3 years ago

In 28213:

Ensure the wp_edit_nav_menu_walker filter is only documented once.

See #26869.

#21 @DrewAPicture
3 years ago

In 28214:

Ensure the wp_set_comment_status action hook is only documented once.

See #26869.

#22 @DrewAPicture
3 years ago

In 28215:

Clean up duplicate hook notations and adjacency for calls to the wp_signup_location filter.

Also adds braces missed in [25535].

See #26869.

@DrewAPicture
3 years ago

$action

@DrewAPicture
3 years ago

$hook

#23 follow-up: @DrewAPicture
3 years ago

@nacin:

  • admin_post_actions.diff is a proposed attempt to handle the dynamic hook currently designated as $action
  • do_feed.diff is a proposed attempt to handle (one of several) dynamic hook(s) currently designated as $hook

#24 in reply to: ↑ 23 @GaryJ
3 years ago

Replying to DrewAPicture:

You've changed behaviour there I think. The old code allowed hooks of:

  • action_post (logged in, request action empty)
  • action_post_nopriv (not logged in, request action empty)
  • action_post_{$action} (logged in, request action populated)
  • action_post_nopriv_{$action} (not logged in, request action populated)

The new code appears to allow (re-ordered to match above):

  • admin_post_ (logged in, request action empty - different)
  • action_post_nopriv_ (not logged in, request action empty - different)
  • action_post_{$action} (logged in, request action populated)
  • action_post_nopriv_{$action} (not logged in, request action populated)

Now I've written it out, it looks like it's just the underscore before an empty ${action} that needs to be handled better.

#25 follow-up: @Otto42
3 years ago

For the admin_post one, I suggest something more like this:

if ( !empty($_REQUEST['action']) )
	$request_action = '_' . $_REQUEST['action'];

and then have both:

do_action( "admin_post_nopriv{$request_action}" );
do_action( "admin_post{$request_action}" );

In your if/else.

@Otto42
3 years ago

$per_page

@Otto42
3 years ago

manage_{$screen->id}_columns

#26 in reply to: ↑ 25 @DrewAPicture
3 years ago

Replying to Otto42:

For the admin_post one, I suggest something more like this:

if ( !empty($_REQUEST['action']) )
	$request_action = '_' . $_REQUEST['action'];

and then have both:

do_action( "admin_post_nopriv{$request_action}" );
do_action( "admin_post{$request_action}" );

In your if/else.

Yup, works for me. I was unsure about whether there would ever be a case of "admin_post_" (invalid or empty action), and whether that would actually constitute a separate, valid hook.

@DrewAPicture
3 years ago

edit_{$post_type}_per_page round 2.

#27 @DrewAPicture
3 years ago

screenid.diff and perpage.2.diff look good to me. Working on a new patch for the admin_post_ actions.

@DrewAPicture
3 years ago

admin_post_* actions round two.

#28 @DrewAPicture
3 years ago

In 28348:

Use interpolation instead of concatenation for the manage_{$screen->id}_columns hook name.

Props Otto42.
See #26869.

#29 @DrewAPicture
3 years ago

  • Summary changed from Hook Docs completed files fixes to Hook Docs clean up

For anyone interested, the reason we're converting several (possibly many) dynamic hook names to use interpolation instead of concatenation is due a need for consistency with the new code reference. Concatenated hook names are tougher to grok.

#30 @DrewAPicture
3 years ago

In 28349:

Convert and rename the $action hook in wp-admin/admin-post to two dynamic hook sets.

  • admin_post_nopriv{$request_action} is fired for logged-out users
  • admin_post{$request_action} is fired for logged-in users

Props Otto42, DrewAPicture.
See #26869.

Edit: s/$request_action/$action

Last edited 3 years ago by DrewAPicture (previous) (diff)

#31 @DrewAPicture
3 years ago

In the [28349] message, should be $action instead of $request_action in the hook names. Oops.

#32 @DrewAPicture
3 years ago

In 28350:

Revert [28349] in favor of retaining the single dynamic admin_post* hook.

See #26869.

#33 @DrewAPicture
3 years ago

In 28351:

Rename the $action hook in wp-admin/admin-post.php to admin_post{$action}.

Also, clarify documentation of priv vs nopriv prefixing of the hook name.

See #26869.

#34 @DrewAPicture
3 years ago

In 28352:

Minor phpDoc fixes for the got_rewrite, got_url_rewrite, and documentation_ignore_functions hooks.

Props GaryJ.
See #26869.

#35 @GaryJ
3 years ago

I'm not in favour of r28349 as it stands.

Looking at just one of them:

/** 
 * Fires the requested handler action for logged-out users. 
 * 
 * The dynamic portion of the hook name, $action, refers to the handler action. 
 * 
 * @since 2.6.0 
 */ 
do_action( "admin_post_nopriv{$action}" ); 

Imagine that is taken out of context and shown on devhub. That now looks like I can provide a handler action of foobar and I can hook into the admin_post_noprivfoobar hook. There's no mention that $action is given an underscore prefix in the hook doc, or perhaps worse, the hook name doesn't reflect that the (missing) underscore is static and always present.

I think here I'd rather see four explicit hooks that are clearly named and unambiguous, than two that are misnamed and don't appear to follow the naming consistency of other dynamic hooks.

@DrewAPicture
3 years ago

Four hooks for admin_post

#36 @DrewAPicture
3 years ago

In 28375:

Fix incomplete inline documentation for the wp_header_image_attachment_metadata filter.

See #26869.

#37 @DrewAPicture
3 years ago

In 28376:

Clarify documentation for *what* gets "automatically fired" when the do_mu_upgrade filter evaluates to true in Multisite.

See #26869.

#38 @DrewAPicture
3 years ago

In 28377:

Add a complete short description to the get_attached_media filter documentation.

See #26869.

#39 @DrewAPicture
3 years ago

In 28378:

Use a proper docs-specific variable for the first parameter passed to the embed_oembed_discover filter.

See #26869.

#40 @DrewAPicture
3 years ago

In 28393:

Clarify inline documentation for back-compat load-* action hooks.

See #26869.

#41 @DrewAPicture
3 years ago

In 28394:

Properly split and document the admin_post* actions into the following four hooks:

  • admin_post_nopriv – for logged-out requests lacking a supplied action
  • `admin_post_nopriv_$action – for logged-out requests with a supplied action
  • admin_post – for logged-in requests lacking a supplied action
  • admin_post_$action – for logged-in requests with a supplied action

See [28349], [28350], [28351].
See #26869.

#42 @DrewAPicture
3 years ago

  • Milestone changed from Future Release to 4.0
  • Resolution set to fixed
  • Status changed from new to closed

That should cover it for this ticket. Let's do new tickets for new issues.

Note: See TracTickets for help on using tickets.