WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#20987 closed enhancement (fixed)

Easier expression of time periods

Reported by: nbachiyski Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Numbers are cool. But sometimes they stand in the way of what we have to say.

set_transient( 'plugin_slugs', $plugin_slugs, 86400 );

From the context it's obvious that 86400 is a time period and with some calculation we can see it's 24 hours/1 day in seconds. Of course, it's not very readable, so usually in the code we see a clarifying comment:

// 1 day

This definitely helps. But we could do better. We can make the whole thing read naturally in English:

set_transient( 'plugin_slugs', $plugin_slugs, DAY_IN_SECONDS );

This way we get all the information we need – we know the transient is set for one day, we can read this in plain English and we don't need to spare extra cycles to either make mathematical calculations or wait to the end of the line to see the actual period in the comment.

In the patch is attached a new file time-constants.php which contains a couple of constants.

All of them follow the pattern: <SINGULAR_OF_TIME_UNIT>_IN_<PLURAL_OF_TIME_UNIT>.

For now there are only *_IN_SECONDS and *_IN_MINUTES, since that's what we found useful on WordPress.com.

Attachments (7)

time-constants.diff (974 bytes) - added by nbachiyski 3 years ago.
20987.patch (6.6 KB) - added by SergeyBiryukov 3 years ago.
20987.2.patch (29.3 KB) - added by SergeyBiryukov 3 years ago.
20987.3.patch (29.9 KB) - added by SergeyBiryukov 3 years ago.
20987.4.patch (29.5 KB) - added by SergeyBiryukov 3 years ago.
more_constants.patch (1.7 KB) - added by pavelevap 3 years ago.
20987.5.patch (1.9 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (25)

@nbachiyski3 years ago

comment:1 @rosshanney3 years ago

  • Cc rosshanney@… added

comment:2 @scribu3 years ago

  • Milestone changed from Awaiting Review to 3.5

Pretty neat!

comment:3 @azaozz3 years ago

+1. Also, perhaps these constants can go in default-constants.php.

Last edited 3 years ago by azaozz (previous) (diff)

@SergeyBiryukov3 years ago

comment:4 @SergeyBiryukov3 years ago

20987.patch is an attempt to use the constants throughout the core.

We could also probably mention them in wp_get_schedules() description:
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-includes/cron.php#L284

comment:5 @SergeyBiryukov3 years ago

More instances in 20987.2.patch.

Looks like only *_IN_SECONDS have actual use cases in core.

@SergeyBiryukov3 years ago

comment:6 follow-up: @nbachiyski3 years ago

  • Keywords dev-feedback added

Sergey, thanks for the awesome patch, it's a tremendous help.

The only change I would make to your patch is to replace DAY_IN_SECONDS / 2 with 12 * HOUR_IN_SECONDS. It just feels more natural to me and everywhere else we refer to it as 12 hours, not half a day.

About the minutes – westi found some use for them in the WordPress.com codebase, and I think they would do no harm.

Any objections if we just commit that?

comment:7 in reply to: ↑ 6 @SergeyBiryukov3 years ago

Replying to nbachiyski:

The only change I would make to your patch is to replace DAY_IN_SECONDS / 2 with 12 * HOUR_IN_SECONDS.

Thanks, done in 20987.3.patch.

@SergeyBiryukov3 years ago

comment:8 @nacin3 years ago

I understand the case for SECONDS but not as much for MINUTES, as they serve no use in core, nor much of an obvious use in general.

I'm also not a fan of MONTH_IN_SECONDS, as month lengths are variously 28-31 days. (A year is at least generally 365 days in length.) We also have no use for it in core.

@SergeyBiryukov3 years ago

comment:9 @SergeyBiryukov3 years ago

MONTH_IN_SECONDS is not used indeed. 20987.4.patch excludes it as well as *_IN_MINUTES.

comment:10 @nacin3 years ago

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

In [21996]:

Introduce constants to allow for easier expression of time periods in seconds. Adds MINUTE_IN_SECONDS, HOUR_IN_SECONDS, DAY_IN_SECONDS, WEEK_IN_SECONDS, YEAR_IN_SECONDS. props nbachiyski, SergeyBiryukov. fixes #20987.

comment:11 @nacin3 years ago

In [21997]:

Do not use time constants in files the WP bootstrap is not or may not be loaded. see #20987.

comment:12 @nbachiyski3 years ago

Nice :-)

comment:13 @F J Kaiser3 years ago

Their names are extremely generic. Could those constants please be prefixed with WP_*? Else it's hard to determine where they belong when you got autocomplete in your IDE and using another system along WP.

Last edited 3 years ago by F J Kaiser (previous) (diff)

comment:14 @pavelevap3 years ago

  • Cc pavelevap@… added

I found some more examples, see attached patch.

@pavelevap3 years ago

comment:15 @SergeyBiryukov3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:16 @nacin3 years ago

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

In [22121]:

Use 2 * DAY_IN_SECONDS rather than 172800. props pavelevap. fixes #20987.

@SergeyBiryukov3 years ago

comment:17 @SergeyBiryukov3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

20987.5.patch adds a comment for [21997] and fixes a typo.

comment:18 @nacin3 years ago

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

In 22531:

Add comments to time intervals we cannot express with the constants. Fix a comment. props SergeyBiryukov. fixes #20987.

Note: See TracTickets for help on using tickets.