Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discourage use current_time( 'timestamp' ) #1791

Closed
Rarst opened this issue Aug 28, 2019 · 8 comments · Fixed by #1808
Closed

Discourage use current_time( 'timestamp' ) #1791

Rarst opened this issue Aug 28, 2019 · 8 comments · Fixed by #1808

Comments

@Rarst
Copy link
Contributor

Rarst commented Aug 28, 2019

Is your feature request related to a problem?

current_time() with timestamp or U format produces a "WordPress timestamp", a sum of Unix timestamp and current time zone offset.

While historically entrenched in core, the use of such timestamps is deeply problematic and we are working on eliminating them in favor of real Unix timestamps and full interoperability with PHP.

Describe the solution you'd like

  • current_time( 'timestamp', true ) / current_time( 'U', true ) are equivalent to time() and should be replaced with it
  • current_time( 'timestamp' ) / current_time( 'U' ) should be discouraged in favor of requesting non-timestamp format, or otherwise refactoring the usage

Additional context (optional)

The use of current_time( 'timestamp' ) had been mostly eliminated from core in #40657. Minimal usage in unit tests and older functions that must operate with WP timestamps for backwards compatibility reasons remains.

@GaryJones
Copy link
Member

Just because I missed the emphasis here on first read - the bad thing here isn't the use of current_time(), but more the use of the first arg being timestamp.

@Rarst
Copy link
Contributor Author

Rarst commented Aug 29, 2019

Yes, current_time() works fine for other formats (now, after round of fixes).

@jrfnl jrfnl added this to the 2.2.0 milestone Sep 11, 2019
@jrfnl jrfnl self-assigned this Sep 11, 2019
@jrfnl jrfnl added Component: Core Focus: WP interoperability Doing things the WP way, prefer using WP functionality over userland/PHP native Type: Enhancement labels Sep 11, 2019
@jrfnl
Copy link
Member

jrfnl commented Sep 12, 2019

I'm working on adding this check. I'm inclined to add it to the TimezoneChange sniff as all date/time related function checks are then contained in one sniff, though this one needs quite some additional logic.
Alternatively I could add a stand-alone sniff, WP.CurrentTimeTimeStamp maybe ?

Opinions ?

@Rarst
Copy link
Contributor Author

Rarst commented Sep 12, 2019

I think (hope) we head towards formally deprecating WP timestamps in the future, so it might make more sense to combine issues related to them as separate sniff. Would make it more convenient down the road to handle them as a separate unit of mess.

PS "timestamp" is a single word traditionally in computing context, so CurrentTimeTimestamp for name then :)

@jrfnl
Copy link
Member

jrfnl commented Sep 12, 2019

@Rarst Can you clarify what you mean ? You seem to contradict yourself in yes/no separate sniffs or not.

As an alternative, I was just thinking, we could also create a separate DateTime category, similar to the DB.

So to summarize the choice we have to make for this issue + #1794 :

  • Do we want to have all the checks for Date/Time related issues in one sniff ? or in separate sniffs ?
  • If separate sniffs: should those all be placed in a new DateTime category to make them easier to find/manage ?
  • If the same sniff, the TimeZoneChange sniff would be the one, but the name would no longer fit the function, so what should be the new name for that sniff ?

Either way, looks like the WP.TimeZoneChange will need to be deprecated in 2.2.0 and be removed in 3.0.0.

@Rarst
Copy link
Contributor Author

Rarst commented Sep 12, 2019

Messed up timezones and messed up timestamps are different problems (and current_time('timestamp') would be one specific aspect of one problem), though they are both within Date/Time component.

So the hierarchy as I see it would go like:

  • Date/Time component
    • use of WP timestamps
      • current_time() returns WP timestamp
    • setting time zone to UTC on WP boot
      • date_default_timezone_set() breaks things

I don't have a strong opinion how that should be organized within PHP CS mechanics.

@jrfnl jrfnl added Focus: DateTime and removed Focus: WP interoperability Doing things the WP way, prefer using WP functionality over userland/PHP native labels Oct 4, 2019
@jrfnl
Copy link
Member

jrfnl commented Oct 5, 2019

@Rarst I just noticed this comment in the Dev docs: https://developer.wordpress.org/reference/functions/current_time/#comment-2373

We may need to find someone with edit rights to Dev docs to remove the comment, or at the very least another comment should be left discrediting the comment.

@Rarst
Copy link
Contributor Author

Rarst commented Oct 7, 2019

Thanks for letting me know, I would probably need to leave bunch of comments around. Will do when new functions are scanned into reference so I can refer to them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants