WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#26874 closed defect (bug) (fixed)

timer_stop() returns a string, not a float

Reported by: GregLone Owned by: jackreichert
Milestone: 3.9 Priority: normal
Severity: minor Version: 3.9
Component: Bootstrap/Load Keywords: needs-patch good-first-bug
Focuses: docs Cc:
PR Number:

Description

Hello.
The inline doc indicates "@return float" for the function timer_stop() but it should be "@return string".
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/load.php#L220

Attachments (2)

load.php.diff (675 bytes) - added by jackreichert 6 years ago.
load.php.2.diff (757 bytes) - added by jackreichert 6 years ago.
more clarity added in description

Download all attachments as: .zip

Change History (12)

#1 @DrewAPicture
6 years ago

  • Keywords needs-patch good-first-bug added

#2 follow-up: @nacin
6 years ago

I just want to clarify that there's an alternative to this ticket: That the docs are correct and the code is wrong. microtime( true ) will return a float. So why is it a string?

First: This is an "OK" good-first-bug. It is both trivial and unclear as to the desired direction. good-first-bugs should be self-contained, but if they take only 3 seconds, it's not a great first bug (and someone will just come along and fix it first). On the other hand, since this is particular to inline docs (more in a moment), then it could of course benefit from a lower barrier to entry.

Back to the alternative reality: It would be prudent to ask if it should return a float or a string. In this case, it is clear that a string is desired due to the use of number_format(_i18n). Thus changing the docs do appear to be in order. This would benefit from an explanation in @return that the number is formatted for human consumption (versus profiling) — it is both localized and rounded.

#3 in reply to: ↑ 2 @DrewAPicture
6 years ago

Replying to nacin:

On the other hand, since this is particular to inline docs (more in a moment), then it could of course benefit from a lower barrier to entry.

Back to the alternative reality: It would be prudent to ask if it should return a float or a string. In this case, it is clear that a string is desired due to the use of number_format(_i18n). Thus changing the docs do appear to be in order. This would benefit from an explanation in @return that the number is formatted for human consumption (versus profiling) — it is both localized and rounded.

Yes, exactly. I'm fairly certain you don't want to get into the further fragmentation of good-first-docs-bug and on we go down the rabbit hole.

I think if a ticket is the Inline Docs component and marked good-first-bug that should prove to be more than enough of a hint of the type of effort, thought, and patch required :)

@jackreichert
6 years ago

more clarity added in description

#4 @jackreichert
6 years ago

Per the discussion above I patched the documentation for the function to accurately match the functionality and explain why.

#5 @nacin
6 years ago

  • Component changed from Inline Docs to Bootstrap/Load
  • Focuses docs added

#6 @nacin
6 years ago

  • Milestone changed from Awaiting Review to 3.9
  • Owner set to jackreichert
  • Status changed from new to assigned

Hi jackreichert, it looks like your diffs are "backwards". It's suggesting to remove what isn't there and add what is. A simple git diff (or preferably git diff --no-prefix) should handle this in the proper order.

#7 @johnbillion
6 years ago

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

In 27332:

Correct the inline docs for timer_stop(). Fixes #26874. Props jackreichert

#8 follow-up: @juliobox
6 years ago

Props GregLone* ?

#9 in reply to: ↑ 8 @johnbillion
6 years ago

Replying to juliobox:

Props GregLone* ?

The props generally goes to the person who writes the patch, but in this particular case, yes the props could have gone to GregLone also. I'll bear it in mind in the future.

#10 @DrewAPicture
6 years ago

In 27335:

Inline documentation fixes for timer_stop().

See #26874.

Note: See TracTickets for help on using tickets.