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

[7.x] Add a more generic minute schedule method #33380

Closed
wants to merge 1 commit into from

Conversation

browner12
Copy link
Contributor

this PR was inspired by #33379

Rather than adding a method for every iteration of minute intervals, the thought is to have one method that accepts a parameter.

If there are any suggestions for a better method name, definitely open to them.

The only concern here is that we make it clear in the documentation how this actually works, specifically with values that are not factors of 60. For example, a value of */17 will run at the 0, 17, 34, and 51 minutes, and then start over again at zero on the next hour, so the expressions is not truly running every 17th minute.

If we can come up with a readable name, we could possibly deprecate and remove the existing function like ->everyFiveMinutes(), ->everyFifteenMinutes(), etc.

the method accepts any integer to splice into the "minute" portion of the cron command
@driesvints
Copy link
Member

Think the existing everyFiveMinutes, etc methods can stay but they can internally re-use your method?

@browner12
Copy link
Contributor Author

Hmmm... the tradeoff is you're introducing another method to the call stack by doing that with the hope of keeping things DRY for better maintenance.

Normally I'd be on the DRY side of that argument, but it feels like you gain so little in this scenario, I might lean towards keeping the extra method call out of the call stack.

I'm fine either way on that, I'll leave that decision up to you guys.

@taylorotwell
Copy link
Member

The odd numbers (like 17) make me want to avoid this.

@browner12
Copy link
Contributor Author

what if we limit it to only factors of 60?

1, 2, 3, 4, 5, 6, 10, 12, 15, 20, 30, 60

if (60 % $minute !== 0) { 
    thrown new Exception();
}

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.

3 participants