[13.x] Make between()/unlessBetween() independent of timezone() call order#60518
Open
ManicardiFrancesco wants to merge 2 commits into
Open
[13.x] Make between()/unlessBetween() independent of timezone() call order#60518ManicardiFrancesco wants to merge 2 commits into
ManicardiFrancesco wants to merge 2 commits into
Conversation
The between/unlessBetween schedule filters evaluated the timezone eagerly when the method was called, so chaining timezone() after between() silently used the wrong timezone. Defer the time interval computation into the filter closure so $this->timezone is read at run time, making the chain order commutative (consistent with other frequency methods). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
|
Agent review: Should fix — src/Illuminate/Console/Scheduling/ManagesFrequencies.php:60 recomputing Carbon::now() on every filter invocation changes behavior for repeatable sub-minute events because filtersPass() is called throughout the minute, so windows like everySecond()->between('10:00', '10:00') can pass only at the first second instead of remaining consistent for the schedule:run. |
Deferring the time-interval computation into the filter closure also made Carbon::now() recompute on every filtersPass() call. ScheduleRunCommand calls filtersPass() in a loop throughout the minute for repeatable sub-minute events, so the window result could flip mid-run. Capture the comparison instant once (when the filter is defined, i.e. once per schedule:run) while still reading $this->timezone lazily inside the closure, restoring the pre-existing frozen-instant behavior and keeping the call-order independence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Fixed ^ : we capture the comparison instant once when the filter is defined (i.e. once per schedule:run), while still reading $this->timezone lazily inside the closure. This restores the original frozen-instant behavior for sub-minute events and keeps the call-order independence (timezone() after between()) that this PR introduced. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
between()/unlessBetween()evaluate the timezone eagerly, the moment the method is called. So the result silently depends on wheretimezone()sits in the chain:Why this is a bug, not user error
between()/unlessBetween()are the only frequency methods whose outcome changes with call order. Every other one is order-independent: testBasicCronCompilation already asserts "chained rules should be commutative". This PR brings these two in line with that existing guarantee.The fix
Defer the time-interval computation into the filter closure, so
$this->timezoneis read when the filter runs rather than when it's defined. The two forms above become equivalent.schedule:run, so movingCarbon::now()into the closure doesn't change timing.Regression test (
testTimeBetweenChecksTimezoneCallOrder) covers bothbetween()andunlessBetween()in both chaining orders.