Make WordPress Core

Opened 10 years ago

Last modified 9 days ago

#29849 assigned enhancement

Better human_time_diff()

Reported by: kovshenin's profile kovshenin Owned by: pbearne's profile pbearne
Milestone: 6.8 Priority: normal
Severity: normal Version:
Component: Date/Time Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Given #9272 #18495 and #27271 I think it's time for a better alternative to human_time_diff(). My proposal is an entirely new function which:

  • Should be simple - no bazillion-dimensional arrays for arguments
  • Should not lack context in translatable strings - "from now", "ago" should be part of the resulting strings
  • Should have consistent strings, no abbreviations such as mins, yrs, hrs, secs
  • Should assume all timestamps have no offsets

Attachments (3)

29849.diff (2.3 KB) - added by kovshenin 10 years ago.
29849.2.diff (11.2 KB) - added by akirk 9 years ago.
29849-codebase-replacements.diff (4.7 KB) - added by akirk 9 years ago.

Download all attachments as: .zip

Change History (19)

@kovshenin
10 years ago

#1 @kovshenin
10 years ago

  • Keywords has-patch dev-feedback added

First pass in 29849.diff.

#2 @SergeyBiryukov
10 years ago

  • Component changed from General to Date/Time

#3 @johnbillion
10 years ago

  • Keywords dev-feedback removed
  • Type changed from defect (bug) to enhancement

A few notes on 29849.diff:

  • The $to argument in human_time_diff() should be retained. Removing it makes it less functional.
  • $diff should be included in the wp_natural_time filter arguments.
  • I'd like to see some places this can be used in core in place of human_time_diff() to improve the i18n.
  • For this to be really useful it should support timestamps in MySQL format, so you can plug in dates straight from the post_date column.

@akirk
9 years ago

#4 @akirk
9 years ago

Maybe we can add some momentum to this again: human_time_diff() is often used in a wrong way, beginning with the example in the Codex: it suggests a string concattenation of ago:

<?php echo human_time_diff( get_the_time('U'), current_time('timestamp') ) . ' ago'; ?>

I'd argue that this is the only way that people use human_time_diff(), many don't know or care that a printf( __( '%s ago' ), human_time_diff( $timestamp ) ); is needed, and use the . ' ago' from the example.

wp_natural_time() would handle this out of the box, while including the other optimizations by @kovshenin. I have changed one thing in the behavior though: it only allows adjacent time units to avoid output like "1 year, 3 minutes ago".

I have added two diffs, one that shows where human_time_diff() could be replaced in the current codebase, the other one incorporates the suggestions by @johnbillon and also adds unit tests.

I'd also suggest to deprecate human_time_diff() and suggest the usage of this new function, also update the Codex and remove the misleading example of string concattenation.

#5 @GaryJ
9 years ago

Codex updated. I've added the internationalized version as an extra example to the one that was already there, since the point isn't to document how to incorporate internationalized strings, but show a more basic usage which can then be built upon.

http://codex.wordpress.org/Function_Reference/human_time_diff#Examples

#6 @akirk
9 years ago

Thank you very much for picking up the ball! I don't want to start a edit war in the Codex, so maybe we can discuss here: in general I think the context is not the place to explain variables, this should be done in a /* translators: */ comment.

Also I am unsure if adding a text-domain to the example is really helpful, part of the idea of wp_natural_time() is to leverage the common text usage (and its translation) of "%s ago".

Last but not least, in my opinion the "internationalized" version should be the only way to use it, we get the "%s ago" string in a translated version for free through WordPress, there is no need to advocate the usage of the concattenating version.

So I'd suggest to simply change the code example in the codex to this:

To print an entry's time ("2 days ago"):
<?php printf( __( '%s ago' ), human_time_diff( get_the_time('U'), current_time('timestamp') ) ); ?>
For comments:
<?php printf( __( '%s ago' ), human_time_diff( get_comment_time('U'), current_time('timestamp') ) ); ?>

#7 @DrewAPicture
9 years ago

I think we're getting to a point now where effort would be best served keeping the inline and curated docs associated with the code reference page updated, rather than the Codex.

@GaryJ: Would you mind also submitting the example(s) to the code reference article? We're just going to have to move it over manually from the Codex anyway.

https://developer.wordpress.org/reference/functions/human_time_diff

#8 @Viper007Bond
9 years ago

I'd encourage you all to take another look at #18495. For purposes of translation, we may be required to support a context parameter.

When I rewrote human_time_diff() for #9272, it got complicated fast but I'm not sure it can be avoided. See the switch ( $args['context'] ) { and following code in my patch:

https://core.trac.wordpress.org/attachment/ticket/9272/18495.patch

#9 @desrosj
4 years ago

  • Keywords needs-refresh added
  • Milestone set to Awaiting Review

The latest patch needs to be refreshed with today's code base.

@rarst would you happen to be interested in this or any thoughts to share for someone else to get started?

#10 @Rarst
4 years ago

  • Owner set to Rarst
  • Status changed from new to accepted

Yeah, I need to do a pass on remainder of Date/Time stuff.

#11 @pbearne
4 weeks ago

  • Owner changed from Rarst to pbearne
  • Status changed from accepted to assigned

This ticket was mentioned in PR #7766 on WordPress/wordpress-develop by @pbearne.


3 weeks ago
#12

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

Introduce the wp_natural_time function to format time differences in a human-readable manner. Add extensive PHPUnit tests to cover various time intervals and edge cases for robust validation.

This ticket was mentioned in PR #7767 on WordPress/wordpress-develop by @pbearne.


3 weeks ago
#13

follow up patch with core changes

#14 @pbearne
3 weeks ago

@desrosj, I have refreshed this patch. Can we merge this?

#15 @pbearne
2 weeks ago

  • Milestone changed from Awaiting Review to Future Release

#16 @pbearne
9 days ago

  • Milestone changed from Future Release to 6.8
Note: See TracTickets for help on using tickets.