strtotime() is evil don't use it

[r-a-y]

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.

[r-a-y]

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]

Encompasses all components.

[DJPaul]

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.

[DJPaul]

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. ;)

[r-a-y]

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).

[DJPaul]

The +0000 workaround looks nice.

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

[DJPaul]

Moving to 2.2 so we have time for more discussion.

[DJPaul]

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

[r-a-y]

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)

[DJPaul]

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.

[r-a-y]

In 11019:

Core: Remove redundant strtotime() calls.

See #5781.

[r-a-y]

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.

[r-a-y]

In 11021:

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

Broke when refactoring some strtotime() calls in r11019.

See #5781.

[DJPaul]

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.

[jdgrimes]

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.

[r-a-y]

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.

[jdgrimes]

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. :-)

[r-a-y]

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

Let's mark this as fixed for now.

Thanks for the feedback, @jdgrimes!

[boonebgorges]

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.

[r-a-y]

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.

[boonebgorges]

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.