WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 2 weeks ago

#28636 closed enhancement (fixed)

Add functions for working with local dates and times

Reported by: mboynes Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 2.9
Component: Date/Time Keywords: has-patch commit dev-reviewed has-unit-tests
Focuses: Cc:
PR Number:

Description

Since WordPress sets date_default_timezone_set( 'UTC' ), working with local dates and times can sometimes be a bit of a nuisance. I'd like to propose adding a couple of functions to replace at least date() and strtotime(). We can add replacements for others too, like mktime() if the interest is there.

I've put together two proposals for accomplishing this, one using date_default_timezone_set() to temporarily change the timezone, and another (far more complicated version) using DateTime/DateTimeZone. I think the former is preferable, unless anyone has any reasons, performance or otherwise, why resetting the default timezone might be a bad thing?

I'm not married to the function names I chose (wp_date() and wp_strtotime()), so if it would be preferable to name them e.g. wp_local_date() and wp_local_strtotime(), I'm open to it.

Attachments (14)

28636.1.diff (2.8 KB) - added by mboynes 5 years ago.
Adds wp_date(), wp_strtotime(), and wp_get_timezone_string() to simplify working with local dates/times
28636.2.diff (2.8 KB) - added by mboynes 5 years ago.
Another option for wp_date() and wp_strtotime() which doesn't use date_default_timezone_set()
28636.3.diff (3.4 KB) - added by mboynes 5 years ago.
Leverage DateTime to parse the timezone from the string
28636.4.diff (2.7 KB) - added by mboynes 5 years ago.
Same as 28636.3.diff but without additional line ending edits
28636.5.diff (2.7 KB) - added by mboynes 5 years ago.
Latest update to date functions, removing adjacent @$ to improve readability
28636.6.diff (3.4 KB) - added by MikeHansenMe 5 years ago.
Refreshed with basic unit tests
wp-date.patch (8.1 KB) - added by Rarst 3 months ago.
localized-time-default-current-invalid-input.patch (1.1 KB) - added by Rarst 2 months ago.
localized-time-invalid-input.patch (3.1 KB) - added by Rarst 2 months ago.
functions.diff (562 bytes) - added by gravityview 3 weeks ago.
Fixes issue where passing $timestamp_with_offset as 0 is interpreted as the current datetime, instead of unix epoch (see comment #34)
functions.2.diff (665 bytes) - added by gravityview 3 weeks ago.
Fixes issue where passing $timestamp_with_offset as 0 is interpreted as the current datetime, instead of unix epoch for both GMT *and* non-GMT (see comment #34)
28636.tests.diff (723 bytes) - added by soulseekah 2 weeks ago.
test 0 epoch
28636.tests.2.diff (723 bytes) - added by soulseekah 2 weeks ago.
(without weird tabs at the end)
28636-handle-zero-timestamp.patch (1.9 KB) - added by Rarst 2 weeks ago.

Download all attachments as: .zip

Change History (54)

@mboynes
5 years ago

Adds wp_date(), wp_strtotime(), and wp_get_timezone_string() to simplify working with local dates/times

@mboynes
5 years ago

Another option for wp_date() and wp_strtotime() which doesn't use date_default_timezone_set()

#1 @nacin
5 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to Future Release

This is clever. I wish we could back away from setting UTC but that ship has long sailed.

I think this would break when dealing with arbitrary GMT offsets. Example, UTC+3.5 (Iranian Standard Time). gmt_offset can store any arbitrary float.

We probably need a more rigorous way to handle timezone juggling, which would make this easier.

I'd be curious what rmccue thinks as he is pretty familiar with date/time handling in PHP and is dealing with similar issues in the REST API.

#2 @nacin
5 years ago

  • Version changed from trunk to 2.9

Since 2.9 per #10940.

#3 follow-up: @rmccue
5 years ago

+1 on this ticket in general.

Looking at it, as nacin noted, gmt_offset allows anything. One huge issue is that DateTimeZone does not handle arbitrary timezone offsets; we've run into this, and had to change our handling because of it.

The latter patch seems to handle this fine.

One thing I see missing right now is checking for "Z" in the time string; Z indicates Zulu time (UTC) in ISO8601/RFC3339. Note that ISO8601 can also take timezone offsets in the form of "+hhmm" or "+hh" (ditto -). IMO, detecting this is more trouble than it's worth, and it might be worth reconsidering that approach.

I'll check this out. :)

#4 in reply to: ↑ 3 @mboynes
5 years ago

Great feedback, thanks!

Replying to rmccue:

One thing I see missing right now is checking for "Z" in the time string; Z indicates Zulu time (UTC) in ISO8601/RFC3339. Note that ISO8601 can also take timezone offsets in the form of "+hhmm" or "+hh" (ditto -). IMO, detecting this is more trouble than it's worth, and it might be worth reconsidering that approach.

You're right, this simply wasn't thorough enough. The more I elaborated on the regex, the more I was reinventing the wheel. I'm attaching another patch to leverage DateTime's parser to see if a timezone is present in the string. If one is, or the string fails to parse, it bails. Since this is to be a drop-in replacement for strtotime(), it makes sense that it should use the same parser (duh).

@mboynes
5 years ago

Leverage DateTime to parse the timezone from the string

@mboynes
5 years ago

Same as 28636.3.diff but without additional line ending edits

#5 @rmccue
5 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 4.1

New patches slipped under my radar; sorry! Looking good. Only change I'd make is to use '@' . @timestamp instead of @$timestamp; the latter looks similar to the error suppression operator to me, so IMO it's jarring to read.

Otherwise, +1 on this patch.

@mboynes
5 years ago

Latest update to date functions, removing adjacent @$ to improve readability

#6 @mboynes
5 years ago

That's funny, it didn't bother me when I wrote it, but once you pointed it out I twitched a little. Latest patch concatenates.

#7 @johnbillion
5 years ago

  • Milestone changed from 4.1 to Future Release

It's a bit late to get these into 4.1.

I'd like to see some example use cases where these functions come in handy.

#8 follow-up: @mboynes
5 years ago

Here's the use-case which influenced this:

Let's say I have a form with a datepicker, and I want to schedule a cron event based on the date string I enter, perhaps 2014-11-19 at 23:00. If I use strtotime(), for me in EST, this will end up firing 5 hours earlier than I'm expecting. wp_strtotime() takes the trouble out of doing the conversion.

I think benefits/use cases for wp_date() are probably a bit more obvious. If I call date( 'Y-m-d H:i:s' ), I don't get my current time, which I think can be confusing for developers. Calling wp_date( 'Y-m-d H:i:s' ) simply provides the site's local time. Similarly, you can pass a timestamp, and you're going to get back the requested date/time format in the site's local timezone.

Since these aren't going to be used anywhere in core out of the gate, I don't think there's any harm in including them late in beta, but if you still want to punt to 4.2, I understand. Let me know if you have any questions!

#9 @swissspidy
5 years ago

Also related to #18146, which aims to add user-level timezone settings.

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


5 years ago

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


5 years ago

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


5 years ago

#13 @rachelbaker
5 years ago

  • Milestone changed from Future Release to 4.2

#14 in reply to: ↑ 8 @SergeyBiryukov
5 years ago

I wonder if we could reuse get_date_from_gmt() for wp_date():

function wp_date( $format, $timestamp = false ) {
	if ( ! $timestamp ) {
		$timestamp = time();
	}

	return get_date_from_gmt( date( 'Y-m-d H:i:s', $timestamp ), $format );
}

@MikeHansenMe
5 years ago

Refreshed with basic unit tests

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


5 years ago

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


5 years ago

#17 @DrewAPicture
5 years ago

@rmccue: Could we get you to follow up here with your best recommendation?

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


5 years ago

#19 @DrewAPicture
5 years ago

  • Milestone changed from 4.2 to Future Release

No activity in more than a month, still looking for confirmation from @SergeyBiryukov and/or @rmccue. Punting.

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


5 years ago

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


4 years ago

#22 @MikeHansenMe
3 years ago

  • Keywords has-unit-tests added

28636.6.diff still applies adding has tests.

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


3 years ago

#24 @Rarst
3 months ago

  • Keywords 2nd-opinion has-patch has-unit-tests removed

I looked over the suggested patch, in context of current work on the component. In general I feel that writing API for just local time zone is a little too narrow. We need to balance between allowing for arbitrary time zone (use case poorly served in existing API) and not reinventing upstream DateTime functionality too much.

I agree that wp_date() is necessary, but for localized output, in other words a less problematic version of date_i18n(). This is definitely in my plans to work on.

On the fence about wp_strtotime() because now it's possible to reduce it to mere new DateTime( $input, wp_timezone() ). And if you routinely need to parse insufficient inputs are the problem really, not parsing them. Will consider if dedicated parsing function would help while I work through fixing up old API functions.

@Rarst
3 months ago

#25 @Rarst
3 months ago

  • Keywords has-patch added

Extracted date localization logic into a new wp_date() function, with saner API using real timestamps and arbitrary time zone. Changed date_i18n() to a wrapper around it and cleaned up logic a bit for clarity.

This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.


3 months ago

#27 @SergeyBiryukov
3 months ago

  • Milestone set to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#28 @SergeyBiryukov
3 months ago

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

In 45901:

Date/Time: Introduce wp_date() to retrieve the date in localized format.

Convert date_i18n() into a wrapper for wp_date().

wp_date() is intended as a replacement for date_i18n() without legacy quirks in it. It accepts a true Unix timestamp (not summed with timezone offset) and an arbitrary timezone.

Props Rarst, mboynes, MikeHansenMe, rmccue, nacin.
Fixes #28636.

#29 @Rarst
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I have lost a bit of behavior where invalid input would result in current time (consistent with upstream PHP date().

Added patch to restore behavior in date_i18n() and made wp_date() implementation consistent.

#30 @Rarst
2 months ago

Actually, no, while date_i18n() does behave like this, date() does not. It would return false on invalid timestamp input.

I am not sure what is preferable here:

  1. Have date_i18n() and wp_date() both return current time for invalid input.
  2. Have date_i18n() return current time, but wp_date() return false for invalid input.

This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.


2 months ago

#32 @Rarst
2 months ago

Went with false for wp_date() since I'd rather have more predictable, less implicit, more consistent with PHP behavior from cleanly designed function.

Added unit tests for invalid input in both functions.

#33 @SergeyBiryukov
2 months ago

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

In 45914:

Date/Time: Restore the previous behavior of date_i18n() where invalid input would result in current time.

Make wp_date() return false on invalid timestamp input, for consistency with upstream PHP date() function.

Props Rarst.
Fixes #28636.

#34 @soulseekah
3 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There's a problem with date_i18n and a timestamp that is 0 now.

before:

date_i18n( 'Y-m-d', 0 ); => '1970-01-01' correct!

after:

date_i18n( 'Y-m-d', 0 ); => '2019-10-24' not correct!

I believe this is a regression.

@gravityview
3 weeks ago

Fixes issue where passing $timestamp_with_offset as 0 is interpreted as the current datetime, instead of unix epoch (see comment #34)

@gravityview
3 weeks ago

Fixes issue where passing $timestamp_with_offset as 0 is interpreted as the current datetime, instead of unix epoch for both GMT *and* non-GMT (see comment #34)

#35 @Rarst
3 weeks ago

Good catch! Probably also worth adding unit tests, I'll look tomorrow.

#36 @SergeyBiryukov
3 weeks ago

Thanks for catching that! This appears to be my fault for changing the strict checks from wp-date.patch to simple "falsy" checks just because they are more common throughout core. Shouldn't have done that here :)

#37 @azaozz
3 weeks ago

  • Keywords commit added

functions.2.diff looks good, +1 to commit.

#38 @SergeyBiryukov
3 weeks ago

  • Keywords dev-reviewed added

@soulseekah
2 weeks ago

test 0 epoch

@soulseekah
2 weeks ago

(without weird tabs at the end)

#39 @Rarst
2 weeks ago

  • Keywords has-unit-tests added

Added combined patch with the fix and tad more precise unit tests. Cheers!

@soulseekah FYI when writing Date/Time tests we now push to set non-UTC timezone and check very complete representation of time, such as RFC3339. Testing at UTC and looser precision caused old tests to be missing ton of bugs, since a lot of issues have to do with time zone configuration and output.

#40 @SergeyBiryukov
2 weeks ago

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

In 46577:

Date/Time: Make sure date_i18n() correctly handles zero timestamp after [45901].

Props soulseekah, gravityview, Rarst.
Reviewed by azaozz, SergeyBiryukov.
Fixes #28636.

Note: See TracTickets for help on using tickets.