If recurring periods are to properly support timezones then the recurring period plugin must know what timezone it is dealing with. #2912347: Use \DateTimeImmutable rather than timestamp for all time calculations adds support for this by using DateTimeImmutable objects instead of timestamps. That change would make the caller of the plugin (in this case commerce_licence) responsible for supplying the correct timezone.

CommentFileSizeAuthor
#4 2914350-4-commerce-licence-timezones.patch6.16 KBerik.erskine
#2 2914350-2-commerce-licence-timezones.patch2.5 KBerik.erskine

Comments

erik.erskine created an issue. See original summary.

erik.erskine’s picture

erik.erskine commented
Status: Active » Needs review
StatusFileSize
new 2914350-2-commerce-licence-timezones.patch2.5 KB

This patch updates commerce licence to reflect the API change in #2912347: Use \DateTimeImmutable rather than timestamp for all time calculations and use a timezone appropriate to the licence owner. It modifies the Licence::calculateExpirationTime function to pass a timezone to the recurring period plugin. Both start and expiry values are still persisted as UTC timestamps.

That's a start, but it's possible that this behaviour is desired for some licences, whereas others should be based on the site timezone, UTC, or something else. Do we need some kind of selection for that, in a similar way to how date fields are handled?

Status: Needs review » Needs work

The last submitted patch, 2: 2914350-2-commerce-licence-timezones.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

erik.erskine’s picture

erik.erskine commented
StatusFileSize
new 2914350-4-commerce-licence-timezones.patch6.16 KB

Updated #2 to fix tests, with the following changes:

  • calculateExpirationTime now returns NULL rather than 0 if there is no expiration date
  • Create a user in tests that define new licenses - this is needed to determine the timezone
erik.erskine’s picture

erik.erskine commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: 2914350-4-commerce-licence-timezones.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

  • joachim committed 292bc09 on 8.x-2.x authored by erik.erskine 
    Issue #2914350 by erik.erskine: Changed licence expiry calculation to be...
joachim’s picture

joachim commented
Status: Needs work » Fixed

Committed with a few tweaks:

- I don't think this issue should be changing the value we store for an unlimited license
- fixes to docs formatting
- tweak to tests -- we don't need to bother with user storage, just use createUser()
- not sure why there was a (string) cast in commerce_licence_get_user_timezone(). Generally, it's a real shame we have to have this -- core should provide something like this!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.