dragonmantank / cron-expression

CRON for PHP: Calculate the next or previous run date and determine if a CRON expression is due
MIT License
4.57k stars 124 forks source link

Offset from cron date #120

Open bilogic opened 3 years ago

bilogic commented 3 years ago

Hi,

  1. I need to collect on invoices every quarter, i.e. 1st of Jan/Apr/Jul/Oct.
  2. I would like to send out a reminder, say 7 days prior.
  3. I was thinking of adding shift() and dependency on https://github.com/briannesbitt/Carbon
    $cron = new Cron\CronExpression('@quarterly');
    echo $cron->shift('-7 days')->getNextRunDate(null, 2)->format('Y-m-d H:i:s');
  4. Would this be an acceptable PR?
bilogic commented 3 years ago

@dragonmantank would love to hear your opinions :)

bilogic commented 3 years ago

@DerekCresswell what do you think? Thanks.

DerekCresswell commented 3 years ago

Sounds like an interesting addition to me. However, I don't think such a thing would be accepted. The maintainer of the repository considers it feature complete. So such a change is unlikely to be added. I would just say extend the class yourself and add this functionality to it.

bilogic commented 3 years ago

@DerekCresswell

Thanks, I mistook you as the maintainer as only your name appeared when I looked through the recent commits.

bilogic commented 3 years ago

@dragonmantank possible to say a yes or no? (preferrably yes 🤣)

dragonmantank commented 2 years ago

Thanks for the suggestion.

As @DerekCresswell mentioned this library tries to stick to the original C implementation as much as possible, so I'm not sure I would add this. I will gladly leave open for others to see if they would like it though.

bilogic commented 2 years ago

Thanks @dragonmantank

How do we vote?

dragonmantank commented 2 years ago

I just leave it open for anyone else to chime in. I added the label to make it easier for people to find when looking through issues, because we have a few feature requests that are like this (might make life easier, but diverge from the original implementation).

nyamsprod commented 2 years ago

@bilogic correct me if I am wrong but can't you not rewrite the code this way ?

$cron = new Cron\CronExpression('@quarterly');
echo $cron->getNextRunDate(null, 2)->sub(DateInterval::createFromDateString('7 days'))->format('Y-m-d H:i:s');

Seems to me that nothing needs to be added to the library to achieve what you are looking for 🤔 Or did I miss something ?

bilogic commented 2 years ago

@nyamsprod

I can't remember my exact thought process, but I'm using this library via Laravel's schedule. I vaguely recall being able to expose an instance of this library, and thus wanted to be able to shift(...) it so that when Laravel calls it's getNextRunDate(), it gets the shifted date.

bilogic commented 2 years ago

@nyamsprod I revisited this, this repo's isDue() needs to return true for the shifted date. As far as I can tell, your code doesn't do that. Basically the shifting logic must be contained within getNextRunDate() otherwise lots of code needs to be modified.