WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 9 months ago

Last modified 9 months ago

#9272 closed enhancement (fixed)

Expand human_time_diff()'s abilities

Reported by: Viper007Bond Owned by: Viper007Bond
Milestone: 3.6 Priority: normal
Severity: normal Version: 2.8
Component: Date/Time Keywords: has-patch needs-testing rtl-feedback westi-likes
Focuses: Cc:

Description

This is partially related to #7250.

Currently human_time_diff() can only do days, hours, and minutes. I propose we expand it a bit, to at least weeks and possibly even months and years (although that can be tricky unless PHP has a shortcut).

It should also support the ability to trim to a certain accuracy, i.e. for a 2 year 1 month old post, minutes don't matter.

In short, I propose we integrate Time Since's functionality into the core by expanding the current human_time_diff().

Attachments (6)

9272.patch (2.5 KB) - added by Viper007Bond 5 years ago.
First pass
human_dif_time_test.php (518 bytes) - added by Viper007Bond 5 years ago.
Test script (drop in WP root)
9272.2.patch (3.3 KB) - added by Viper007Bond 2 years ago.
Patch refresh plus reverse order if RTL and add seconds boolean parameter
18495.patch (8.2 KB) - added by Viper007Bond 2 years ago.
Rewritten again. See comments below for details.
years.diff (767 bytes) - added by westi 17 months ago.
Simple patch to add years
9272.3.patch (1.9 KB) - added by SergeyBiryukov 11 months ago.

Download all attachments as: .zip

Change History (56)

comment:1 follow-up: GamerZ5 years ago

I did a plugin called WP-RelativeDate http://plugins.trac.wordpress.org/browser/wp-relativedate which does basically what you mentioned.

comment:2 in reply to: ↑ 1 Viper007Bond5 years ago

Replying to GamerZ:

I did a plugin called WP-RelativeDate http://plugins.trac.wordpress.org/browser/wp-relativedate which does basically what you mentioned.

Unless I'm reading it wrong, your plugin is very similar to the existing human_time_diff() except that it can do weeks ( days / 7 ) as well as Yesterday/Today.

Viper007Bond5 years ago

First pass

Viper007Bond5 years ago

Test script (drop in WP root)

comment:3 Viper007Bond5 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Milestone changed from Unassigned to Future Release
  • Version set to 2.8

Alright, there's a first pass at a patch. I've tested it and it seems to work well while retaining backwards compatibility (i.e. it should appear identical for existing usages).

However someone with translator knowledge, RTL specifically, needs to take a look at it. The part I'm not sure about is the joining of units as it goes "X years, X months, X days, etc.". The comma is translatable, but the order isn't. Does it need to be? I assume so. :/

comment:4 Denis-de-Bernardy5 years ago

  • Component changed from General to Date/Time

comment:5 Denis-de-Bernardy5 years ago

  • Component changed from Date/Time to i18n
  • Milestone changed from Future Release to 2.8

patch works fine in english.

moving this to i18n in case such an rtl tester is around.

comment:6 Viper007Bond5 years ago

  • Milestone changed from 2.8 to Future Release

This doesn't need to be rushed into 2.8.

comment:7 dd325 years ago

  • Component changed from i18n to Date/Time

Moving back to Date/Time too.. I dont like the idea of using the component field for things such as that..

comment:8 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.9

comment:9 Denis-de-Bernardy5 years ago

still applies clean

comment:10 cnorris234 years ago

Still works on revision 12,153 of trunk, but then again, I'm not running a localized rtl install.

comment:11 ryan4 years ago

  • Milestone changed from 2.9 to Future Release

comment:12 r-a-y3 years ago

Any movement on this? It makes sense to include months and years in human_time_diff().

BuddyPress has its own version of this in bp_core_time_since(). Would be better if Wordpress did this natively.

comment:13 DJPaul3 years ago

  • Cc DJPaul added

comment:14 nathanrice3 years ago

With a patch available, and a solution built into another project (BuddyPress), it seems odd that this hasn't been addressed for 3.2 yet.

comment:15 martinhover3 years ago

This code works well: http://wordpress.pastebin.com/11Rr8Sn4

Would be fantastic to have this implemented in next core upgrade.

comment:16 dd322 years ago

Seeing 336 days ago instead of 11 months ago is just lame.. I'd like to see us bring the BuddyPress version into core, or maybe viper's patch, given the BP function looks like it'd have internationalisation issues.

comment:17 nacin2 years ago

Could do this for 3.4. Viper007Bond, might you want to do a new pass at the patch given it is 3 years old, or is it good?

comment:18 Viper007Bond2 years ago

It's likely fine but I'll freshen it up a little bit, namely adding RTL support (array_reverse()).

Viper007Bond2 years ago

Patch refresh plus reverse order if RTL and add seconds boolean parameter

comment:19 Viper007Bond2 years ago

My patch could use feedback from translators, especially RTL ones.

comment:20 follow-up: pavelevap2 years ago

  • Cc pavelevap@… added

Related as regards localization: http://core.trac.wordpress.org/ticket/18495

Problem is that this function can be used in two different meanings (due to special inflection).

comment:21 in reply to: ↑ 20 Viper007Bond2 years ago

So some type of usage context parameter is needed for the function? That seems frustrating. :(

comment:22 pavelevap2 years ago

Yes, but I am not sure about exact solution. Our problem is that some plugins and themes use this function in context of freshness (5 days old) and other in context of history (5 days ago). In English there are only "days", but for some languages it is real problem, because of inflexon where "days" can be for example "dní", but also "dny" in other context.

comment:23 Viper007Bond2 years ago

Do you know how does other software handles this?

comment:24 dd322 years ago

If i was doing this, i'd have:

human_time_diff( $args ) {
  if ( ! is_array($args ) {
    $args = array();
    list($args['from'], $args['to']) = func_get_args();
  }
  $defaults = array('from' => null, 'to' => time(), 'labels' => ....
  ...
}

human_time_diff_ago($args) {
  $args['labels']['months'] = array( '%s month ago', '%s months ago' );
  ...
}
human_time_diff_until($args) {
  $args['labels']['months'] = array( 'in %s month', 'in %s months' );
  ...
}
human_time_diff_age($args) {
  $args['labels']['months'] = array( '%s month old', '%s months old' );
  ...
}

of course, that would still have the problem of 'slotting' a translated string into another string, not exactly ideal. Other chances that age|until|ago doesn't cover everything either that it'd be used in.

comment:25 Viper007Bond2 years ago

I like that solution and it's easier than having to fiddle with a filter. I'll swap over the args format. :)

Viper007Bond2 years ago

Rewritten again. See comments below for details.

comment:26 follow-up: Viper007Bond2 years ago

Alright, quite a few changes. :)

  • Now accepts arguments as an array. Props dd32.
  • Added "context" parameter that controls how the unit names are translated. Are more contexts needed? For example past versus future? Dumb English speaker here. :)
  • The separator can be controlled via the arguments, as can the unit labels.
  • Removed the units filter. This function won't work with units smaller than a second and I don't forsee people wanting to output "X decades". ;)
  • Add two helper functions per dd32's idea for the difference usage contexts. TODO: Go through existing usages of human_time_diff() in core and replace them with the context helper functions.

comment:27 Viper007Bond2 years ago

  • Keywords rtl-feedback added

comment:28 follow-up: petervanderdoes2 years ago

I would like to see an option to add a cut-off option.
What I mean is when I call the function and the difference is more as Y I just want to display the date. As mentioned the output of "X decades" is not foreseen, the cut-off option would give the option to the users how far the want to go with the display of X units.

The cut-off can be in seconds and when the diff in line 1958 is bigger output normal date format, or return false and have the developer check for false.
A cut-off value of false would keep the current functionality and that would be the default value.

comment:29 in reply to: ↑ 28 Viper007Bond2 years ago

Replying to petervanderdoes:

I would like to see an option to add a cut-off option.
What I mean is when I call the function and the difference is more as Y I just want to display the date. As mentioned the output of "X decades" is not foreseen, the cut-off option would give the option to the users how far the want to go with the display of X units.

The cut-off can be in seconds and when the diff in line 1958 is bigger output normal date format, or return false and have the developer check for false.
A cut-off value of false would keep the current functionality and that would be the default value.

This is currently how core uses the function, although it does it manually:

https://core.trac.wordpress.org/browser/tags/3.3.1/wp-admin/includes/class-wp-posts-list-table.php#L573

Supporting that through the function itself is an interesting proposal, but I'll wait to see what others think before adding any code.

comment:30 dd322 years ago

'years' => array( _x( '%d year', 'age' ), _x( '%d years', 'age' ) ),

I think we've got a helper for that here somewhere.... _nx_noop() and friends

return sprintf( $args['labels'][$smallest][0], 1 );

I believe that should pass through the same translation process just for consistency and filtering.

I would like to see an option to add a cut-off option.

For the first round here, I'm not sure it's needed, but I prefer the current methodology, If you pass something to it, you explicitly want a 'human time diff', If you want to display the date if it's older than a month, that should be handled before the function is called.

comment:31 dd322 years ago

I also don't see any harm on a filter on $units but if added, a protective sort should be applied (arsort($units, SORT_NUMERIC);).

It would open up to specific use-cases of using alternate durations, Although the only time I can see that being useful is non-jullian calenders.. and in those cases, the current fuzzy duration logic probably wouldn't be accurate enough for their use-case.

comment:32 dd322 years ago

I think we've got a helper for that here somewhere.... _nx_noop() and friends

It's not just a helper function btw, It ties the singular+plurals together for when the translations are being written in.

comment:33 Viper007Bond2 years ago

Just realized I used the wrong ticket number on the patch filename. Whoops.

Thanks dd32. I'll take a look. :)

comment:34 in reply to: ↑ 26 ; follow-up: westi2 years ago

Replying to Viper007Bond:

Alright, quite a few changes. :)

  • Now accepts arguments as an array. Props dd32.
  • Added "context" parameter that controls how the unit names are translated. Are more contexts needed? For example past versus future? Dumb English speaker here. :)
  • The separator can be controlled via the arguments, as can the unit labels.
  • Removed the units filter. This function won't work with units smaller than a second and I don't forsee people wanting to output "X decades". ;)
  • Add two helper functions per dd32's idea for the difference usage contexts. TODO: Go through existing usages of human_time_diff() in core and replace them with the context helper functions.

I like the extension of the functionality of the function here but not the way in which is is achieved.

I think we should aim to have the api look something like this:

  • human_time_diff( $from, $to = '' ) - current "duration" based behaviour - e.g. 5 days, 4 hours
  • human_time_diff_ago( $from, $to = '' ) - returns 5 days, 4 hours ago
  • human_time_diff_old( $from, $to = '' ) - returns 5 days, 4 hours old
  • __human_time_diff_helper( $args ) - private helper function which takes the $from, $to, $varients as arguments and is used to implement all three other functions.

This gives us a simpler external API, a simpler handler function because it gets the labels to use passed in and they can live in the calling functions, an easy to extend function if you want to use it in another context.

We also need a more correct way to handle RTL than array_reversing and crossing our fingers :)

Rather than taking an array of labels we should probably have an array of variants e.g:

  • %d days, %d seconds ago
  • %d hours, %d seconds ago

This gives translators full control.

comment:35 in reply to: ↑ 34 ; follow-up: Viper007Bond2 years ago

Replying to westi:

This gives us a simpler external API, a simpler handler function because it gets the labels to use passed in and they can live in the calling functions, an easy to extend function if you want to use it in another context.

Can you explain your preference for having all of the code live in a helper function and human_time_diff() becoming a wrapper for it? Is it to avoid the legacy argument code at the top of my patch or something else?

We also need a more correct way to handle RTL than array_reversing and crossing our fingers :)

Which is why my patch has the "rtl-feedback" tag. ;)

Rather than taking an array of labels we should probably have an array of variants e.g:

  • %d days, %d seconds ago
  • %d hours, %d seconds ago

This gives translators full control.

This would be a very, very long list of combinations, plus doubling it for context.

comment:36 in reply to: ↑ 35 westi2 years ago

Replying to Viper007Bond:

Replying to westi:

This gives us a simpler external API, a simpler handler function because it gets the labels to use passed in and they can live in the calling functions, an easy to extend function if you want to use it in another context.

Can you explain your preference for having all of the code live in a helper function and human_time_diff() becoming a wrapper for it? Is it to avoid the legacy argument code at the top of my patch or something else?

My preference is because as an end-user of these functions they are much more friendly and usable if I can just call them with one/two timestamps as arguments rather than having to wrap up and array of named arguments.

$args as a single argument is really useful when you have lots of optional arguments but also makes it harder for less experience developers to use things and should be reserved for more "complex" functions than human_time_diff().

I don't think we should be using $args unless we have:

  • Many optional arguments which we expect 80%+ of callers to specify a least one of.

comment:37 Viper007Bond2 years ago

I intended for all of the functions in my patch to accept ( $from, $to ) but just now realized that the func_get_arg() code won't pass through the wrapper functions. That was an oversight on my part and I agree totally. Being forced to construct an array is annoying and was not my intention (it was supposed to be optional).

Version 0, edited 2 years ago by Viper007Bond (next)

comment:38 l3rady2 years ago

  • Cc scott@… added

Would love to see a new human_time_diff added to core. I went about writing my own function based of the original which you can find here: http://pastebin.com/7zffa3Wn

I wish I had looked here first before writing this. Anyway take a look at my simple approach that is working for me, maybe you can use some of it.

comment:39 follow-ups: nacin22 months ago

Just leaving a note here: From an i18n perspective, "X ago" and "X old" will require two different translations for X in a number of languages.

Perhaps we can start by making human_time_diff() operate on more than just "days" -- expanding to weeks, and months, years perhaps.

westi17 months ago

Simple patch to add years

comment:40 in reply to: ↑ 39 westi17 months ago

Replying to nacin:

Just leaving a note here: From an i18n perspective, "X ago" and "X old" will require two different translations for X in a number of languages.

Agreed

Replying to nacin:

Perhaps we can start by making human_time_diff() operate on more than just "days" -- expanding to weeks, and months, years perhaps.

This came up again for years and so I put a quick patch together for them :)

comment:41 westi17 months ago

  • Keywords westi-likes added

comment:42 in reply to: ↑ 39 Viper007Bond17 months ago

Replying to nacin:

Just leaving a note here: From an i18n perspective, "X ago" and "X old" will require two different translations for X in a number of languages.

Just noting that 18495.patch (oops, wrong ticket number in filename) supports this as it has a context parameter for age vs. duration.

comment:43 iandunn13 months ago

  • Cc ian_dunn@… added

comment:44 follow-up: nacin11 months ago

  • Milestone changed from Future Release to 3.6

We're now using human_time_diff() in the admin for revisions, which makes this a bit more obvious (it was previously only used in fairly hidden areas, like $mode = excerpt in some of the list tables).

I'd like to add weeks, months, years, using the simple approach from westi. We can consider a bigger overhaul that includes better i18n in 3.7?

SergeyBiryukov11 months ago

comment:45 in reply to: ↑ 44 ; follow-up: SergeyBiryukov11 months ago

Replying to nacin:

I'd like to add weeks, months, years, using the simple approach from westi.

Done in 9272.3.patch.

Also changed the conditions a bit. Currently, when the difference is exactly one hour, the function returns "60 mins". With the patch, it returns "1 hour".

comment:46 greenshady11 months ago

  • Cc justin@… added

comment:47 in reply to: ↑ 45 ; follow-up: Nessworthy11 months ago

There is also an opportunity for consideration of specifics here.

In 9272.3.patch, your expected result will always be in one unit of time.
E.g. 9 hours or 6 years, and it doesn't get any more specific than that.

What if an additional parameter was/additional paramteters were introduced to be able to dig down in to a more specific time period depending on what is passed?

For example, depending on what you pass into it, if the time reaches years, it will return 5 years, 3 months, but if it only stays in months, it will only return 5 months.

A third parameter, something like $breakdown_as_array could cause the function to return an array like:

Array (
    'years' => 5,
    'months' => 2,
    'days' => 15,
    'hours' => 3,
    'minutes' => 55,
    'seconds' => 10
)

Just some food for thought. Although the last idea does take away the returned value from being immediately readable without using additional code.

Edit: A filter could actually be added here, allowing themes and plugins to directly modify the output. If something like the array above is passed through a filter, with the expectations of a formatted string as the result, it would be fairly easy to modify without going through core code.

And, of course, if the filter is unused, it can default back to it's regular usage. This could also make the third parameter be something like $apply_filter instead.

Last edited 11 months ago by Nessworthy (previous) (diff)

comment:48 in reply to: ↑ 47 Viper007Bond11 months ago

Replying to Nessworthy:

There is also an opportunity for consideration of specifics here.

I think that seems overly complicated. My patch already supports controlling the number of outputted units and a theme/plugin is welcome to do it's own additional logic to decide it's own number of units value.

The patch you reference was specifically written to be a simple improvement to the function as a short term fix. Larger fixes and improvements are contained within my patch. See the above comments for details.

comment:49 nacin9 months ago

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

In 24582:

Expand human_time_diff() from minutes/hours/days to also include weeks/months/years. Fix off-by-one issue.

props SergeyBiryukov, westi.
fixes #9272.

comment:50 nacin9 months ago

Given [24582] for 3.6, two options here:

  • New ticket for a better human_time_diff() API, for a future release.
  • Re-open this ticket and move it to the future milestone.

Seems like a fresh start in a new ticket wouldn't be bad.

Note: See TracTickets for help on using tickets.