WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#31248 closed defect (bug) (fixed)

PHPDoc improvements for wp-admin/includes/template.php

Reported by: ipm-frommen Owned by: DrewAPicture
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: docs Cc:

Description

Originally, I just wanted to replace the missing short description of the get_inline_data function with the comment directly placed above the PHPDoc block. But then I fixed several other things that either my IDE complained about or that I just saw while plowing through the code.

Applying the attached patch does the following:

  • add short description to get_inline_data function;
  • replace default value '1' of int variable $position in wp_comment_reply function with 1;
  • replace empty return; statements for _list_meta_row function with @return string PHPDoc comment with return '';;
  • replace default value false of string variable $selected in wp_dropdown_roles function with '';
  • replace loose comparison with strict string comparison in wp_dropdown_roles function;
  • remove unused (last!) argument $limit_styles in iframe_header function (as well as the according PHPDoc comment);
  • remove unused globalization of $current_user variable in iframe_header function;
  • add missing @return string PHPDoc comment for get_submit_button function;
  • replace default values null of string variable $text and array variable $other_attribues in get_submit_button function with appropirate values (i.e., '' and array());
  • add missing @param PHPDoc comments for enqueue_scripts and dismiss_pointers_for_new_users functions.

Attachments (4)

31248.patch (3.8 KB) - added by ipm-frommen 6 years ago.
31248-2.patch (3.9 KB) - added by ipm-frommen 6 years ago.
Mark parameter deprecated/unused instead of removing it
31248-3.patch (5.5 KB) - added by ipm-frommen 6 years ago.
31248-4.patch (5.4 KB) - added by ipm-frommen 6 years ago.

Download all attachments as: .zip

Change History (17)

@ipm-frommen
6 years ago

#1 @ipm-frommen
6 years ago

  • Keywords has-patch added

#2 @ipm-frommen
6 years ago

I also created a Pull Request on GitHub.

EDIT: Just for your information, I closed the PR. Optimistic as I am sometimes, I just thought Pull Requests would already be accepted. It's been a while since WordCamp San Francisco, right...? :)

Last edited 6 years ago by ipm-frommen (previous) (diff)

@ipm-frommen
6 years ago

Mark parameter deprecated/unused instead of removing it

#3 @ipm-frommen
6 years ago

The new patch does almost exactly what the first one does. The only difference is that it marks the unused parameter in the iframe_header function unused/deprecated and does not remove it.

Besides that, is there anything wrong with this ticket?

Last edited 6 years ago by ipm-frommen (previous) (diff)

#4 follow-ups: @DrewAPicture
6 years ago

Just a cursory glance at the patch reveals that there are code changes mixed in with doc changes. Pretty much all the cases where you've changed default values for parameters or expected returns within functions would require also having accompanying unit tests.

So I would suggest splitting the docs changes from the code changes into separate patches and probably separate tickets. we're trying to move away from these types of general-purpose optimizations unless it's a blessed task.

#5 in reply to: ↑ 4 @ipm-frommen
6 years ago

Replying to DrewAPicture:

there are code changes mixed in with doc changes. Pretty much all the cases where you've changed default values for parameters or expected returns within functions would require also having accompanying unit tests.

Well, what these changes are all about is that the overall behavior of the code/functions does not change at all, which means that the existing unit tests are all working absolutely fine.

I would suggest splitting the docs changes from the code changes into separate patches and probably separate tickets.

Okay, that's why I asked if something was wrong; I silently assumed something like this. I will update ticket and patch, and open a new ticket (or maybe two) for parts of the changes.

I just wasn't sure what was best: one ticket handling a bunch of changes in a single file, or several tickets for different changes in the same file. Anyway, now I know what you prefer. Thanks. :)

#6 @ipm-frommen
6 years ago

  • Focuses template removed
  • Summary changed from Optimizations for wp-admin/includes/template.php to PHPDoc improvements for wp-admin/includes/template.php

@ipm-frommen
6 years ago

#7 in reply to: ↑ 4 @ipm-frommen
6 years ago

Replying to DrewAPicture:

I would suggest splitting the docs changes from the code changes...

I just did that.

#8 @SergeyBiryukov
6 years ago

  • Component changed from General to Administration

#9 in reply to: ↑ 4 @ipm-frommen
6 years ago

Replying to DrewAPicture:

I would suggest splitting the docs changes from the code changes...

Is there anything unclear with this ticket?

I edited the title, but since I cannot edit the ticket description, it might not be clear what the ticket now is about. The last patch only contains PHPDoc changes. Any other aspect has been taken care of in different tickets already.

#10 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 4.2
  • Owner set to DrewAPicture
  • Status changed from new to reviewing

#11 @DrewAPicture
6 years ago

  • Keywords needs-refresh added

Looks like the patch is going to need a refresh. Sorry I didn't get to reviewing it in time.

@ipm-frommen
6 years ago

#12 @ipm-frommen
6 years ago

  • Keywords needs-refresh removed

#13 @DrewAPicture
6 years ago

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

In 31599:

Clean up and refine DocBlocks for a variety of functions and methods in wp-admin/includes/template.php.

  • Also documents the default arguments of wp_terms_checklist() as a hash notation.

Props ipm-frommen, DrewAPicture.
Fixes #31248.

Note: See TracTickets for help on using tickets.