Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#44224 closed defect (bug) (fixed)

Missing @return in Doc comment

Reported by: subrataemfluence's profile subrataemfluence Owned by: subrataemfluence's profile subrataemfluence
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.1
Component: Plugins Keywords: has-patch good-first-bug needs-refresh
Focuses: docs Cc:


File name: wp-admin/includes/plugin.php

Function/Methods missing @return in Doc comment

get_plugin_page_hookname and user_can_access_admin_page

I have uploaded a patch.

Attachments (2)

44224.diff (785 bytes) - added by subrataemfluence 6 years ago.
44224-2.diff (1.0 KB) - added by subrataemfluence 6 years ago.

Download all attachments as: .zip

Change History (16)

#1 @desrosj
6 years ago

  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to subrataemfluence
  • Status changed from new to assigned

@subrataemfluence good catch!

Can we also add function and parameter descriptions, which are also missing? The return value should have a description as well.

Marking as good-first-bug and assigning as to mark as claimed.

Related: #42505.

#2 @subrataemfluence
6 years ago

Sure! I would love to :) Thank you for assigning me the task.

#3 @desrosj
6 years ago

  • Component changed from General to Plugins
  • Focuses docs added; coding-standards removed

#4 @netweb
6 years ago

  • Keywords needs-refresh added; 2nd-opinion removed

#5 @subrataemfluence
6 years ago

@desrosj Please let me know if the patch looks ok!

#6 @SergeyBiryukov
6 years ago

  • Milestone changed from 5.0 to 5.0.1

#7 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#8 @pento
6 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#9 @audrasjb
6 years ago

  • Keywords commit added; needs-refresh removed

This ticket is triaged in milestone 5.0.3. The patch looks good, small enough and self contained so it would be ok to land in the next minor. Adding the commit keyword to see if it can land in 5.0.3.

#10 @desrosj
6 years ago

  • Keywords needs-refresh added; commit removed
  • Milestone changed from 5.0.3 to 5.1

Moving this to 5.1.

One more nit. I think Required. is implied when there is no default value specified for a parameter, so I don't think we need that.

I looked through core and only found a handful of @param tags with Required.. They seem to fall into a few scenarios:

  • The parameter has a default value specified.
  • It is a required parameter within an array and others are optional (Although the handbook seems to favor marking the optional parameters instead).
  • Parameters that are required, but may have scenarios that call for falsey/empty values to be passed.
  • Functions where there are a mix of optional and required parameters.

Clarifying appropriate use of Required. in the documentation handbook may be helpful.

#11 @subrataemfluence
6 years ago

Thank you @desrosj. I am reading the handbook.

#12 @subrataemfluence
6 years ago

@desrosj, None of $plugin_page and $parent_page arguments has a default value set.

Now since get_admin_page_parent function argument has a default value (empty string) set for $parent, I think adding a default value (empty string) for $parent_page might worth. By doing this we can keep this argument optional.

As far as $plugin_page argument is concerned, it has no default value set, so to my understanding, Required should be used for this. In other words, again, to me understanding, argument doesn't have a default value specified should probably be declared as Required.

I would suggest the following modification in Doc block and argument list:

 * @global array $admin_page_hooks
 * @param string $plugin_page. Name of the page generated by plugin. Required.
 * @param string $parent_page. Name of the parent page of $plugin_page.

function get_plugin_page_hookname( $plugin_page, $parent_page = '' ) {
   $parent = get_admin_page_parent( $parent_page );

Please let me know if it makes sense!

#13 @pento
6 years ago

  • Milestone changed from 5.1 to 5.2

#14 @SergeyBiryukov
5 years ago

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

In 45085:

Docs: Improve documentation for get_plugin_page_hook() and get_plugin_page_hookname().

Props subrataemfluence, desrosj.
Fixes #44224.

Note: See TracTickets for help on using tickets.