nextOpen() and nextClose() don't return the correct values when called simultaneously

[nfragnet]

Hello,

The following test does not pass :

public function testItReturnsNextOpenAndNextClose()
{
    $mondayAfternoon = new \DateTime('2018-10-15 15:00'); // monday 15:00

    $openingHours = OpeningHours::create(['monday' => ['14:00-18:00'], 'tuesday' => ['09:00-12:00']]);

    $this->assertEquals(new \DateTime('2018-10-16 09:00'), $openingHours->nextOpen($mondayAfternoon));
    $this->assertEquals(new \DateTime('2018-10-15 18:00'), $openingHours->nextClose($mondayAfternoon));
}

DateTime $mondayAfternoon is modified when passed to nextOpen() and nextClose() methods. After the first assertion, $mondayAfternoon value is the same as nextOpen() return.

The problem persists even when I replace $mondayAfternoon param by a clone.

$mondayAfternoon is not modified anymore, but nextClose() will return a result based on a previous nextXXX() call.

The following test

public function testItReturnsNextCloseTwiceInARow()
{
    $mondayAfternoon = new \DateTime('2018-10-15 15:00'); // monday 15:00

    $openingHours = OpeningHours::create(['monday' => ['14:00-18:00'], 'tuesday' => ['09:00-12:00']]);

    $this->assertEquals(new \DateTime('2018-10-15 18:00'), $openingHours->nextClose(clone $mondayAfternoon)); // OK
    $this->assertEquals(new \DateTime('2018-10-15 18:00'), $openingHours->nextClose(clone $mondayAfternoon)); // NOT OK
}

fails with the following message

Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
-2018-10-15T18:00:00.000000+0000
+2018-10-16T12:00:00.000000+0000

Finally I cannot pass a new DateTimeImmutable object to nextOpen() and nextClose() methods (even their signatures accept DateTimeInterface) because of the return type hint (DateTime) but it may be another ticket.

[kylekatarnls]

Hi, I would add the fact that the method modify the date is unexpected. It should be copied. Then this should be reassigned here: https://github.com/spatie/opening-hours/blob/master/src/OpeningHours.php#L181 to work with immutable date.

[kylekatarnls]

An other problem is $timeRange iteration is not reset between 2 calls:
https://github.com/spatie/opening-hours/blob/master/src/OpeningHoursForDay.php#L76

[kylekatarnls]

We work around this bug using:
https://github.com/kylekatarnls/business-time/blob/master/src/Cmixin/BusinessTime.php#L256
I hope it can help you @nfragnet

[nfragnet]

Thanks a lot for the tips @kylekatarnls

As I had to decorate the whole object anyway for business purposes, I used the decoration to temporary fix the different issues I encountered. As I'm only using few methods, this solution - even if not the prettiest thing I coded - makes the job

final class MyOpeningHours
{
    /**
     * @var OpeningHours
     */
    private $openingHours;

    /**
     * @var string
     */
    private $tz;

    public function __construct(array $data, string $tz = 'UTC')
    {
        $this->openingHours = OpeningHours::create($data);
        $this->openingHours->setTimezone($tz);

        $this->tz = $tz;
    }

    public function nextOpen(DateTimeInterface $dateTime) : DateTime
    {
        $clonedOpeningHours = OpeningHours::create($this->toArray());
        $clonedOpeningHours->setTimezone($this->tz);
        $clonedDateTime = new DateTime($dateTime->format(DateTime::ATOM));

        return $clonedOpeningHours->nextOpen($clonedDateTime);
    }

    /**
     * Return the array that can be used to rebuild another OpeningHours object
     *
     * @return array In a format acceptable by the __construct and factory method
     *
     * [
     *     'monday' => [...]
     *     'tuesday' => [...]
     *     ...
     * ]
     */
    public function toArray() : array { //... }
}

I may use your workaround if our needs evolve before the correction is merged :) thanks anyway