WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 7 weeks ago

#40657 closed enhancement (fixed)

Deprecate current_time( 'timestamp' )

Reported by: jdgrimes Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Date/Time Keywords: has-patch
Focuses: Cc:
PR Number:

Description

I've just opened #40653 to address some issues with how current_time() operates internally. That ticket provides a bit of background on the age of the current_time() function and why it does what it does.

In this ticket I'd like to specifically address the use of current_time( 'timestamp' ), which 'gets the Unix timestamp of the current time in the site's timezone.'

The problem is that timestamps are actually timezoneless, they are UTC, meaning that they are always counting the number of seconds from the Unix epoch, which is a certain point in time, simultaneous across all timezones.

What WordPress is trying to do is explained by Otto here:

Basically, in order to maintain consistency across older PHP 4 installations, it was decided to have WP force the PHP environment to be using UTC dates for everything. [I.e., date() is always GMT.] Then WordPress itself applies the adjustments when needed.

...

A call to current_time( 'timestamp' ) will give you the current timestamp adjusted by the necessary amount to get the correct time display. So to get the correct date in WordPress using date(), you'd do this:

date('Y-m-d', current_time( 'timestamp' ));

How it breaks down:

  • PHP date and time functions should always give you the info in UTC.
  • current_time( 'timestamp' ) will give you a UTC timestamp adjusted by the amount necessary to show the local date.

So this was deemed necessary for legacy reasons (PHP 4), but it is very confusing and tends to fool learning devs into a complete misunderstanding of timestamps, causing them to think that timestamps somehow have a timezone. In reality, this is just a quirk of legacy handling provided by WordPress. But it actually is no longer necessary, and probably ought to be deprecated, or at least discouraged.

I say that it isn't necessary because the current_time() function now allows for any format string to be passed. This means that instead of this:

date('Y-m-d', current_time( 'timestamp' ));

You can skip the step of getting the "non-GMT" timestamp, and just do this:

current_time( 'Y-m-d' );

Also, because the timestamp does not actually carry any timezone information, if you do it the first way and try to format it later in a manner that includes timezone information, it will be UTC. (current_time() actually has this same problem right now, but it would be fixed by #40653.)

In summary:

  • current_time( 'timestamp' ) is not necessary to use, the format you want to convert the timestamp to can be passed directly.
  • It confuses devs into thinking that timestamps are timezone-relative, when they are not.
  • It won't work when ultimately converted to formats that include timezone information, since the timestamp does not actually include timezone info.

It might not be worth it to actually fully deprecate the 'timestamp' arg when $gmt is false, just because of the volume of legacy code that might be using this, but I thought at least I would open a ticket to raise awareness.

Attachments (2)

trac-40657-reduce-timestamp.diff (9.2 KB) - added by Rarst 16 months ago.
Reduced explicit local current_time() timestamps use in core.
removed-local-timestamp-revision-title.patch (613 bytes) - added by Rarst 7 weeks ago.

Download all attachments as: .zip

Change History (21)

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


2 years ago

#2 @Rarst
19 months ago

Problem is some function expect this "WordPress timestamp" as input, notably date_i18n(). It cannot be dropped without major Date/Time component rewrite.

This is the main problem with current state of the component, it just cannot be fixed in piecemeal fashion. Everything from storage, to computation, to output needs be replaced together since it's all linked and fuses functionality and bugs together.

#3 @johnstonphilip
16 months ago

FWIW, I was one of those learning devs who completely misunderstood, and had to do massive changes and upgrade routines to fix it all.

I wonder if we could at least remove this out-dated instruction [here]https://codex.wordpress.org/Function_Reference/current_time:

current_time( 'timestamp' ) should be used in lieu of time() to return the blog's local time. In WordPress, PHP's time() will always return UTC and is the same as calling current_time( 'timestamp', true ).

#4 @Rarst
16 months ago

I’ve just recently started on that very major fixup of whole Date/Time component, so getting there. :)

current_time() is clearly one of the most problematic functions (after date_i18n()) and there are 6+ tickets about dealing with it. So I don't have a clear idea what are we going to do with it yet, but we definitely handle it in some way. It would be nice to deprecate it altogether, if possible.

@Rarst
16 months ago

Reduced explicit local current_time() timestamps use in core.

#5 @Rarst
16 months ago

  • Keywords has-patch added

Don't think we are at a point where we can deprecate timestamp use in core yet, but added a patch that eliminates most of the calls (down to literally three).

#6 @SergeyBiryukov
9 months ago

  • Milestone changed from Awaiting Review to 5.2
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#7 @SergeyBiryukov
7 months ago

In 44809:

Date/Time: Reduce explicit local current_time( 'timestamp' ) usage in favor of native PHP functions.

Timestamps don't carry any timezone information, using the intended format directly simplifies the logic and makes the code less confusing.

Props Rarst, jdgrimes.
See #40657.

#8 @desrosj
7 months ago

Should this ticket continue? or can it be closed out after [44809]?

#9 @Rarst
7 months ago

Let me look into remainder of uses some more. I hadn't decided if they can be wrapped up here or worth handling in separate issues down the road.

#10 @SergeyBiryukov
7 months ago

  • Milestone changed from 5.2 to 5.3

As 5.2 Beta 1 approaches, moving to 5.3 for further discussion.

#11 @Rarst
5 months ago

  • Keywords needs-patch added; has-patch removed

I've looked through remaining uses (5 or so) and I can (probably) clean them up once I get wp_timezone() merged.

Though some of them are _very_ ambiguous, like getting timezone-less event date/time from API or fuzzy before/after in date queries.

#12 @Rarst
5 months ago

Related #41782

#13 @Rarst
4 months ago

Related #47463

#14 @Rarst
4 months ago

Related #47464

#15 @Rarst
8 weeks ago

Down to last use in wp_post_revision_title_expanded()! Will work on getting rid of it.

#16 @Rarst
7 weeks ago

  • Keywords has-patch added; needs-patch removed

Added patch for last use that can be dropped.

Don't think we can formally deprecate it yet. Once last bit is merged I'll open WPCS issue to discourage the use for starters.

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


7 weeks ago

#18 @SergeyBiryukov
7 weeks ago

In 45900:

Date/Time: Remove the last remaining current_time( 'timestamp' ) instance in wp_post_revision_title_expanded().

Timestamps don't carry any timezone information, using $revision->post_modified_gmt simplifies the logic.

Props Rarst.
See #40657.

#19 @Rarst
7 weeks ago

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

Opened WPCS issue https://github.com/WordPress/WordPress-Coding-Standards/issues/1791

Going to close ticket as we are pretty much done with getting rid of it (as much as possible), but we can't formally deprecate it yet, with WP timestamps not really out of core completely.

We'll probably revisit deprecation in bulk after work on revamping the component is done and dust settles for a while.

Note: See TracTickets for help on using tickets.