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

[8.x] Use a more appropriate test for yearlyOn() #34772

Closed
wants to merge 1 commit into from
Closed

[8.x] Use a more appropriate test for yearlyOn() #34772

wants to merge 1 commit into from

Conversation

u01jmg3
Copy link
Contributor

@u01jmg3 u01jmg3 commented Oct 9, 2020

If you wanted to schedule a task to run specifically on a Tuesday, you wouldn't also define a day of the month as this may not always fall on the desired day of the week. I've therefore updated the test to reflect this.

Relates to #34728

@driesvints driesvints changed the title Use a more appropriate test for yearlyOn() [8.x] Use a more appropriate test for yearlyOn() Oct 9, 2020
@taylorotwell
Copy link
Member

Not sure this is correct.

@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Oct 9, 2020

@taylorotwell

$schedule->mondays()->yearlyOn(7, 20, '09:01') equates to the expression 1 9 20 7 1

  • As per Crontab guru:

    2021-07-05 09:01:00 // Monday
    2021-07-12 09:01:00 // Monday
    2021-07-19 09:01:00 // Monday
    2021-07-20 09:01:00 // Tuesday <-- isn't on a Monday
    2021-07-26 09:01:00 // Monday
    

If we've told the scheduler we are only interested in Mondays, I wouldn't have expected the Tuesday date (20/7). In which case, the expression 1 9 * 7 1 makes more sense. i.e. don't define both a day of the week and a day of the month in the same expression


Relating this to the existing test, in 2022 this is the output.

$schedule->tuesdays()->yearlyOn(7, 20, '09:01')

2022-07-05 09:01:00 // Tuesday
2022-07-12 09:01:00 // Tuesday
2022-07-19 09:01:00 // Tuesday
2022-07-20 09:01:00 // Wednesday <-- isn't on a Tuesday
2022-07-26 09:01:00 // Tuesday

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.

2 participants