-
Notifications
You must be signed in to change notification settings - Fork 11k
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 endOfMonth() to Console\Scheduling\ManagesFrequencies. #19870
[5.4] Add endOfMonth() to Console\Scheduling\ManagesFrequencies. #19870
Conversation
return function () { | ||
$now = Carbon::now(); | ||
|
||
return $now->day() == $now->daysInMonth(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you test it? day
method sets day for the current Carbon not returns day number. daysInMonth
doesn't even exist.
You need return $now->day == $now->daysInMonth;
See briannesbitt/Carbon#973
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh! You're right!! That's what I get for racing to do it over breakfast... 😊 Correction incoming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected.
I do that every single time with Carbon. Gah. Hats the method/property name overlap, we do.
return function () { | ||
$now = Carbon::now(); | ||
|
||
return $now->day == $now->daysInMonth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are comparing 2 integers, I believe we can strictly compare with ===
One thing I'm not sure about with this one is wouldn't it be expected to run a "end of month" report at midnight on the first day of the next month? Is that what this does? If not, and you are running an end of month report, it seems like you would miss the entire last day of the month. |
* @param string $time | ||
* @return $this | ||
*/ | ||
public function endOfMonth($time = '0:0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change to '23:59'
as a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to month a monthly report it seems to me you would just run monthly()
on the scheduler, which will run at midnight of first day of month.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe someone has task for current month values. So it fails on the next month.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it may be a method without a justifiable use case. I wasn't thinking so much reports for this one as notifications... end of the month is a common deadline. Of course then you'd want x days prior... Hmmm.
@taylorotwell While I have your attention, what are your thoughts about an |
I'll just let |
Gonna make one more pitch... ;-) Carbon offers:
So it would appear that there was sufficient use case for Carbon. ;-) Of course, the argument could be made to... just use Carbon. I'm thinking that exchanging the |
Yitzchok, with #19771 in |
The title says it all. The last day of the month is a common use case, this provides a convenience method for that use case akin to
weekends()
being a convenience method for its use case. Implementation is consistent with the similar combination ofbetween()
andinTimeInterval()
.