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

Overhaul the time functions #227

Closed
Zodiac1978 opened this issue Mar 20, 2022 · 1 comment · Fixed by #232
Closed

Overhaul the time functions #227

Zodiac1978 opened this issue Mar 20, 2022 · 1 comment · Fixed by #232

Comments

@Zodiac1978
Copy link
Member

Zodiac1978 commented Mar 20, 2022

Describe the bug

We track visits in this line:

$data['created'] = strftime( '%Y-%m-%d', current_time( 'timestamp' ) );

This is explicitly not recommended since WP 5.3:
https://make.wordpress.org/core/2019/09/23/date-time-improvements-wp-5-3/
https://developer.wordpress.org/reference/functions/current_time/#comment-3414

This saves a WP timestamp (not a correct UNIX timestamp) with time() plus timezone adjustments if I understand correctly.

But in

$current_date = current_time( 'Y-m-d' );
we read/compare the data with a PHP date formatted setting (without timezone adjustments). This leads to false data in the dashboard widget.

Additionally strftime is deprecated in PHP 8.1 and it is strongly recommended to NOT use it anymore:
https://www.php.net/manual/de/function.strftime.php

This would explain some reports of false data from the support forums.

Maybe there are more errors and deprecations in time functions. Needs more checks.

@Zodiac1978 Zodiac1978 added the bug label Mar 20, 2022
@stklcode
Copy link
Contributor

stklcode commented Aug 25, 2022

we read/compare the data with a PHP date formatted setting (without timezone adjustments). This leads to false data in the dashboard widget.

Actually no.

The transformation used is not elegant in any way (guess it was in a time before PHP DateTime), but at least it yields the same results.
The "WordPress timestamp" like a unix-timestamp represents the current epoch seconds, adjusted by timezone offset.

My local system runs on CEST (UTC + 2), so current_time( 'timestamp' ) - time() === 7200. So current time 14:30 CEST is transformed to a timestamp equivalent of 14:30 UTC. But the timezone is omitted here, so we effectively get 14:30 local time. So current_time( 'H:i' ) === strftime( %H:%M', current_time( 'timestamp' ) )

(Hour/minute example just for demonstration. If the time matches, the date values are the same either.)

Ran a quick verification test on WP test site:

die( 'Unix timestamp: ' . time()  . '<br>' 
   . 'String from Unix timestmap: ' . strftime( '%Y-%m-%d %H:%M:%S', time() ) . '<br>'
   . 'WP timestamp: ' . current_time( 'timestamp' ) . '<br>'
   . 'String from WP timestamp: ' . strftime( '%Y-%m-%d %H:%M:%S', current_time( 'timestamp' ) ) . '<br>'
   . 'WP current time: ' . current_time( 'Y-m-d H:i:s' ) );

outputs

Unix timestamp: 1661431434
String from Unix timestmap: 2022-08-25 12:43:54
WP timestamp: 1661438634
String from WP timestamp: 2022-08-25 14:43:54
WP current time: 2022-08-25 14:43:54


Assuming we want to stay with local date values in the database, we should simply synchronize the notation here, i.e. use the shorter, non-deprecated and more intuitive current_time( 'Y-m-d' ) on both ends.

stklcode added a commit that referenced this issue Aug 25, 2022
We use "WP timestamp" in combination with strftime in the tracking
routine and retrieve WP Time in "Y-m-d" format for retrieval in the
dashboard logic. While it is discouraged to work with "WP Timestamps"
and strftime is deprecated as of PHP 8.1, is makes sense to use the same
source of time on both ends.

The present logic is not inherently wrong. The "WP timestmap" is
basically a unix timestamp with zone offeset already calculated and
parsing the result yields a local time value shifted to GMT. Zone is not
part of the output here, so it's effectively the same as Y-m-d local
date.

We not use the current time in Y-m-d notation on both ends without
breaking or invalidating the present logic.
stklcode added a commit that referenced this issue Aug 25, 2022
We use "WP timestamp" in combination with strftime in the tracking
routine and retrieve WP Time in "Y-m-d" format for retrieval in the
dashboard logic. While it is discouraged to work with "WP Timestamps"
and strftime is deprecated as of PHP 8.1, is makes sense to use the same
source of time on both ends.

The present logic is not inherently wrong. The "WP timestmap" is
basically a unix timestamp with zone offeset already calculated and
parsing the result yields a local time value shifted to GMT. Zone is not
part of the output here, so it's effectively the same as Y-m-d local
date.

We not use the current time in Y-m-d notation on both ends without
breaking or invalidating the present logic.
stklcode added a commit that referenced this issue Aug 25, 2022
We use "WP timestamp" in combination with strftime in the tracking
routine and retrieve WP Time in "Y-m-d" format for retrieval in the
dashboard logic. While it is discouraged to work with "WP Timestamps"
and strftime is deprecated as of PHP 8.1, is makes sense to use the same
source of time on both ends.

The present logic is not inherently wrong. The "WP timestmap" is
basically a unix timestamp with zone offeset already calculated and
parsing the result yields a local time value shifted to GMT. Zone is not
part of the output here, so it's effectively the same as Y-m-d local
date.

We not use the current time in Y-m-d notation on both ends without
breaking or invalidating the present logic.
@stklcode stklcode linked a pull request Aug 25, 2022 that will close this issue
stklcode added a commit that referenced this issue Nov 1, 2022
We use "WP timestamp" in combination with strftime in the tracking
routine and retrieve WP Time in "Y-m-d" format for retrieval in the
dashboard logic. While it is discouraged to work with "WP Timestamps"
and strftime is deprecated as of PHP 8.1, is makes sense to use the same
source of time on both ends.

The present logic is not inherently wrong. The "WP timestmap" is
basically a unix timestamp with zone offeset already calculated and
parsing the result yields a local time value shifted to GMT. Zone is not
part of the output here, so it's effectively the same as Y-m-d local
date.

We not use the current time in Y-m-d notation on both ends without
breaking or invalidating the present logic.
@stklcode stklcode added this to the 1.8.4 milestone Nov 4, 2022
stklcode added a commit that referenced this issue Nov 4, 2022
We use "WP timestamp" in combination with strftime in the tracking
routine and retrieve WP Time in "Y-m-d" format for retrieval in the
dashboard logic. While it is discouraged to work with "WP Timestamps"
and strftime is deprecated as of PHP 8.1, is makes sense to use the same
source of time on both ends.

The present logic is not inherently wrong. The "WP timestmap" is
basically a unix timestamp with zone offeset already calculated and
parsing the result yields a local time value shifted to GMT. Zone is not
part of the output here, so it's effectively the same as Y-m-d local
date.

We not use the current time in Y-m-d notation on both ends without
breaking or invalidating the present logic.
stklcode added a commit that referenced this issue Nov 4, 2022
We use "WP timestamp" in combination with strftime in the tracking
routine and retrieve WP Time in "Y-m-d" format for retrieval in the
dashboard logic. While it is discouraged to work with "WP Timestamps"
and strftime is deprecated as of PHP 8.1, is makes sense to use the same
source of time on both ends.

The present logic is not inherently wrong. The "WP timestmap" is
basically a unix timestamp with zone offeset already calculated and
parsing the result yields a local time value shifted to GMT. Zone is not
part of the output here, so it's effectively the same as Y-m-d local
date.

We not use the current time in Y-m-d notation on both ends without
breaking or invalidating the present logic.
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.

2 participants