Doesnt handle daylight savings

[calumbrodie]

Currently if a recurring job is scheduled to be run at 1:30AM (for example) and daylight savings moves the time back at this time at 2am - it will run twice.

Similarly a job scheduled to run at the same time, wont run when the clocks move forward.

[gsdevme]

Are you sure this happens? My thought was crontab handles this edge case

[calumbrodie]

but crontab just runs every minute in this bundle. crontab does actually handle this properly but seeing as this bundle replaces crontab it should try and handle this...

[gsdevme]

Crontab does not have to be timezone aware, infact the default is for crontab to run in UTC mode just many linux vendors change it (as per my understanding).

Simple solution would be to restore crontab to run in UTC.

[calumbrodie]

Not sure you understand the issue. The way this bundle works is by running a job every minute (via crontab).

https://github.com/usemarkup/JobQueueBundle/blob/master/Command/AddRecurringConsoleJobToQueueCommand.php

Edit: see mtdowling/cron-expression#116

[gsdevme]

Doh "This isn't correct" moment here... I see. Should the JobQueueBundler perhaps do its calculations in UTC then? You specific a timezone say Europe/London to the bundle and it converts the yml into UTC then does a check?

DateTime would handle the edge case for us and always give us the correct UTC value (in theory)

[calumbrodie]

@gsdevme the issue is that the 'isDue' will just use the timezone settings of PHP which will change the time when the time changes.

[gsdevme]

Yeah isDue() needs to calculate in UTC and the yml is in local time and converted to UTC

[gsdevme]

I mean like this https://3v4l.org/CWaIZ not sure if Im being clear (the clock is ticking UTC here though maybe also should be changed)

[calumbrodie]

But it would require not using Cron/CronExpression because it has no support to pass in TimeZone...

edit: https://github.com/mtdowling/cron-expression/blob/master/src/Cron/CronExpression.php#L287

[gsdevme]

https://github.com/mtdowling/cron-expression/blob/master/src/Cron/CronExpression.php#L276`

    public function isDue($currentTime = 'now')
    {
        if ('now' === $currentTime) {
            $currentDate = date('Y-m-d H:i');
            $currentTime = strtotime($currentDate);
        } elseif ($currentTime instanceof DateTime) {
            $currentDate = clone $currentTime;
            // Ensure time in 'current' timezone is used
            $currentDate->setTimezone(new DateTimeZone(date_default_timezone_get()));
            $currentDate = $currentDate->format('Y-m-d H:i');
            $currentTime = strtotime($currentDate);
        } elseif ($currentTime instanceof DateTimeImmutable) {
            $currentDate = DateTime::createFromFormat('U', $currentTime->format('U'));
            $currentDate->setTimezone(new DateTimeZone(date_default_timezone_get()));
            $currentDate = $currentDate->format('Y-m-d H:i');
            $currentTime = strtotime($currentDate);
        } else {
            $currentTime = new DateTime($currentTime);
            $currentTime->setTime($currentTime->format('H'), $currentTime->format('i'), 0);
            $currentDate = $currentTime->format('Y-m-d H:i');
            $currentTime = $currentTime->getTimeStamp();
        }
        try {
            return $this->getNextRunDate($currentDate, 0, true)->getTimestamp() == $currentTime;
        } catch (Exception $e) {
            return false;
        }
    }

Problem is this library isn't doing anything wrong really..

            // Ensure time in 'current' timezone is used
            $currentDate->setTimezone(new DateTimeZone(date_default_timezone_get()));

29th of March 2016 at 1am happens twice... so his library is telling the truth but for our use case we don't want this... I would say this libraries isDue() is not fit for our purpose.

Bin the library or fork it :) and make it UTC only.

[calumbrodie]

Would rather encourage a PR to the bundle to allow an optionally different behaviour - like being able to set a DateTimeZone against the cron expression which overrides 'date_default_timezone_get()' if that has been set.

Wouldn't it be better for us to assume all jobs are defined in .yml as UTC - and convert them to the current timezone before sending to IsDue?

Reason being this would be much more compatible with this other bundle (which I'm not going to bin as it's a well maintained bundle)

[gsdevme]

That library is doing nothing wrong though, I doubt the PR would get accepted.

I think our use case for the library is wrong. The 29th of March 1am happens twice in the UK.. That's a fact.

Sent from my iPhone

On 18 Jun 2016, at 16:23, Calum Brodie notifications@github.com wrote:

Would rather encourage a PR to the bundle to allow an optionally different behaviour - like being able to set a DateTimeZone against the cron expression which overrides 'date_default_timezone_get()' if that has been set.

Wouldn't it be better for us to assume all jobs are defined in .yml as UTC - and convert them to the current timezone before sending to IsDue?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.