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

[6.x] Fix defaultTimezone not respected in scheduled Events #33834

Merged
merged 1 commit into from Aug 12, 2020

Conversation

sodoardi
Copy link
Contributor

Update CallbackEvent
-> CallbackEvents were not injected with the timezone on construction making them ignore the schedulers timezone on $event->between() settings

@GrahamCampbell GrahamCampbell changed the base branch from 6.x to master August 12, 2020 07:48
@GrahamCampbell GrahamCampbell changed the title [6.x] fix that causes defaultTimezone not to be respected in scheduled Events [8.x] Fix defaultTimezone not respected in scheduled Events Aug 12, 2020
@GrahamCampbell
Copy link
Member

I have re-targetted to Laravel 8, because upgrading to version 3 of the cron package is a major breaking change, because they have changed how things work since version 2, and we already upgraded Laravel 8 only, for that reason.

@GrahamCampbell
Copy link
Member

I have also rebased your PR, so the composer.json updates don't show up anymore.

@GrahamCampbell GrahamCampbell force-pushed the scheduleTimezone6x branch 2 times, most recently from 45b8f83 to 00eeef9 Compare August 12, 2020 07:55
@sodoardi
Copy link
Contributor Author

@GrahamCampbell I took out the the cron package change and created a new PR without the update, this is a new PR with only the CallbackEvent changes

Co-Authored-By: Stephen Odoardi <sodoardi@users.noreply.github.com>
@GrahamCampbell GrahamCampbell changed the title [8.x] Fix defaultTimezone not respected in scheduled Events [6.x] Fix defaultTimezone not respected in scheduled Events Aug 12, 2020
@GrahamCampbell GrahamCampbell changed the base branch from master to 6.x August 12, 2020 09:13
@GrahamCampbell
Copy link
Member

Ah, I see, so you're saying the upgraded version was not necessary?

@GrahamCampbell GrahamCampbell marked this pull request as draft August 12, 2020 09:14
@sodoardi
Copy link
Contributor Author

Ah, I see, so you're saying the upgraded version was not necessary?

It is necessary for the other bug :), which affects all dailyAt() etc. which evaluate in the cron expression
this PR fixes the bug regarding between() for CallbackEvents which happens in the filter

@GrahamCampbell
Copy link
Member

It is necessary for the other bug

Right, I see. I don't think the upstream package authors considered that to be a bug, but decided to change the behaviour in v3 to "what you would expect", and considered it a breaking change. For this reason, you will have to wait till Laravel 8 to get v3 of that package. It should still be possible to work around the issue though. There should be some information about the changes on the upstream repo. :)

@GrahamCampbell GrahamCampbell marked this pull request as ready for review August 12, 2020 11:38
@sodoardi
Copy link
Contributor Author

sodoardi commented Aug 12, 2020

Right, I see. I don't think the upstream package authors considered that to be a bug,

yea unfortunately they fixed together with the major change, I asked them to backport the fix to the 2.x versions, lets see. Its in the changelog fixes:

  • Fixed issue where logic for dropping seconds to 0 could lead to a timezone change

but decided to change the behaviour in v3 to "what you would expect", and considered it a breaking change. For this reason, you will have to wait till Laravel 8 to get v3 of that package. It should still be possible to work around the issue though. There should be some information about the changes on the upstream repo. :)

in my project im doing the good old dragonmark/cron-expression:"3.0.0 as 2.4.0" hack, since the breaking change in behaviour doesn't affect my project

@taylorotwell taylorotwell merged commit 60c7f31 into laravel:6.x Aug 12, 2020
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.

None yet

3 participants