Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#33397 closed enhancement (fixed)

Add constant MONTH_IN_SECONDS

Reported by: toscho's profile toscho Owned by: egill's profile egill
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: General Keywords: good-first-bug needs-docs has-patch
Focuses: Cc:

Description

When the time constants were introduced in #20987, MONTH_IN_SECONDS wasn't included because the core didn't it need at that time, and each month has a different length, so it would never be accurate.

I think we should rethink that decision:

  1. Now, we do use 30 * DAY_IN_SECONDS in human_time_diff() to calculate a "month". All the other time units are expressed in pure constants in that function, this one stands out. This is inconsistent.
  1. The other values aren't accurate either, (think DST, leap years/seconds, see Falsehoods programmers believe about time to get some ideas). Precision never was the point of these constants, their value is convenience. If you need an accurate time, use better tools like DateTime.
  1. Global constants are an API. An API has to be predictable. Right now, we always have to remember the "missing interval", or we have to add it in our plugins. That's not good.

My suggestion is to use a rounded value of 30 days like human_time_diff() does already, and maybe add a note to wp_initial_constants() reminding developers about potential problems with these simplified values.

Attachments (6)

33397.patch (539 bytes) - added by egill 9 years ago.
33397.2.patch (2.4 KB) - added by egill 9 years ago.
33397.3.patch (1.2 KB) - added by egill 9 years ago.
Take 3 :)
33397.4.patch (1.2 KB) - added by egill 9 years ago.
it's been a long day
33397.5.patch (1.3 KB) - added by egill 9 years ago.
33397.6.patch (724 bytes) - added by egill 9 years ago.
wrapping constants in a DocBlock template

Download all attachments as: .zip

Change History (22)

#1 @peterwilsoncc
9 years ago

  • Keywords needs-patch good-first-bug added

@egill
9 years ago

#2 @egill
9 years ago

  • Keywords has-patch added; needs-patch removed

#3 @DrewAPicture
9 years ago

  • Owner set to egill
  • Status changed from new to assigned

Marking the good-first-bug as "claimed".

#4 follow-up: @jorbin
9 years ago

I would love to see some inline docs here pointing out that these times are not fully accurate (especially for month) and a pointer to http://php.net/manual/en/class.datetime.php if a developer needs to be more accurate.

#5 @DrewAPicture
9 years ago

  • Keywords needs-docs added

#6 in reply to: ↑ 4 @egill
9 years ago

Something like this?

/**
 * Constants for expressing human-readable intervals
 * in their respective number of seconds.
 *
 * Please note that the values are not accurate and provided
 * for convenience, especially MONTH_IN_SECONDS!
 *
 * If you need more accuracy please consider using the DateTime class (http://php.net/manual/class.datetime.php).
 */

#7 @jorbin
9 years ago

The first and third sentences look really good. The second one I think could work better if it was more possitive. What do you think of:

Please note that these values are approximate and are provided for convenience. For example, MONTH_IN_SECONDS wrongly assumes every month has 30 days.

#8 @egill
9 years ago

Definitely. I feel a bit out of sync with the WordPress documentation. Did read the documentation guidelines, but need to read more of the actual inline documentation :)

Thanks for the feedback (and for staying positive ftw!)

Posting an updated patch with the inline comments (now, 100% more positive).

@egill
9 years ago

#9 @egill
9 years ago

Ignore that patch :)

@egill
9 years ago

Take 3 :)

@egill
9 years ago

it's been a long day

@egill
9 years ago

#10 @jorbin
9 years ago

  • Milestone changed from Awaiting Review to 4.4

This ticket was mentioned in Slack in #core by jorbin. View the logs.


9 years ago

#12 follow-up: @jorbin
9 years ago

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

In 33698:

Add new constant MONTH_IN_SECONDS and expand time convenience documentation.

The "month" isn't really a month. It's a WordPress Month. As the docs make clear, it's not about accuracy as much as it about convenience. This adds a missing step in the time convenience constants.

Props egill
Fixes #33397

#13 in reply to: ↑ 12 ; follow-up: @GaryJ
9 years ago

  • Keywords has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to jorbin:

expand time convenience documentation.

Per standard PHP documentation parsing, the /** ... */ DocBlock here is only applying to the first constant.

Instead, it should wrap around all of the define()'s, like wp-config.php does.

#14 in reply to: ↑ 13 ; follow-up: @egill
9 years ago

Replying to GaryJ:

Per standard PHP documentation parsing, the /** ... */ DocBlock here is only applying to the first constant.

Instead, it should wrap around all of the define()'s, like wp-config.php does.

You're absolutely right. So this would be more like it?:

/**#@+
 * Constants for expressing human-readable intervals
 * in their respective number of seconds.
 *
 * Please note that these values are approximate and are provided for convenience.
 * For example, MONTH_IN_SECONDS wrongly assumes every month has 30 days and
 * YEAR_IN_SECONDS does not take leap years into account.
 *
 * If you need more accuracy please consider using the DateTime class (http://php.net/manual/class.datetime.php).
 */
define( 'MINUTE_IN_SECONDS', 60 );
define( 'HOUR_IN_SECONDS',   60 * MINUTE_IN_SECONDS );
define( 'DAY_IN_SECONDS',    24 * HOUR_IN_SECONDS   );
define( 'WEEK_IN_SECONDS',    7 * DAY_IN_SECONDS    );
define( 'MONTH_IN_SECONDS',  30 * DAY_IN_SECONDS    );
define( 'YEAR_IN_SECONDS',  365 * DAY_IN_SECONDS    );
/**#@-*/
Last edited 9 years ago by egill (previous) (diff)

@egill
9 years ago

wrapping constants in a DocBlock template

#15 in reply to: ↑ 14 @GaryJ
9 years ago

  • Keywords has-patch added

Replying to egill:

Replying to GaryJ:
So this would be more like it?

That looks good to me. I can't see Docblock Templates mentioned in the draft PSR-5, but at least we're being consistent with existing WP core code.

#16 @wonderboymusic
9 years ago

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

In 33737:

After [33698], wrap the time constants in a DocBlock template.

Props egill.
Fixes #33397.

Note: See TracTickets for help on using tickets.