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

No way to pass DateTime with custom TimeZone to isDue #78

Closed
taylorotwell opened this issue Feb 27, 2015 · 10 comments
Closed

No way to pass DateTime with custom TimeZone to isDue #78

taylorotwell opened this issue Feb 27, 2015 · 10 comments

Comments

@taylorotwell
Copy link

In Laravel's scheduler component, we allow the user to specify a Timezone to evaluate the Cron against. However, cron-expression appears to always overwrite the timezone to the current application timezone, allowing no custom timezone to be set.

Is there any way this can be resolved?

@mtdowling
Copy link
Owner

I don't remember exactly why this was added really. This is what I got from the blame: cbde1b5

I guess it's trying to normalize the timezone. I'm happy to make a change if needed.

@taylorotwell
Copy link
Author

It appears we can maybe work around it on our end as well by passing a string representation instead of a DateTime object, though it does seem intuitively like the Timezone of the given DateTime object should be respected.

@taylorotwell
Copy link
Author

Maybe I'm wrong there. Here is the PR someone is working on our side (laravel/framework#7636) ... personally I kind of wonder if it should be fixed over here though.

@swekaj
Copy link

swekaj commented Feb 27, 2015

I agree it should be fixed here. The library shouldn't be changing the timezone without giving the user the option to specify the timezone it changes to. It's very important to use the correct timezone when checking to see if a cron expression is due since the expressions themselves are not timezone-aware. If you create expressions for GMT-8:00, then the library will always be off by 8 hours if it's converting any DateTimes to GMT/UTC.

I remember a year or so ago, I modified my local copy of the library to accept an optional timezone in the CronExression::factory() method. That timezone was then used for any ::isDue() and similar calls. Unfortunately that work has been lost (I had forgotten I did that when I updated to using composer), but I'd be willing to recreate it.

I think it'd be most useful to allow providing a default timezone in the ::factory() method as well as allowing the user to override that by passing in a DateTime with the appropriate timezone set in ::isDue() and similar. If no default timezone is supplied, then the system's default is used where necessary.

I could probably work on this this weekend, if you'd like.

@DerManoMann
Copy link
Contributor

Looks like this is not handled consistently across all of the public API.
getRunDate() does just clone the given date/time if it is a DateTime instance. So, calling getMultipleRunDates() with a properly set up $currentTime will respect the timezone as given on $currentTime.
I remember that was something I was working towards as I also use this to evaluate against different timezones.
My stand is that if a DateTime instance is given, the timezone set in that instance should be used. For strings or 'now' the default timezone should be used.

@taylorotwell
Copy link
Author

Is there a suggested fix for this? Anything I can do?

@mtdowling
Copy link
Owner

It can be fixed. We need a PR that removes all of the timezone modifications from the library and any time a DateTime object will be modified, it's cloned.

@taylorotwell
Copy link
Author

We fixed it on our end for now by passing a string representation of the
time.

On Thu, Mar 26, 2015 at 11:38 AM, Michael Dowling notifications@github.com
wrote:

It can be fixed. We need a PR that removes all of the timezone
modifications from the library and any time a DateTime object will be
modified, it's cloned.


Reply to this email directly or view it on GitHub
#78 (comment)
.

@laurencei
Copy link
Contributor

@mtdowling @dragonmantank - it looks like someone has made a PR for this issue here #115

Any chance of reviewing it - and potentially merging?

I've run into the same issue as @taylorotwell (ironically for inside a Laravel application) that I really need to support custom timezones...

@dragonmantank
Copy link
Collaborator

This should have been corrected with #134, so closing.

peter279k pushed a commit to peter279k/cron-expression that referenced this issue Nov 25, 2020
This commit is part of a campaign to reduce the amount of data transferred to save global bandwidth and reduce the amount of CO2. See Codeception/Codeception#5527 for more info.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants