Opened 6 years ago
Closed 4 months 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)
Change History (54)
@
6 years ago
Another option for wp_date() and wp_strtotime() which doesn't use date_default_timezone_set()
#1
@
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.
#3
follow-up:
↓ 4
@
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
@
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).
#5
@
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.
#6
@
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
@
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:
↓ 14
@
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!
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
#14
in reply to:
↑ 8
@
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 ); }
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
This ticket was mentioned in Slack in #core by drew. View the logs.
5 years ago
#19
@
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
This ticket was mentioned in Slack in #core by sergey. View the logs.
3 years ago
#24
@
6 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.
#25
@
6 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.
6 months ago
#27
@
6 months ago
- Milestone set to 5.3
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#29
@
6 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
@
6 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:
- Have
date_i18n()
andwp_date()
both return current time for invalid input. - Have
date_i18n()
return current time, butwp_date()
returnfalse
for invalid input.
This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.
6 months ago
#32
@
6 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.
#34
@
4 months 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.
@
4 months ago
Fixes issue where passing $timestamp_with_offset as 0
is interpreted as the current datetime, instead of unix epoch (see comment #34)
@
4 months 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)
#36
@
4 months 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 :)
#39
@
4 months 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.
Adds wp_date(), wp_strtotime(), and wp_get_timezone_string() to simplify working with local dates/times