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

[5.4] Add Event timezone in inTimeInterval $now #19959

Merged
merged 5 commits into from
Jul 10, 2017
Merged

[5.4] Add Event timezone in inTimeInterval $now #19959

merged 5 commits into from
Jul 10, 2017

Conversation

pavinthan
Copy link
Contributor

It's will reduce timezone issue in scheduling, please have a look into this.

It's will reduce timezone issue in scheduling, please have a look into this.
@tillkruss
Copy link
Contributor

Can you add a test for this, please?

pavinthan added 2 commits July 8, 2017 12:00
replace strtotime with Carbon parse.
Test case updated for Event timezone in inTimeInterval .
@pavinthan
Copy link
Contributor Author

I've updated the test, please have a look into this, Thanks.

@@ -53,9 +53,11 @@ public function unlessBetween($startTime, $endTime)
private function inTimeInterval($startTime, $endTime)
{
return function () use ($startTime, $endTime) {
$now = Carbon::now()->getTimestamp();
$now = Carbon::now($this->timezone)->getTimestamp();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just :

return Carbon::now($this->timezone)->between(
    Carbon::parse($startTime, $this->timezone),
    Carbon::parse($endTime, $this->timezone),
    true
);

?

pavinthan added 2 commits July 8, 2017 13:36
Event time check method replace with carbon between.
@pavinthan
Copy link
Contributor Author

@lucasmichot 👍

@tillkruss tillkruss changed the title Add Event timezone in inTimeInterval $now [5.4] Add Event timezone in inTimeInterval $now Jul 8, 2017
@taylorotwell
Copy link
Member

This changes code unrelated to the timezone. Only make the change specified in the title.

@themsaid
Copy link
Member

This fixes the following case:

$schedule->command('inspire')
    ->everyMinute()
    ->timezone('Africa/Cairo')
    ->between('2017-07-10 14:25', '2017-07-10 15:25');

At 2017-07-10 14:25 the UTC time is 2017-07-10 12:25, without this PR we currently compare the time without respect to the specified timezone, so this command will wait until UTC time is 14:25 which is two hours after it should run.

I believe this fix is ok.

@taylorotwell taylorotwell merged commit cfe8dee into laravel:5.4 Jul 10, 2017
@pavinthan
Copy link
Contributor Author

Thank you guys 👍

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

Successfully merging this pull request may close these issues.

5 participants