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

[2.7] CRUD and API timezones and timestamps #13498

Closed
mikejolley opened this issue Mar 7, 2017 · 44 comments
Closed

[2.7] CRUD and API timezones and timestamps #13498

mikejolley opened this issue Mar 7, 2017 · 44 comments
Labels
type: bug The issue is a confirmed bug. type: epic Container issue with high-level description of work that will be done in sprint.
Milestone

Comments

@mikejolley
Copy link
Member

mikejolley commented Mar 7, 2017

First off, this is a large item which I wish we'd identified earlier, but it's good that we found it before launch and can have this discussion. This is in fix territory but also has implications for extensions and services who have worked with the current system in RC1.

With that in mind, I'm pinging some devs for feedback (sorry if I miss you, these were off the top of my head). @franticpsyx, @bekarice, @bryceadams, @thenbrent who mentioned timestamps and dates to me a few weeks ago, and @nylen who've I've heard shakes fists at WP when he thinks about it.

tl;dr

CRUD gets and sets dates in the site timezone like WordPress which has flaws, the main one being that changing the site timezone invalidates all stored data.

Longer version

As we know, WordPress stores dates for the most part in site timezone based, mysql format. It too is affected by the timezone issue above. We've always followed this way of doing things but the CRUD gives us a good opportunity to change this because it's a new layer we control.

If this is left out of 2.7.x, future versions cannot make this change without it breaking and we'll be stuck with non-UTC times like WordPress indefinitely. Thats why I've raised this issue now.

I think it's important that our REST API works with a standard; UTC timestamps. Likewise I think it's important that the API reflects how the CRUD works. So I'd like to use this standard everywhere.

What is needed?

This is a list of everything that needs to be handled or considered to make this happen:

  • Update CRUD objects (product, customer, order, coupon)
    • Make setters/getters expect/return UTC timestamps
    • or add dedicated setters and getters for UTC data.
    • Convert non-UTC timestamps to UTC on read
    • Document inline.
  • Update API endpoints
    • Legacy APIs - convert UTC to timezone if needed or remove any existing conversion if not.
    • v2 API - Ensure UTC.
  • Update v2 API docs.
  • Admin screens:
    • Send correct data to CRUD
    • Read correct data from CRUD.
  • Migrating existing data: Modify on read, or find a way to bulk update timestamps in a routine.
  • Directly accessed methods: Ensure they use timezone like 2.6.x.

How does this affect 2.7 launch?

If this is accepted as something which needs to be done prior to launch we'll need to do an RC2 as soon as the above is ready, another post, and then launch ~2 weeks after this. Ideally this would end up moving the launch date to ~28th March (?).

if this work is not done, we simply need to update the v2 API to use UTC times and accept that times will be wrong if the user changes timezones. Nothing else changes including the release.

cc'ing @mattyza

Over to you.

@justinshreve @coderkevin @claudiosanches @claudiulodro @bor0 @roykho @gedex @WPprodigy

@mikejolley mikejolley added API / CLI type: bug The issue is a confirmed bug. labels Mar 7, 2017
@mikejolley mikejolley added this to the 2.7 milestone Mar 7, 2017
@allendav
Copy link
Contributor

allendav commented Mar 7, 2017

👍 for UTC :allofthethings: - we adopted that as a standard on WooCommerce Services's server as well

cc @jeffstieler

@thenbrent
Copy link
Contributor

Fixing this doesn't have to result in breaking changes with RC1 or prior versions.

  1. Add UTC APIs
  2. Save UTC dates (and possibly also set UTC dates on upgrade to 2.7, or at least when object data is set)
  3. Keep all the old and new site timezone dates and APIs available and unchanged (as inherently dangerous as they may be).

Then you can still provide ways to work around the underlying issues with dates in site timezone, without breaking anyone's integrations with 2.7 or prior. Those that need exact dates (like Subscriptions) can use UTC APIs.

For example, for orders:

  • add {get|set}_date_{created|paid|completed|modified}_gmt() methods (edited)
  • set a gmt meta value at the same time as setting the non-gmt value, e.g. set a new _paid_date_gmt value when _paid_date is set, or _paid_date_gmt value when _paid_date is set

Providing both options is similar to the way WP does it, e.g. post_date_gmt and post_date columns. The added benefit is that you can use those to figure out what the timezone actually was at the time the date was set (rather than relying on the site's current timezone value).

Sure always handling all dates in UTC would be the best option. But there are million+ stores and goodness knows how much code written that expects the opposite, so IMO adding a new UTC layer is better than replacing the existing date handling.

@bekarice
Copy link
Contributor

bekarice commented Mar 7, 2017

@mikejolley In principle, this makes a lot of sense, and especially given the fact that date functions now return a timestamp in 2.7+, which should be UTC, but are local times. Totally in favor of a change to store dates as UTC (we do this in extensions ie user membership data or a voucher record already).

However, considering the previous behavior, when reading any date via property / accessor, I think core should provide a way to get these in local timestamp, either via a dedicated method or param to get UTC vs local.

For example, a payment gateway will often use the paid date for the order -- while this should be stored in UTC, it's displayed / used in the site timezone. Therefore a core method to get the local time or UTC timestamp would be a requirement here (defaulting to the local time as per pre-2.7 functionality). Custom code snippets should also be able to expect a local date rather than converting it themselves.

While storing in UTC is the best way to go, most all "reading" should be using a local time, so I don't think pushing the requirement to format this in a site timezone off to extensions would be a good decision since everything then has to implement / duplicate this code. Not sure if this is what you're planning here based on "Directly accessed methods: Convert to timezone to match 2.6.x return values.", just want to clarify that I think a core way to read dates / timestamps in local time should be part of this.

@woocommerce/skyverge has some other clarifying questions RE the get_post_meta notes and some other areas.

If this is accepted as something which needs to be done prior to launch we'll need to do an RC2 as soon as the above is ready, another post, and then launch 1-2 weeks after this. Ideally this would end up moving the launch date to 21st or 28th March.

I don't think this is realistic. First of all, we can't make any moves with extensions until a core decision is made here, which will likely take a day or two for everyone to see this.

Second, if core provides a way to read dates in the local time, this definitely makes it more straight-forward for plugins, but this means we do need to audit all date usage throughout plugins to confirm. This probably means we need ~2 weeks from when a core decision is made to do this. WooCommerce core should then be pushed out 3 weeks to give us time to release updates prior to 2.7 launch.

If plugins are going to need to implement their own helpers (which I would strongly disagree with given how much extension usage of dates would be leveraging the local time), then more time is needed.

I think @thenbrent would probably need at least this amount of time as well, given writing / setting dates would probably be the trickier thing to get right here (this affects SkyVerge less as we have far fewer plugins setting dates than reading them).

Definitely in favor of the change overall, just need enough time to ensure all usage on the extension side is done the right way.

@bekarice
Copy link
Contributor

bekarice commented Mar 7, 2017

@thenbrent lol @ posting at the exact same time 👊 👭

@mikejolley
Copy link
Member Author

mikejolley commented Mar 7, 2017

Custom code snippets should also be able to expect a local date rather than converting it themselves.

@bekarice Just to note, the getters return a timestamp, so you need to do some work anyway to convert this to a human readable string.

Providing both options is similar to the way WP does it, e.g. post_date_gmt and post_date columns. The added benefit is that you can use those to figure out what the timezone actually was at the time the date was set (rather than relying on the site's current timezone value).

That can work.

But there are million+ stores and goodness knows how much code written that expects the opposite, so IMO adding a new UTC layer is better than replacing the existing date handling.

No stores are using the CRUD layer, thats why it can be changed there. Changing the existing data that is stored can be avoided, even if that involves adding new fields as you suggest. The question is whether CRUD offers both getter/setter options, or just UTC methods.

From slack:

Would you want those _gmt/_utc setters to set the non-utc time too? I.e. make it so either can be used?

probably not, that feels like it’s breaking convention of the other setters (which just set the data of their namesake), and as you know, site timezone can refer to different things at different points in time. For example, the date in site timezone on an order shouldn’t change IMO even if the site’s timezone changes, because it is a snapshot of the site timezone at the time the order was placed, not a reflection of the site’s current timezone.

Then thats why that solution would suck - you’d have to set both and know you need to set both. That doesn’t mirror WP post_date/post_date_gmt

Needing to set both values seems like extra work and extra code for something which should be done for you. If CRUD instead provides a consistent layer for all date functions, storage can be handled by the data store and old get_post_meta calls can work as they do now.

This also means if the data ever moves away from post meta, that timezone based data does not need to be migrated in the same way?

@mikejolley
Copy link
Member Author

For ref; with post_date/post_date_gmt, WP sets the GMT date if you don't provide it https://core.trac.wordpress.org/browser/tags/4.5.3/src/wp-includes/post.php#L3182

@thenbrent updated OP. Just need to decide on new methods + meta data vs new meta data only.

@thenbrent
Copy link
Contributor

thenbrent commented Mar 7, 2017

WP sets the GMT date if you don't provide it

That's when inserting a post. I agree WC should set both automatically if one isn't provided when first creating it via wc_create_order() or a similar API function. But not if updating one date via a direct setter method.

That would still mirror WP in part, because with wp_update_post(), you can change just one date's value, because the existing $post data is used if no new data is passed in for it.

Needing to set both values seems like extra work and extra code for something which should be done for you.

I agree, but that is true for a lot of CRUD code which sets just one piece of data. I understand the date data is more closely related than most other data, and some setters, like set_status() do change more than one prop, but I don't think when someone asks to set_date_paid(), they should find that the value of get_date_paid_gmt() method is now different also.

The duplication issue could be addressed with a general set_date( $date_type ) method which will automatically set both dates. This has the advantage of also being able to also enforce validation too. For example, should it really be possible to set an order's completed date before its creation date?

The obvious disadvantage is that you have duplicate APIs with an unclear difference in purpose. But again, that's no different to other CRUD APIs, like being able to either call WC_Order::add_product() or manually create a WC_Order_Item_Product object, add data to it, and then call WC_Order::add_item().

@allendav
Copy link
Contributor

allendav commented Mar 7, 2017

@bekarice wrote:

While storing in UTC is the best way to go, most all "reading" should be using a local time, so I don't think pushing the requirement to format this in a site timezone off to extensions would be a good decision since everything then has to implement / duplicate this code.

I beg to differ a bit here - especially for Javascript clients - WooCommerce Services' client, for example, uses built ins like toLocaleDateString to generate user-facing label purchase/refund dates from API provided UTC timestamps. (toLocaleDateString automatically displays in the browser's TZ)

Also, many popular APIs like EasyPost, for example, return not timestamps but "Z" postfixed time strings (e.g. 2013-11-08T15:50:00Z) indicating UTC time.

For "reading" API endpoints, I'd include UTC timestamps as a minimum, and optionally include additional date and time strings formatted using the WordPress Date and Time settings.

@thenbrent
Copy link
Contributor

thenbrent commented Mar 7, 2017

The question is whether CRUD offers both getter/setter options, or just UTC methods.

Answering that question from our perspective, it won't bother us either way which method is chosen. It's quite easy for us to change the updates we've made for 2.7 for the dates we do depend on WC for because we already abstract it all behind a get_date() method. But that's just Subscriptions.

It does sound like from @bekarice's comment that it will cause a lot of problems for SkyVerge's plugins, and I imagine there will be issues for other plugins that have already updated for 2.7 and miss this. It might be worth asking in the WooCommerce Slack about this to get quick straw poll on how big of an issue it would be.

@maxrice
Copy link
Contributor

maxrice commented Mar 7, 2017

I think it's important that our REST API works with a standard; UTC timestamps. Likewise I think it's important that the API reflects how the CRUD works. So I'd like to use this standard everywhere.

I'm all for a consistent standard, but IMO I don't think UTC timestamps are the right choice for the REST API/CRUD because the consumers of both generally desire a local time.

Think of a fulfillment provider that wants to display the date of an order that was fetched via the REST API. If you just provide the UTC timestamp, they'll need to convert that to a local time either using the info the user provided upon signup, or by fetching the site's timezone via the REST API.

However, if the REST API/CRUD methods returned ISO 8601 DateTimes, the timezone information is already present which allows them to display the local time. There could be separate convenience methods (and attributes in REST API responses) for getting the UTC DateTime.

I think there's two separate issues at play here:

  1. How does core store DateTime data?
  2. How does core return DateTime data?

There's no need for those to be consistent; in fact I'd argue that it's undesirable for them to be -- oftentimes the best way to store a piece of data is most certainly not the best way to return it to consumers of your REST/internal API.

I'd propose that core:

  1. store DateTimes as a unix timestamp (integer) and the current site's timezone (e.g. America/New_York) as a separate piece of meta on the order, for historical purposes.
  2. getters return DateTimes in ISO 8601 format.
  3. setters require DateTimes to be in ISO 8601 format so the correct adjustment to UTC can be made internally before storing.

In this proposal, the existing date methods that were added in 2.7 would change to return an ISO 8601 DateTime string, already adjusted for the site's timezone. The reason for this is that most semi-technical users and a lot of developers expect that using a method to get date information about an order would return it in the site's timezone.

Then, for both reasons of clarity and convenience, I think new methods should be added which explicitly return ISO 8601 DateTime strings in UTC time, e.g. $order->get_date_created_utc().

For any other areas that need unix timestamps (integers), passing an ISO 8601 DateTime string to strtotime() properly handles the timezone conversion to UTC. There's way more instances where we need to use actual DateTime strings vs. integer timestamps so I think requiring a bit of extra work there would be fine.

@mikejolley
Copy link
Member Author

@thenbrent The other option is dual purpose setters and getters, similar to current_time whereby they set and get local times by default, but you can pass in a flag for UTC.

Both would be stored in meta to avoid the bugs around time zone changes, but you'd only ever need to set once.

@thenbrent
Copy link
Contributor

thenbrent commented Mar 7, 2017

you can pass in a flag for UTC.

That works fine too and probably gets the best of both worlds (i.e. no new API methods, maintains backward compat by default, but make it possible to access UTC if required). 👍

you'd only ever need to set once.

I still worry about that. If I call set_date_paid( $timestamp, 'site' ) and find that get_date_paid( $context, 'gmt' ) returns a different value, I'd be surprised. I'd also be frustrated if I needed to update just one timestamp but not the other. Again, setting both in wc_create_order() 👍 , doing so via direct setter calls with explicit timezone 👎 .

@mikejolley
Copy link
Member Author

mikejolley commented Mar 7, 2017

@maxrice I believe ISO 8601 DateTimes are returned by our REST API, but @claudiosanches can confirm. For the CRUD, In all usage cases you'd end up using strtotime because you're very unlikely to consume or display the raw ISO 8601 DateTime as-is. I personally prefer timestamps there.

@maxrice
Copy link
Contributor

maxrice commented Mar 7, 2017

I believe ISO 8601 DateTimes are returned by our REST API, but @claudiosanches can confirm.

Yep, I think that's accurate. My point was in reference to your comment:

I think it's important that our REST API works with a standard; UTC timestamps.

adding UTC timestamps to the REST API would be fine, but ISO 8601 DateTimes should remain.

In all usage cases you'd end up using strtotime because you're very unlikely going to consume or display the raw ISO 8601 DateTime as-is

it depends right? ISO 8601 is, after all, the established standard for DateTime presentation so it's not like it's some rare or niche format (like say, mySQL's DateTime format). Returning DateTimes in that format gives you the most flexibility across all types of usage and also Just Works™ with a lot of other systems that also make use of/require ISO 8601 format.

The other big thing is that timestamps (by design) give you no information about the timezone; they indicate seconds elapsed from the Unix epoch. While the current date setters in 2.7 get this totally wrong, fixing them to be actual unix timestamps then require consumers to use date( ... ) everywhere they need an actual DateString, which is like 90% of the time. I don't think that's desirable.

One other thought -- if having an actual unix timestamp would be really convenient, then why not add another method? $order->get_date_created_timestamp() or similar?

@mikejolley
Copy link
Member Author

mikejolley commented Mar 7, 2017

I still worry about that. If I call set_date_paid( $timestamp, 'site' ) and find that get_date_paid( $context, 'gmt' ) returns a different value, I'd be surprised. I'd also be frustrated if I needed to update just one timestamp but not the other. Again, setting both in wc_create_order() 👍 , doing so via direct setter calls with explicit timezone 👎

@thenbrent I think it depends on how you're looking at this data. Is _gmt a representation of your local time? OR is your local time something you want to capture regardless of actual gmt time? I.e. we got X order at 3pm our time in whatever timezone we had at the time.

I'm sure there will be cases where one gets forgotten about and then you have non-matching dates. Does this matter?

Returning DateTimes in that format gives you the most flexibility across all types of usage and also Just Works™ with a lot of other systems that also make use of/require ISO 8601 format.

@maxrice That one will definitely screw up all extensions using the new date getters already. Is that an issue :)

Some ++ for unix times; they are numbers, easier to sanitize and validate, and more compact to store to the DB than strings. And you can pass them straight into date() which I think is 99% of all usage in core. Note; whatever format the getters return, the setters must accept.

@maxrice
Copy link
Contributor

maxrice commented Mar 7, 2017

That one will definitely screw up all extensions using the new date getters already. Is that an issue

IMO no, because this overall changes requires us to update all our usages anyhow, given that previously the timestamp returned was adjusted for the site's timezone and now it won't be. It would make things a bit easier for us as the majority of our usages require a DateTime string rather than a timestamp 😄

Some ++ for unix times; they are numbers, easier to sanitize and validate, and more compact to store to the DB than strings.

I agree with all of these things, which is why storing DateTimes as unix timestamps is 100% the right thing to do. But from the perspective of getting DateTimes, those things don't really matter and the considerations I mentioned above are more important.

And you can pass them straight into date() which I think is 99% of all usage in core.

Do you think core has/will have more usages of these date methods, or will the tens of thousands of extensions and custom code? Obviously they need to work for everyone, but just looking through the lens of "what's best for core" doesn't give you the full picture.

The one other thing I'll say here is that the date method names don't really indicate that they would even return a timestamp. When a method says "returns a date", I think a string, something human readable. When it says "returns a timestamp", I think an integer. So, from that perspective we might consider adding new methods like get_date_created_timestamp(), of which core would be a heavy user.

@thenbrent
Copy link
Contributor

@thenbrent I think it depends on how you're looking at this data. Is _gmt a representation of your local time?

No, it's a constant amount of time since the unix epoch that never changes, even if the site's timezone changes, or a stored representation of the equivalent date in the site's timezone changes.

(When I say GMT, I'm really referring to UTC+0. WP uses "GMT" despite the inaccuracies, so I've been using that name instead of saying UTC).

OR is your local time something you want to capture regardless of actual gmt time?

If I know my local timezone, I don't need a constant, because I can work from that, but we don't have that data for timestamps atm so a constant in UTC helps workaround issues with that. Using Max's suggestion to store the timezone separately is an alternative solution that obsoletes the need for the UTC timestamp.

I'm sure there will be cases where one gets forgotten about and then you have non-matching dates. Does this matter?

I'm sure there are also cases where people have accidentally called wp_update_post( array( 'post_date' => $new_date ) ) making the dates out of sync. But there are also legitimate cases where you want to update one without having to do a direct SQL query because the setter also updates the other.

Max's idea for storing the timezone resolves this issue. If the timezone is updated (possibly via an optional param) when set_date_*() is called, there is no UTC date stored and no reason to update it but it can still be accurately determined. It has other potential issues that will need to be accounted for, e.g. the timezone will need to be accurate enough to determine the UTC offset historically rather than currently to make sure it takes daylight savings into account (you can't just use a PHP timezone identifier like America/Los_Angeles). But those issues are easier to address this the duplication.

@mikejolley
Copy link
Member Author

No, it's a constant amount of time since the unix epoch that never changes, even if the site's timezone changes, or a stored representation of the equivalent date in the site's timezone changes.

I get that, reverse it then :D When we get the local time from an order, should it be the local time equivalent of the actual GMT value or doesn't it matter. i.e. should they always match up.

(When I say GMT, I'm really referring to UTC+0. WP uses "GMT" despite the inaccuracies, so I've been using that name instead of saying UTC).

To my understanding, GMT is an actual timezone whereas UTC is a standard. Thats probably why they use _gmt.

Max's idea for storing the timezone resolves this issue. If the timezone is updated (possibly via an optional param) when set_date_*() is called, there is no UTC date stored and no reason to update it but it can still be accurately determined. It has other potential issues that will need to be accounted for, e.g. the timezone will need to be accurate enough to determine the UTC offset historically rather than currently to make sure it takes daylight savings into account (you can't just use a PHP timezone identifier like America/Los_Angeles). But those issues are easier to address this the duplication.

I must say I do not like that idea at all. Everything we're discussing here is a crutch/hack/workaround to support the WordPress way of doing things when in realty we should be using UTC everywhere, and so should WP. Shame :(

To maintain BW compat we have to keep local times around, but going forward storing UTC makes way more sense than timezones.

@mikejolley
Copy link
Member Author

Claudio pointed out that WP_Date_Query/WP_Query uses the site timezone so looks like we cannot get around storing both even If I wanted to break the internet 👿

@thenbrent
Copy link
Contributor

thenbrent commented Mar 7, 2017

I get that, reverse it then :D When we get the local time from an order, should it be the local time equivalent of the actual GMT value or doesn't it matter. i.e. should they always match up.

If you don't have the timezone which the order was placed in, you can't accurately get the "local time equivalent of the actual GMT value", even if you have the GMT time. Hence you have to get whatever value is stored. And you won't know if they accurately match up. (You will know if they are out-of-sync if they're more than 24 hours apart, but that's the best you can determine).

@mikejolley
Copy link
Member Author

If you don't have the timezone which the order was placed in, you can't accurately get the "local time equivalent of the actual GMT value", even if you have the GMT time.

You can get the equivalent based on current timezone which is what counts.. Doesn't matter what the original timezone was?

@bryceadams
Copy link
Contributor

bryceadams commented Mar 7, 2017 via email

@maxrice
Copy link
Contributor

maxrice commented Mar 7, 2017

My proposal to store the timezone on an order at the time it's created is just for historical purposes. It wouldn't be associated with any of the dates specifically, e.g. in 2.7 the meta would be something like:

  • _date_paid: 1488924139
  • ...etc

Then, there would be one additional meta for the site timezone:

  • _store_timezone: America/New_York

As @thenbrent mentioned, storing the PHP timezone identifier may not be the best, so you could use UTC offsets or something, but ultimately you want to store something so you can answer the question "What was the local time of this order at the time it was placed?".

This allows you to maintain historical accuracy in case the merchant later changes the site's timezone (which probably happens more often than we think given how easy it is to get started -- imagine a merchant that starts a clothing store in NYC but moves to LA to be closer to their suppliers). Honestly I can't really see any reason to not store this information. It's there if you need it, but otherwise has zero influence.

RE: the REST API, I think having both is the way to go, e.g. created_at returns local time in ISO 8601 and created_at_utc returns UTC time in ISO 8601. You could even change that to created_at_timestamp which returns a unix timestamp if you think that'd be more convenient for clients that want UTC time.

@mikejolley
Copy link
Member Author

mikejolley commented Mar 7, 2017

@maxrice Is that _date_paid example UTC timestamp or site timezone timestamp? What about people bypassing CRUD and using get_post_meta? CRUD would need to handle conversions so they would miss this? And values stored in 2.6.x already what about those?

@maxrice
Copy link
Contributor

maxrice commented Mar 7, 2017

Is that _date_paid example UTC timestamp or site timezone timestamp?

UTC timestamp. I think using the word "timestamp" (and meaning an integer timestamp) to represent anything other than a unix timestamp is fraught with all sorts of peril. The unix timestamp works because there's no question as to the timezone (it has none), but if you say "oh it's a site timezone timestamp", it's still an integer but now I don't what timezone it's associated with. The current site's timezone? Or the one that it was at the time the order was placed? How do I know that it has (or hasn't) changed since then? No way to know.

What about people bypassing CRUD and using get_post_meta? Does that matter?

This is an issue already, no? If I have custom code that uses get_post_meta( $order->id, '_paid_date', true ), in 2.6 it returns a mySQL DateTime string (in the site's timezone), but in 2.7 it returns an integer timestamp (also in the site's timezone, at least as of RC1). So the date handling code, as-is, breaks backwards compat and requires changes regardless.

One possibility to maintain backward compat here would be to add both the old meta and new meta, e.g. for an order placed in 2.7 you'd have something like:

  • _date_created
  • _date_updated
  • _date_paid
  • _date_completed

and

  • _completed_date
  • _paid_date

That new meta would use unix timestamps and the core code would use them to get/set dates. At the same time, core would also set that old meta to the same format that was used previously so custom code would continue to function. In some future release, you'd stop adding the old meta. I think this would work, at the risk of having yet more meta inserts which I know is a sore spot for WC's performance atm.

The one last thought here is that if core switched to new meta, I think you could then filter get_post_meta calls for that old meta and throw a deprecated notice. This would give people fair notice that they need to update their code to use the new CRUD API and then it could be removed in a later version when core stops adding that old meta.

@mikejolley
Copy link
Member Author

This is an issue already, no? If I have custom code that uses get_post_meta( $order->id, '_paid_date', true ), in 2.6 it returns a mySQL DateTime string (in the site's timezone), but in 2.7 it returns an integer timestamp (also in the site's timezone, at least as of RC1). So the date handling code, as-is, breaks backwards compat and requires changes regardless.

I had not noticed this. Should NOT be the case. This should be fixed up - @thenbrent is this one of the changes you've had to deal with?

@claudiosanches
Copy link
Contributor

Nice post about the best way of storing date and time in database: https://derickrethans.nl/storing-date-time-in-database.html
And this describe how it's hard to keep consistency when you only save timestamps and timezone.

@thenbrent
Copy link
Contributor

I had not noticed this. Should NOT be the case. This should be fixed up - @thenbrent is this one of the changes you've had to deal with?

Yes. I assumed it was a deliberate change.

You can get the equivalent based on current timezone which is what counts.. Doesn't matter what the original timezone was?

That depends on what the date is being used for. It certainly can't be relied on for any important logic. But it even causes issues for display. For example, if the site's timezone has daylight savings time, even without an admin changing the site's timezone, then the order dates in the site timezone will all be 1+ hours different depending on whether they are seen 1 minute before or after midnight on the day DST kicks in.

@maxrice
Copy link
Contributor

maxrice commented Mar 8, 2017

And this describe how it's hard to keep consistency when you only save timestamps and timezone.

My takeaway from that post is that saving a timestamp + timezone is good enough, so long as you understand the limitations. Given that my suggestion of storing the timezone identifier would only be for historical purposes, the responsibility of correctly determining the local time for an order when it was placed (assuming the site's timezone has since changed) would be on the developer attempting to do that, not core.

I don't think that at this point we have time to investigating the suggestions from the post for overcoming those limitations, not least of which is a lot of the fancier Date manipulation functions require PHP 5.3+ 😦

@manospsyx
Copy link
Member

I am not working with dates in my extensions, but here's my take:

What to store?

+1 for UTC timestamps. Also agree it makes a lot of sense to store something that will allow converting that timestamp to local shop time (when the order was placed). Either a timestamp offset or a time zone code.

Regarding the issue of meta + backwards compatibility, I'd lean towards what Max suggested:

One possibility to maintain backward compat here would be to add both the old meta and new meta

Eventually, legacy date meta could be deleted to encourage use of the CRUD API before moving data to new tables (~2.9?).

What to return?

The other option is dual purpose setters and getters, similar to current_time whereby they set and get local times by default, but you can pass in a flag for UTC.

+1

What's important here is that these methods are backwards compatible (just a matter of choosing the right default flag value?). This should extend to the API (each API version should still return exactly what it did before). Regarding getters + setters, I'd love to have the ability to get/set in:

i) UTC timestamp,
ii) date format in UTC+0 and
iii) date format in the local shop time zone when the order was placed.

@mikejolley
Copy link
Member Author

mikejolley commented Mar 8, 2017

something that will allow converting that timestamp to local shop time (when the order was placed)

I actually think this is a separate, inherent issue from WP we shouldn't try to account for in these methods. If a store owner changes the store timezone for any reason, they should expect displayed dates to change if this is modified. It should not be out place to track changes over time. Let's not forget post dates suffer the same limitation.

What UTC timestamps do solve is that you can accurately get the date even if that timezone IS changed. It doesn't matter what timezone the admin sets now or in the future, you have a UTC timestamp, this is accurate regardless. This is what we are missing. A UTC timestamp. Because right now, dates are stored in whatever timezone WP uses. If you have a UTC timestamp, you don't need to store timezone at time, or handle conversions and offsets. @franticpsyx

The summer time abnormalities I think Brent was trying to explain are (to my knowledge) handled on the PHP side when converting a UTC timestamp. I.e. if the UTC timestamp falls within British Summer Time, the date function will subtract/add 1 hour accordingly, IF using the London timezone. The store admin does not need to change the clocks when BST starts if using the London timezone.

@ragulka
Copy link
Contributor

ragulka commented Mar 8, 2017

FWIW, I don't see a lot of value of storing historical timezones/local datetimes. I personally cannot think of an use case for "what was the local datetime when this order was placed, given that we have changed our WP timezone since?"

The beauty of storing dates in UTC is that you can always get the local datetime from it. And you only ever need to know the current timezone when displaying the date/time. If the site's timezone is changed at some point, you will most likely want to see order dates in the new timezone rather than in the timezone that the site used at the moment the orders were created.

@mikejolley
Copy link
Member Author

I'm experimenting with this so not final, but here are my thoughts working towards a solution.

TO MAINTAIN BACKWARDS COMPATIBILITY

  • Meta data that existed in 2.6.x must be stored in the same format as 2.6.x with the same timezone settings.
  • Direct property accessors need to return the data in the same format as 2.6.x with the same timezone settings rather than use the getters format.

GETTER/SETTER METHODS

Setter:

	/**
	 * Set date_x.
	 *
	 * @param string|integer $date UTC timestamp, or ISO 8601 DateTime. If the DateTime string has no timezone information, WordPress site timezone will be assumed.	 * @throws WC_Data_Exception
	 */
	public function set_date_x( $date ) {
		$this->set_prop( 'date_x', new WC_DateTime( is_numeric( $date ) ? '@' . $date : $date ) );
	}

Getter:

	/**
	 * Get date_x.
	 *
	 * @param  string $context
	 * @return WC_DateTime object
	 */
	public function get_date_x( $context = 'view' ) {
		return $this->get_prop( 'date_x', $context );
	}

STORAGE

  • New meta data (2.7.x) stored as a UTC timestamp.
  • Old meta data (2.6.x) stored in 2.6.x format - either as a mysql date string or LOCAL Timestamp.

USAGE

The getter will return something other than a timestamp since a timestamp (in the case of WP anyway) is ambiguous.

Option #1: Return a DATETIME object (timezone either local or UTC - undecided).

Example:

printf( '<time datetime="%s">%s</time>', esc_attr( $the_order->get_date_created()->format( 'c' ) ), esc_html( date_i18n( __( 'Y-m-d', 'woocommerce' ), strtotime( $the_order->get_date_created()->format( 'c' ) ) ) ) );

Since it's an object, you can change timezones, format, get timestamps or whatever you need.

Option #2: Return an ISO 8601 string formatted to blog timezone.

printf( '<time datetime="%s">%s</time>', esc_attr( date( 'c', strtotime( $the_order->get_date_created() ) ) ), esc_html( date_i18n( __( 'Y-m-d', 'woocommerce' ), strtotime( $the_order->get_date_created() ) ) ) );

Working with strings only so will require the use of strtotime for timestamps/date functions.

@mikejolley
Copy link
Member Author

^ with option 1 and the datetime wrapper we'd have opportunity to help with UTC timestamps, and offset timestamps with the WP timezone considered.

Similar to the examples in @maxrice's article https://www.skyverge.com/blog/down-the-rabbit-hole-wordpress-and-timezones/, we could have something like this:

class WC_DateTime extends DateTime {

	/**
	 * Missing in PHP 5.2.
	 * @return int
	 */
	public function getTimestamp() {
		return method_exists( 'DateTime', 'getTimestamp' ) ? parent::getTimestamp() : $this->format( 'U' );
	}

	/**
	 * Get the timestamp with the WordPress timezone offset added or subtracted.
	 * @return int
	 */
	public function getOffsetTimestamp() {
		return $this->getTimestamp() + $this->getOffset();
	}
}

This gives you an easy way to get either timestamp without needed to convert a datetime string manually.

@UserName011
Copy link

UserName011 commented Mar 8, 2017

@claudiosanches There is nothing wrong with saving timestamps and timezone.

It's important to understand that MySQL always stores the date and time in UTC. regardless of how you want to see the date/time.
When you do SELECT NOW() what actually happens is that MySQL counts the number of seconds from 1970-01-01 00:00:01 to now and calculates the result plus your timezone configured.
when you do SELECT UTC_TIMESTAMP() it does the same and just ignore the timezone setting.

What I am trying to say here is that it doesn't matter how you define the field. As long as the information is there you can always convert between the types.

Anyhow there are two ways to solve it easily:

  1. after timezone has changed perform in MySQL:
    SET time_zone = 'proper timezone';
    This will do automatic conversions for TIMESTAMP fields and affect the results of NOW() and CURDATE() functions. Important to note that this will not automatically convert DATE, TIME, and DATETIME fields.
    So it depends what you will use.

  2. Use VIEW to wrap your tables..

Assume this is the table in question:

CREATE TABLE `SomeDateTable` (
  `id`    int(11) NOT NULL auto_increment,
  `value` float NOT NULL default '0',
  `date_register`  datetime NOT NULL default '0000-00-00 00:00:00',
  PRIMARY KEY  (`id`)
)

Wrap it with VIEW:

CREATE VIEW view_SomeDateTable AS
SELECT id,
            value,
           CONVERT_TZ( date_register, 'UTC', 'Europe/Stockholm' ) as date
FROM SomeDateTable

Now instead of doing
select date from SomeDateTable where id=1
do:
select date from view_SomeDateTable where id=1

The Europe/Stockholm can be changed to function as a variable selecting the proper timezone from the database settings / stored value from table.

This solution means that you don't need to change the data stored in your tables.. You just wrap it with the proper display.

I would highly recommend that WooCommerce will always work with one basic time zone. All information will be saved,processed,updated with this time zone and anytime the user asks to display he would see the result after conversion. This also means that when user gives input before storing the data it has to be converted from his known time zone to the WooCommerce basic time zone.

@claudiosanches
Copy link
Contributor

claudiosanches commented Mar 8, 2017

@UserName011 sorry, but we are not talking about how MySQL store it.
We are talking if should about store an integer as a timestamps or a string in date/time format, but not the MySQL fields for it, since people can just use other databases too.

@mikejolley
Copy link
Member Author

mikejolley commented Mar 8, 2017

@thenbrent @franticpsyx @maxrice @bekarice @justinshreve Take a quick look over #13512. This idea returns datetime objects, and accepts strings and timestamps on the setter with timezone conversion.

Uses post_date_gmt and post_modified_gmt on read.

@maxrice
Copy link
Contributor

maxrice commented Mar 8, 2017

@ragulka

FWIW, I don't see a lot of value of storing historical timezones/local datetimes.

@thenbrent may have a specific use case for this sort of thing, but my broader point is that if you don't store the site timezone string at the time the order is placed, then there is absolutely no way for you to come back to that order and know what the local time was, with 100% certainty. There's literally no downside to storing it, but a huge potential headache if you don't.

To be clear, I don't think that core should have any code or handling for it (i.e. no methods that convert any of the order dates into local time from when the order was placed, no getters for it, etc). It's there for historical purposes and if a developer wants/needs to figure out that local time, they can use the timestamp + unix timestamp to figure it out themselves.

The best way to think of it is is like insurance -- we'd pay an incredibly small premium to store it, with a potentially large payoff if it's ever needed in the future.

@mikejolley your proposed solution is looking good to me, a few comments:

Old meta data (2.6.x) stored in 2.6.x format - either as a mysql date string or LOCAL Timestamp

This must be stored as a mySQL date string, no? Otherwise custom code using get_post_meta() will break if a "local timestamp" (integer) is returned.

Return a DATETIME object (timezone either local or UTC - undecided).

I really like this, especially with with a DateTime class wrapper. I would strongly recommend defaulting the timezone to local, as that's what most usages would require. Adding a convenience method for returning the UTC time would be easy.

I'd also love to see ISO 8601 declared throughout the DateTime class wrapper as the "standard" for DateTime strings, and then a convenience __toString() method added so that doing something like echo $order->get_date_created(); would return a string like '2017-03-08T12:12:01-05:00'

@mikejolley
Copy link
Member Author

There's literally no downside to storing it, but a huge potential headache if you don't.

Not entirely true. Thats one new postmeta string for each and every order, and more data to migrate and handle in the future if it needs to stick around.

This must be stored as a mySQL date string, no? Otherwise custom code using get_post_meta() will break if a "local timestamp" (integer) is returned.

It needs to be stored however it was in 2.6.x regardless. We can handle those abnormalities in the data stores.

then a convenience __toString() method added so that doing something like echo $order->get_date_created(); would return a string like '2017-03-08T12:12:01-05:00'

+1

I would strongly recommend defaulting the timezone to local, as that's what most usages would require

Thats handled in the set_date_prop method. It does default to local - tests pass confirming this. I borrowed the code from your blog post.

@DavidAnderson684
Copy link
Contributor

However this ends up being done, please add a note to the developer migration documentation.... I'd assumed that the change of behaviour in RC1 was intentional, so coded for it, so will need to look at that again.

@mikejolley
Copy link
Member Author

@DavidAnderson684 Will do. If you have feedback on the approach please share.

@unfulvio
Copy link
Member

unfulvio commented Mar 9, 2017

@ragulka @maxrice @mikejolley

I don't see either much benefit from storing a timezone -- perhaps if there is a use case or there will be in future some support for multisite/multiple shops in a single install where having this sort of information will be of some value...?

@mikejolley mikejolley added the type: epic Container issue with high-level description of work that will be done in sprint. label Mar 9, 2017
@DavidAnderson684
Copy link
Contributor

@mikejolley My take: I agree with @maxrice - as a developer, I just want the (unambiguous, well-understood, not-requiring-constant-trips-to-the-PHP-manual) epoch time. Any timezone handling and database wrangling that has to be done to deal with the legacy problems should be internal to WC, and invisible to the CRUD API consumer. Since timezones are to do with display (and since the shop-timezone-at-order-time has no relevance to anything), they shouldn't be appearing in API calls or results.

It sounds like this will involve some unpleasant redundancy in the database to allow people accessing the postmeta table directly to see what they expect. That's unfortunate, but since there's a plan to stop using postmeta entirely in future, it's at least a piece of ugliness that has an end date on it...?

@Nebrassk
Copy link

Just see the date and time of the device you are using

I lasted 6 hours solving the problem I wasted my time and when I ran out of patience, by chance, I tried changing the computer's time, as it was different, the problem was solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug The issue is a confirmed bug. type: epic Container issue with high-level description of work that will be done in sprint.
Projects
None yet
Development

No branches or pull requests