Skip to:
Content

Opened 3 years ago

Closed 16 months ago

Last modified 12 months ago

#5781 closed defect (bug) (fixed)

strtotime() is evil don't use it

Reported by: r-a-y Owned by: r-a-y
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc:

Description

In reference to r8692, bp_core_time_since() is used in conjunction with strtotime().

strotime() is unnecessary since we use gmmktime() in bp_core_time_since(). See line 795:
https://buddypress.trac.wordpress.org/browser/tags/2.0.1/bp-core/bp-core-functions.php#L792

Attached patch removes the strtotime() function from r8692.

Attachments (5)

5781.01.patch (541 bytes) - added by r-a-y 3 years ago.
5781.02.patch (14.2 KB) - added by r-a-y 3 years ago.
Encompasses all components.
5781.03.patch (3.0 KB) - added by r-a-y 17 months ago.
5781.mysql2date.patch (13.3 KB) - added by r-a-y 17 months ago.
5781.strtotime-unit-test.patch (1.8 KB) - added by r-a-y 17 months ago.

Download all attachments as: .zip

Change History (29)

@r-a-y
3 years ago

#1 @r-a-y
3 years ago

02.patch introduces bp_core_strtotime() to ensure that the timestamp is indeed UTC.

There are some other things that I could use feedback on.

  • The admin dashboard uses a mixture of the localized date/time format and UTC dates.
    • Activity dashboard index uses the localized timezone, whereas when you edit an activity item in the dashboard, the timezone is UTC.
    • I mirrored this behavior for the Groups dashboard as well for now, but we should probably stick to UTC to avoid confusion.
  • I didn't touch the XProfile datebox fields because that probably doesn't need to be converted to UTC.

@r-a-y
3 years ago

Encompasses all components.

#2 @DJPaul
3 years ago

I don't understand why we want to introduce something like bp_core_strtotime when WordPress doesn't have the same problem. Nor do I understand what this change is trying to fix.

It sounds like this is a hack around the fact that (apparently) the raw timestamps that we use throughout BuddyPress aren't UTC. Fixing that seems like the correct fix, as is making sure we always convert to the server's current timezone on output.

#3 @DJPaul
3 years ago

And from experience, a bunch of servers often have the date_default_timezone_set option locked down, so you can't change it at runtime. Anything in our code that tries to change a server default (even temporarily) gives me the heebie-jeebies. ;)

#4 @r-a-y
3 years ago

WordPress uses localized times for the most part and are not as reliant on UTC as we are.

However, they do make one change in strtotime() when using the post's GMT date:
https://core.trac.wordpress.org/browser/tags/3.9.1/src/wp-admin/includes/meta-boxes.php#L170
https://core.trac.wordpress.org/browser/tags/3.9.1/src/wp-includes/functions.php#L30
https://core.trac.wordpress.org/browser/tags/3.9.1/src/wp-includes/post.php#L3379

They either use the timezone difference "+0000" or the GMT timezone so the time is set to GMT. Perhaps that's something we should think about doing ourselves? Could use some feedback here.

For now, I'll remove the bp_core_strtotime() references, but the other changes from my patch should be good (removal of strtotime() in various spots, using bp_core_current_time( true, 'timestamp' ) where necessary).

#5 @DJPaul
3 years ago

The +0000 workaround looks nice.

But I still don't understand what this change is trying to fix.

#6 @DJPaul
3 years ago

  • Keywords early added
  • Milestone changed from 2.1 to 2.2

Moving to 2.2 so we have time for more discussion.

#7 @DJPaul
3 years ago

  • Keywords early removed
  • Milestone changed from 2.2 to Future Release

#8 @DJPaul
18 months ago

@r-a-y can you enlighten me as to why strtotime is evil? Otherwise I want to close this ticket.

#9 @DJPaul
17 months ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from new to closed

#10 follow-up: @r-a-y
17 months ago

  • Milestone set to Awaiting Review

Sorry for missing this ticket update.

What this ticket is trying to fix is redundant strtotime() calls and inconsistencies with time parsing.

03.patch implements the redundant strtotime() calls by replacing strtotime( bp_core_current_time() ) with bp_core_current_time( true, 'timestamp' ) because as noted in the ticket description:

strotime() is unnecessary since we use gmmktime() in bp_core_time_since(). See line 795:
https://buddypress.trac.wordpress.org/browser/tags/2.0.1/bp-core/bp-core-functions.php#L792


As for the inconsistencies with time parsing, I still think altering all our strtotime() calls to add +0000 would be safer. WordPress does the same thing (see comment:4).

Here's the main takeaway, all our strtotime() calls assume that GMT is the default timezone.

In the documentation for strtotime(), this is what is stated:

Each parameter of this function uses the default time zone unless a time zone is specified in that parameter. Be careful not to use different time zones in each parameter unless that is intended. (emphasis added)

@r-a-y
17 months ago

#11 @r-a-y
17 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#12 @DJPaul
17 months ago

  • Milestone changed from Awaiting Review to 2.7
  • Owner set to r-a-y
  • Status changed from reopened to assigned

Thanks @r-a-y

5781.03.patch looks OK to add. Can you commit it please?

Adding +0000 to the other strtotime calls also seems like it makes good sense.

#13 @r-a-y
17 months ago

In 11019:

Core: Remove redundant strtotime() calls.

See #5781.

#14 @r-a-y
17 months ago

Adding +0000 to the other strtotime calls also seems like it makes good sense.

So WP core has a function - mysql2date() - that adds +0000 to the date and uses strtotime() when used like this mysql2date( 'G', 'SOME DATE STRING' );

I've gone ahead replaced all calls to strtotime() with mysql2date( 'G', X ) except xprofile date fields. As mentioned in comment:1, I wasn't sure if xprofile date field data should be stored in GMT or not.

#15 @r-a-y
17 months ago

In 11021:

Core: Make sure user last activity is saved as a date string.

Broke when refactoring some strtotime() calls in r11019.

See #5781.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


17 months ago

#17 @DJPaul
17 months ago

Good questions. I think it SHOULD but... if current xprofile is storing values in server timezone (or the timezone set in WP), if we switched to GMT, surely we'd have to run a conversion on update. If you want to put in the effort to do that, that's fab, otherwise let's not bother for now. This could be done in the future as a separate thing.

Activity dashboard index uses the localized timezone, whereas when you edit an activity item in the dashboard, the timezone is UTC.

I know this comment is 2 years old but if still the case, we should convert it to local timezone on display, and then back again to GMT on save. I'm assuming this also will need to happen on the front-end as the underlying implementations are similar.

#18 in reply to: ↑ 10 @jdgrimes
17 months ago

I just happened to see this ticket noted in the dev chat notes posted on the bpdevel blog. I've been playing a bit with dates and times in WordPress recently, and this ticket seemed to contradict my findings, so I thought I would take a closer look. My conclusion is that specifying UTC when using strtotime() is entirely unnecessary.

Replying to r-a-y:

As for the inconsistencies with time parsing, I still think altering all our strtotime() calls to add +0000 would be safer. WordPress does the same thing (see comment:4).

Here's the main takeaway, all our strtotime() calls assume that GMT is the default timezone.

In the documentation for strtotime(), this is what is stated:

Each parameter of this function uses the default time zone unless a time zone is specified in that parameter. Be careful not to use different time zones in each parameter unless that is intended. (emphasis added)

The examples of where WordPress does this that you pointed out are all really ancient. They are left-over from a time when WordPress used to just let the server timezone be whatever it wanted to be. Today however, WordPress always sets the default timezone to UTC, so date(), time(), and strtotime() will return UTC times by default. This is reflected in newer code.

This is all just FYI. But basically I'm saying that there is no need to switch out strtotime() for mysql2date() just for timezone issues.

#19 follow-up: @r-a-y
17 months ago

Thanks for the interest, @jdgrimes.

This ticket has nothing to do with WordPress date settings and everything to do with the server that WordPress is hosted on and whether they have set the default timezone to something other than UTC.

I've attached a unit test - strtotime-unit-test.patch - that outlines my example.

Update - Missed this part:

Today however, WordPress always sets the default timezone to UTC, so date(), time(), and strtotime() will return UTC times by default. This is reflected in newer code.

You're technically correct, but other plugins could, in theory, modify the timezone. Patch could still be applied to babyproof our timezone calls.

Last edited 17 months ago by r-a-y (previous) (diff)

#20 in reply to: ↑ 19 @jdgrimes
17 months ago

Replying to r-a-y:

Update - Missed this part:

Today however, WordPress always sets the default timezone to UTC, so date(), time(), and strtotime() will return UTC times by default. This is reflected in newer code.

You're technically correct, but other plugins could, in theory, modify the timezone. Patch could still be applied to babyproof our timezone calls.

That's true, but note that if a plugin changed this it would also affect current_time() since it assumes that date() and time() will always be UTC. (And probably many other places in core do this as well.)

So it's fine to baby-proof it, but its worth noting that nothing is going to break it without breaking WordPress core. :-)

#21 @r-a-y
16 months ago

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

I've had time (pun intentional!) to think about this.

Let's mark this as fixed for now.

Thanks for the feedback, @jdgrimes!

#22 @boonebgorges
12 months ago

For future reference: On a client site, I found an instance of a direct SQL query attempting to get recently active users like this:

SELECT ... FROM {$bp->members->table_name_last_activity} ... WHERE date_recorded >= DATE_SUB( UTC_TIMESTAMP(), INTERVAL 1 HOUR ) ...

The changes in this ticket mean that I needed to switch UTC_TIMESTAMP() to NOW() in order for recently-active users to be picked up. This seems backward to me: my understanding was that the goal here was to move away from timezone dependence. I haven't taken the time to dig any deeper into what's happening, but I thought I'd leave a note here.

#23 @r-a-y
12 months ago

Thanks for the report, @boonebgorges. Also see #7396.

We should always use UTC. I see a few spots where there are still strtotime() references that might be causing your problem as well as in #7396. Updates and a patch will go in #7396.

#24 @boonebgorges
12 months ago

It turned out in my case that there was a plugin running date_default_timezone_set() and setting it to something other than UTC. This was causing problems when BP started saving stuff as UTC. Once I let WP handle timezones, things seemed to correct themselves.

Note: See TracTickets for help on using tickets.