WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#38773 closed defect (bug) (fixed)

Calculation error in human_time_diff

Reported by: SGr33n Owned by: adamsilverstein
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Date/Time Keywords: has-unit-tests commit
Focuses: Cc:

Description

Hi,
a few days ago I used human_time_diff() for the first time. It was in a wp-cron task manager, to check when the last run happened. Now the human_time_diff is rounded by php round, this means that if 2h and 1min ago it returns 2 hours ago, but if 2h, 30m and 1sec it returns 3 hours... and this is not true, because 3 hours haven't passed... and this also happens on 2 years and 1 hour (3 years?). So there is a patch to correct this, even if you want to correct it.

Thanks :)

Attachments (5)

38773.diff (1.8 KB) - added by SGr33n 5 years ago.
38773.2.diff (3.7 KB) - added by adamsilverstein 5 years ago.
38773.3.diff (3.7 KB) - added by adamsilverstein 5 years ago.
38773.4.diff (2.0 KB) - added by adamsilverstein 4 years ago.
38773.5.diff (2.1 KB) - added by adamsilverstein 4 years ago.

Download all attachments as: .zip

Change History (14)

@SGr33n
5 years ago

#1 @swissspidy
5 years ago

  • Keywords has-patch needs-unit-tests added

#2 @ocean90
5 years ago

  • Version trunk deleted

Introduced in [1976].

Related: #9272, #29849

#3 @adamsilverstein
5 years ago

@SGr33n good catch, thanks for opening the ticket.

I did some testing and can see that a difference of 2h, 30m and 1sec returns 3 hrs, however for 2 years and 1 hour the function returned 2 years. To get 3 years I had to use 2 years six months and a day.

After my testing, I'm not sure floor is actually better there than round, for example if 1 year 364 days have elapsed, floor will return 1 year which seems off. Furthermore, the existing behavior has been in core for 12 years, and changing it is likely to impact users who expect it to work as is. Finally, the human_time_diff filter applied on the returned value enables developers to adjust the default behavior for all uses.

In 38773.2.diff I added a few unit tests to verify the rounding behavior. All three assertions in test_rounding will fail before with round vs floor because they round up.

38773.3.diff is the unit tests only, adjusted to match the current core behavior, these will break if the patch is applied.

#4 @SGr33n
5 years ago

Hi @adamsilverstein, thanks :) you're right, I was wrong to write, It was 2 years 6 months and 1 day that returned 3 years.
So I will follow the thread to see how this will evolve.

#5 @adamsilverstein
4 years ago

38773.4.diff
Add unit tests verifying the current rounding behavior of human_time_diff.

@SGr33n We aren't going to change this long-established behavior, by now the rounding is expected. if you want to change the core behavior to using floor, you can use the filter that runs at the end of the function, add something like this.

#6 @adamsilverstein
4 years ago

  • Keywords has-unit-tests added; has-patch needs-unit-tests removed

#7 @adamsilverstein
4 years ago

  • Keywords commit added

In 38773.5.diff:
Add some additional test cases around the rounding margins.

#8 @adamsilverstein
4 years ago

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

In 41018:

Date/Time: Add unit tests for the human_time_diff function.

Verify that human_time_diff works as expected for minute, hour and day intervals. Test that rounding works as expected when the time difference is near the rounding margin.

Props SGr33n.
Fixes #38773.

#9 @adamsilverstein
4 years ago

  • Milestone changed from Awaiting Review to 4.9
Note: See TracTickets for help on using tickets.