WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#25531 closed defect (bug) (fixed)

Hook Docs: wp-includes/capabilities.php

Reported by: ptahdunbar Owned by: SergeyBiryukov
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Inline Docs Keywords: has-patch
Focuses: Cc:
PR Number:

Description

Attached patch adds phpdocs for all hooks in wp-includes/capabilities.php. Please review to confirm wording/correctness :)

Attachments (2)

ticket-25531.diff (4.9 KB) - added by ptahdunbar 6 years ago.
25531.1.diff (3.3 KB) - added by kpdesign 6 years ago.
Second pass

Download all attachments as: .zip

Change History (9)

#1 @rzen
6 years ago

Ptah, solid work!

Overall

  • I suggest renaming several params below, all because of how these docs will be read from outside the codebase. I know you named them to match what is literally being passed, but someone referencing these params in their own functions will have to rename them or else face errors of attempting to reference objects that don't exist. Therefore, I think it's most sane that we suggest logical param names up front.

role_has_cap

  • Simply the @param variable names (e.g. $this->capabilities becomes $capabilities)

set_user_role

  • Instead of "Perform an action after..." we could simply say "Fires after..."
  • Rename $this->ID param to $user_id.

user_has_cap

  • You're welcome to bend the 80 character margin when its just pushing a single widowed word to a new line :) (http://d.pr/i/1E1b)
  • Rename $this->allcaps param to $allcaps
  • Rename $this param to $user

auth_post_meta_{$meta_key}

  • You're welcome to bend the 80 character margin in the short description here, too. It's a bit jarring to have"post." on a line all by its lonesome.
  • The first @param should be named something, I suggest $allowed.
  • The $post->ID param can be renamed to $post_id.

#2 @rzen
6 years ago

  • Keywords 2nd-opinion added

I'd like to see what others have to say about my recommendations for set_user_role, user_has_cap and auth_post_meta_{$meta_key} in regards to naming the params different from the actual variables being used in the filter call.

My feeling is that it makes the docs more human-readable when separated from the context of the code, with the obvious trade-off that the pairing becomes awkward when viewed in code (and possibly within an IDE). This may be a good topic to discuss during a weekly Inline Docs meeting.

#3 @rzen
6 years ago

The result from today's inline-docs meeting in #wordpress-sfd is that yes, we do want to use alternate variable names whenever a param is not cast to a variable or whenever its a class property of some kind. It is a little strange to see it side-by-side with the code, but its of much greater benefit when read outside the code.

#4 @DrewAPicture
6 years ago

  • Keywords needs-patch added; has-patch 2nd-opinion removed

@kpdesign
6 years ago

Second pass

#5 @kpdesign
6 years ago

  • Keywords has-patch added; needs-patch removed

25531.1.diff incorporates changes from patch review in comment:1, and corrected @since values.

#6 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 3.8

#7 @SergeyBiryukov
6 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 26126:

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

props ptahdunbar, kpdesign.
fixes #25531.

Note: See TracTickets for help on using tickets.