Opened 9 years ago
Closed 9 years ago
#33397 closed enhancement (fixed)
Add constant MONTH_IN_SECONDS
Reported by: | toscho | Owned by: | 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:
- Now, we do use
30 * DAY_IN_SECONDS
inhuman_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.
- 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.
- 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)
Change History (22)
#4
follow-up:
↓ 6
@
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.
#6
in reply to:
↑ 4
@
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
@
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
@
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).
This ticket was mentioned in Slack in #core by jorbin. View the logs.
9 years ago
#12
follow-up:
↓ 13
@
9 years ago
- Resolution set to fixed
- Status changed from assigned to closed
In 33698:
#13
in reply to:
↑ 12
;
follow-up:
↓ 14
@
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:
↓ 15
@
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 ); /**#@-*/
#15
in reply to:
↑ 14
@
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.
Marking the good-first-bug as "claimed".