Opened 5 years ago
Closed 5 weeks ago
#28636 closed enhancement (fixed)
Add functions for working with local dates and times
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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)
@
5 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
@
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.
#25
@
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
@
3 months ago
- Milestone set to 5.3
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#29
@
3 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
@
3 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()returnfalsefor invalid input.
This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.
3 months ago
#32
@
3 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
@
5 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.
@
5 weeks ago
Fixes issue where passing $timestamp_with_offset as 0 is interpreted as the current datetime, instead of unix epoch (see comment #34)
@
5 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)
#36
@
5 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 :)
#39
@
5 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.
Adds wp_date(), wp_strtotime(), and wp_get_timezone_string() to simplify working with local dates/times