Framework: Schedule `between` and `unlessBetween` are wrong if spanning midnight.

[aarondfrancis]

• Laravel Version: 5.5.43
• PHP Version: 7.3.3
• Database Driver & Version: NA

Description:

The docs show this as an example:

$schedule->command('reminders:send')
                    ->hourly()
                    ->unlessBetween('23:00', '4:00');

which eventually ends up in Carbons between method. That method switches the dates based on which one is greater. So instead of actually evaluating to between 23:00 and 4:00 it evaluates to between 4:00 and 23:00. Which is the exact opposite of what was requested.

I don't think this is a documentation bug as much as just regular bug. At the very least, a warning should be added to the docs.

Steps To Reproduce:

In Console/Kernel.php

protected function schedule(Schedule $schedule)
{
    $schedule->command('list')->between('23:59', '00:01');
}

From the command line (at any other time besides midnight): artisan schedule:run.

You'll see that it ran the list command. If you swap the arguments:

$schedule->command('list')->between('00:01', '23:59');

and run artisan schedule:run again, exact same thing because internally Carbon is swapping the arguments to be in early --> late order.

[devcircus]

IMO This would be expected. The idea here is "run the command daily between these two times. It doesn't really make sense to include multiple days in a range like that.
I think a note in the docs would be sufficient.

[aarondfrancis]

@devcircus wouldn't it make sense to set up a command that only ran in off hours? E.g.: run this intense command between 8pm and 4am local time. In which case your code would logically say ->between('20:00', '04:00').

That doesn't seem like a reach at all to end up having your commands run at the exact opposite time you were thinking they'd run.

[devcircus]

Definitely makes sense. I guess since I've never ran across a situation where it was needed, I've never thought about it. Doesn't seem like it would be too difficult.

[driesvints]

Huh, this is definitely a bug. I can even reproduce it in the current release. I was also taken aback by this. I'm wondering how come it's broken for so long.

[driesvints]

I kind of went deep down the rabbit hole on this and came out in the inTimeInterval method on the ManagesFrequencies trait which I modified like this:

    /**
     * Schedule the event to run between start and end time.
     *
     * @param  string  $startTime
     * @param  string  $endTime
     * @return \Closure
     */
    private function inTimeInterval($startTime, $endTime)
    {
        $start = Carbon::parse($startTime, $this->timezone);
        $end = Carbon::parse($endTime, $this->timezone);

        if ($start->isSameDay($end) && $end->lessThan($start)) {
            $start = $start->subDay(1);
        }

        return function () use ($start, $end) {dump('start', $start, 'end', $end);
            return Carbon::now($this->timezone)->between($start, $end);
        };
    }

Basically when the end time is earlier than the start time and both are on the same day I subtract the start time by one day to do the comparison.

But when I run the test below (in the ConsoleScheduledEventTest class) it gives incorrect results:

    public function testTimeBetweenChecksOffHours()
    {
        $app = m::mock(Application::class.'[isDownForMaintenance,environment]');
        $app->shouldReceive('isDownForMaintenance')->andReturn(false);
        $app->shouldReceive('environment')->andReturn('production');
        Carbon::setTestNow(Carbon::now()->startOfDay()->addHours(2));

        $event = new Event(m::mock(EventMutex::class), 'php foo');
        $event->timezone('UTC');dump('now', Carbon::now());
        $this->assertFalse($event->between('8:00', '10:00')->filtersPass($app));
        $this->assertTrue($event->between('23:00', '3:00')->filtersPass($app));

        $this->assertTrue($event->unlessBetween('8:00', '10:00')->filtersPass($app));
        $this->assertFalse($event->unlessBetween('23:00', '3:00')->filtersPass($app));
    }

."now"
Illuminate\Support\Carbon @1561428000 {#800
  date: 2019-06-25 02:00:00.0 UTC (+00:00)
}
"start"
Illuminate\Support\Carbon @1561449600 {#800
  date: 2019-06-25 08:00:00.0 UTC (+00:00)
}
"end"
Illuminate\Support\Carbon @1561456800 {#802
  date: 2019-06-25 10:00:00.0 UTC (+00:00)
}
"start"
Illuminate\Support\Carbon @1561449600 {#800
  date: 2019-06-25 08:00:00.0 UTC (+00:00)
}
"end"
Illuminate\Support\Carbon @1561456800 {#802
  date: 2019-06-25 10:00:00.0 UTC (+00:00)
}
F                                                                4 / 4 (100%)

Time: 99 ms, Memory: 10.00 MB

There was 1 failure:

1) Illuminate\Tests\Console\ConsoleScheduledEventTest::testTimeBetweenChecksOffHours
Failed asserting that false is true.

/Users/driesvints/Sites/laravel/framework/tests/Console/ConsoleScheduledEventTest.php:123

FAILURES!
Tests: 4, Assertions: 23, Failures: 1.

As you can see the second assertion uses the same timestamps from the first assertion. Why this is is completely beyond me. There seems to be some referencing going wrong somewhere.

I'm going to have to leave it at this for now because I can't spend more time at it atm but any help is greatly appreciated.

[mfn]

I could swear months ago there was a PR dealing with a similar thing but I couldn't find it.

[aarondfrancis]

@driesvints thanks for looking into this so carefully, I definitely thought it was a bug and totally got bit by it in production :(

I wonder if the reference bug above could be solved by passing the strings into the closure instead of parsing them outside and then passing in. See below.

    /**
     * Schedule the event to run between start and end time.
     *
     * @param  string $startTime
     * @param  string $endTime
     * @return \Closure
     */
    private function inTimeInterval($startTime, $endTime)
    {
        return function () use ($startTime, $endTime) {
            $start = Carbon::parse($startTime, $this->timezone);
            $end = Carbon::parse($endTime, $this->timezone);

            dump('start', $start, 'end', $end);

            if ($start->isSameDay($end) && $end->lessThan($start)) {
                $start = $start->subDay(1);
            }

            return Carbon::now($this->timezone)->between($start, $end);
        };
    }

[aarondfrancis]

👏 @laurencei nice work, thank you so much! I can't test this in my app because I'm still on 5.5 at the moment, but I really appreciate you getting this sorted.

[laurencei]

@aarondfrancis - you should be able to apply this same fix manually to your copy - the code is unchanged. Does that solve your issue? (This PR will never be backported to 5.5 since that does not get bug fixes anymore).

[driesvints]

@laurencei 5.5 is LTS and gets bugfixes until august 30 this year: https://laravel.com/docs/5.8/releases#support-policy

[laurencei]

@driesvints - yep, my bad.

So do I switch the PR to the 5.5 branch and to master?

[driesvints]

@laurencei it's ok. We can backport if it gets accepted. In any case I still believe it should target master and that would mean 5.5 wouldn't get the bug fix.

[aarondfrancis]

@laurencei I actually just "fixed" it for now by changing my userland code from between to unlessBetween and not spanning midnight. I had to do something quick to stop my commands from executing at the wrong times :/

[driesvints]

Ow, this was merged btw and is fixed in 6.0