allow construct FrozenClock without argument

[slepic]

Hi,

I have a real world project where I have implemented basically the same as your package does (except i call Clock ClockInterface, and FronzenClock as ConstantClock instead :)). I have randomly encountered this package and I think that I would replace my code with this package instead of keeping it as part of an unrelated private repository that just wants to use the functionality.

However I have one use case, which is not unsolvable in your implementation, but it would be simpler to allow create frozen clock without arguments, basically to keep the time the same for all consumers thorughout a longer process.

[lcobucci]

Just a tiny thing 🙂
Thanks for helping out with this lib!

[slepic]

@lcobucci hey there

I see why you need this but what do you think about introducing something like SystemClock#freeze(), which would return an instance of a FrozenClock (passing the current time as construction argument).

I was having the very same dilema :/

I was considering several variations, let me show them and make some pros and cons (considering the structure, method names can be different).

Current state of things

new FrozenClock(new DateTimeImmutable());
new FrozenClock(new DateTimeImmutable('now', $tz));

Pros:

• does not require knowledge of SystemClock

Cons:

• verbose

Default constructible FrozenClock

$clock = new FrozenClock();
$clock = new FrozenClock(new DateTimeImmutable('now', $tz));

Pros:

• does not require knowledge of SystemClock

Cons:

• verbose when using custom timezone

Default constructible with timezone

$clock = new FrozenClock();
$clock = FrozenClock::withTimezone($tz);

Pros:

• does not require knowledge of SystemClock

SystemClock->freeze

$clock = (new SystemClock())->freeze();
$clock = (new SystemClock($tz))->freeze();

Cons:

• verbose
• violates SRP
• requires knowledge of SystemClock
• need create object (SystemClock) and discard it right away

SystemClock::freeze

$clock = SystemClock::freeze();
$clock = SystemClock::freeze($tz);

Cons:

• violates SRP
• requires knowledge of SystemClock

FrozenClock::fromSystemTime

$clock = FrozenClock::fromSystemTime();
$clock = FrozenClock::fromSystemTime($tz);

Pros:

• does not require knowledge of SystemClock

FrozenClock::fromClock

$clock = FrozenClock::fromClock(new SystemClock());
$clock = FrozenClock::fromClock(new SystemClock($tz));

Pros:

• works with any Clock implementation

Cons:

• verbose
• requires knowledge of SystemClock
• need create object (SystemClock) and discard it right away

global function (alternative to class extensions like in C#)

$clock = freeze(new SystemClock());
$clock = freeze(new SystemClock($tz));

Pros:

• works with any Clock implementation

Cons:

• verbose
• not OOP
• requires knowledge of SystemClock
• need create object (SystemClock) and discard it right away

---

I think I do prefer my previous suggestion, since it avoids code duplication.

Using the same lower level API (DateTimeImmutable constructor) in two implementations of the same interface is not a code duplication, imho, and so i didn't put this into the "cons" sections. Where I consider code repetition, it is how much the caller must repeat, not the implementation.

So to summarize, I like the "default constructible with timezone" variant the most (basically what I implemented in this PR, except I didn't care about the custom timezone variation).

I am willing to redo this to any variant you choose.

But, honestly If any of the variants below "default constructible" is chosen, I will probably use the "current state of things" variant in my application. And so at least for me, the feature will be useless.

Weeell, reconsidering, maybe the fromSystemTime() variant might be ok too...

[slepic]

@lcobucci Any update on this? I think the best we can agree on is FrozenClock::fromNow(): self {return new self(new DateTimeImmutable());}. Shall I go ahead and implement it?

[lcobucci]

@lcobucci Any update on this? I think the best we can agree on is FrozenClock::fromNow(): self {return new self(new DateTimeImmutable());}. Shall I go ahead and implement it?

Sorry about my delay. Honestly I'm not convinced that it's the best design.

It's surely the most practical to meet both our needs. However, I disagree with SRP violation you brought since the client using the API might be interested in both getting the current time and freezing things.

When I use this library I always have an instance of the system clock hooked on the DI container, so calling SystemClock#freeze() on a middleware to pass things down sounds like a reasonable solution.

Could you share how you configure things so that we can have a more informed decision?

[slepic]

However, I disagree with SRP violation you brought since the client using the API might be interested in both getting the current time and freezing things.

If I wanna freeze at current system time I am indeed interested in both those things. The system time is provided by the DateTimeImmutable API, not by SystemClock. Responsibility of SystemClock is to fulfil the Clock interface which is to provide current time in an abstract way, in this case by propagating the actual system time.

If SystemClock->freeze() exists, SystemClock is then responsible for providing current time (which is responsibility of any Clock), and now also responsible for creating another Clock implementation.

There is probably no consumer that will want to use both those methods (now() and freeze()).
Also there is no interface I can typehint to in places which wanna call the freeze() method.

On other hand, if FrozenClock::fromNow() exists, it is only an extension to implicit responsibility of every class - which is ability to create its instances.
In this case both SystemClock and FrozenClock only depend on DateTimeImmutable API (which they already do) and they only fullfill their responsibilty to implement the Clock interface.

When I use this library I always have an instance of the system clock hooked on the DI container.

Well in my use case, I don't need SystemClock instance, actually I don't even have a DI container, because I only create a single end-user object. There is just a bunch of constructors called and the objects composed together to create the result, which is a symfony console application.

It all goes like this:

final class Kernel
{
  public function __construct(
    private string $optionA,
    private int $optionB
  ) {}

  public static function fromEnvironment(): self
  {
    return new self(
      $_ENV['APP_A'] ?? 'default',
      (int) ($_ENV('APP_B'] ?? 0)
    );
  }

  public function createConsoleApplication(): \Symfony\Component\Console\Application
  {
    $frozenClock = new FrozenClock(new \DateTimeImmutable());
    $application = new Application();
    $application->addCommands([
      new CommandA('daily', new ServiceA($frozenClock, ...), ...),
      new CommandB('weekly', new ServiceB($frozenClock, ...), ...),
    ]);
    return $application;
  }
}

Kernel::fromEnvironment()
    ->createConsoleApplication()
    ->run();

[lcobucci]

@slepic thanks for sharing your setup (and also for the discussion).

Let's go with FrozenClock::fromNow(?DateTimeZone $timezone = null) or FrozenClock::fromSystemTime(?DateTimeZone $timezone = null). It keeps everything contained and suits your needs.

Would you mind handling the changes?

[slepic]

@lcobucci Yeah, definitely, I may have some time on the weekend to do it.

Btw please choose the name for the method. I find myself usualy struggling with naming things :D I would prefer one over the other and later on realize I shouldn't have :D

Personally I think i prefer fromSystemTime, even tho it is longer. I'm even thinking about just fromSystem. But I will leave this decision to you :)

There is one more thing I am worried about though.

Do you remember #32 ?
I am now wondering whether we shouldn't have done it the other way around.
Now the test uses data_default_timezone_get the same as the SystemClock itself.
But that means that the system clock produces instances with different timezone, based on some global state (during the SystemClock construction).
Shouldn't this be done the other way around? That default SystemClock would always be created in UTC and if you want a different timezone, set it explicitly? So the test would use hardcoded "UTC" string (as it used to), so would the SystemClock default constructor and so would the FrozenClock::fromSystemTime? (btw "UTC" could maybe a constant on the Clock interface? It would be best if const \DateTimeZone::UTC was a thing, and guess what? it is, but is is integer. how disappointing... but i think such constant makes sense on the Clock interface)

Of course, I can just implement this feature in the same way that SystemClock behaves and if the timezone logic changes, both will have to be fixed... (starting to realize why you said you didn't like nullable parameters, I actually don't too, but I just didn't see any problem with native php classes which themselves have default constructor. now i realize they use global state and I dont like it....)

[lcobucci]

@lcobucci Yeah, definitely, I may have some time on the weekend to do it.

Thanks!

There is one more thing I am worried about though.

Do you remember #32 ?
I am now wondering whether we shouldn't have done it the other way around.
Now the test uses data_default_timezone_get the same as the SystemClock itself.
But that means that the system clock produces instances with different timezone, based on some global state (during the SystemClock construction).
Shouldn't this be done the other way around? That default SystemClock would always be created in UTC and if you want a different timezone, set it explicitly? So the test would use hardcoded "UTC" string (as it used to), so would the SystemClock default constructor and so would the FrozenClock::fromSystemTime? (btw "UTC" could maybe a constant on the Clock interface? It would be best if const \DateTimeZone::UTC was a thing, and guess what? it is, but is is integer. how disappointing... but i think such constant makes sense on the Clock interface)

Of course, I can just implement this feature in the same way that SystemClock behaves and if the timezone logic changes, both will have to be fixed... (starting to realize why you said you didn't like nullable parameters, I actually don't too, but I just didn't see any problem with native php classes which themselves have default constructor. now i realize they use global state and I dont like it....)

I had the same thought these days... Using system timezone is kind of the "PHP way" (and honestly I just hope folks use UTC).

Changing our default is a BC-break, which I'm completely fine doing on the next major version. However we may introduce some named constructors to eventually remove the global state dependency.

Btw please choose the name for the method. I find myself usualy struggling with naming things :D I would prefer one over the other and later on realize I shouldn't have :D

Personally I think i prefer fromSystemTime, even tho it is longer. I'm even thinking about just fromSystem. But I will leave this decision to you :)

What do you about using fromUTCTime()? It's more explicit, removes the need for global state stuff, and kicks off the named constructors wagon...

[slepic]

What do you about using fromUTCTime()? It's more explicit, removes the need for global state stuff, and kicks off the named constructors wagon...

I'm all in about being explicit. But I have little objection, since UTC=Universal Time Coordinated, it seems weird to repeat the "time" word.

What do you think about eventually ending up at this:

interface Clock
{
  public function now(): \DateTimeImmutable;
}
final class SystemClock implements Clock
{
  public function __construct(\DateTimeZone $timeZone);
  public static function UTC(): SystemClock;
}
final class FrozenClock implements Clock
{
  public function__construct(\DateTimeImmutable $dateTime);
  public static function fromUTC(): FrozenClock;
  public function setTo(\DateTimeImmutable $dateTime): void;
}

I wouldn't mind doing it right away...

[lcobucci]

What do you about using fromUTCTime()? It's more explicit, removes the need for global state stuff, and kicks off the named constructors wagon...

I'm all in about being explicit. But I have little objection, since UTC=Universal Time Coordinated, it seems weird to repeat the "time" word.

Silly me 🤦
FrozenClock::fromUTC() is perfect IMHO.

What do you think about eventually ending up at this:

interface Clock
{
  public function now(): \DateTimeImmutable;
}
final class SystemClock implements Clock
{
  public function __construct(\DateTimeZone $timeZone);
  public static function UTC(): SystemClock;
}
final class FrozenClock implements Clock
{
  public function__construct(\DateTimeImmutable $dateTime);
  public static function fromUTC(): FrozenClock;
  public function setTo(\DateTimeImmutable $dateTime): void;
}

I wouldn't mind doing it right away...

I'd like to keep the scope of this PR only about the FrozenClock but, yes, that's the direction we should aim to (though changing SystemClock#__construct() to disallow null is a BC-break).

We can have another PR to add SystemClock::UTC() (or maybe SystemClock::forUTC()?) for the v1.4.0 and then break BC on v2.0.0.

[slepic]

Sounds good to me.

[localheinz]

@lcobucci @slepic

As some food for thought, here's what @OskarStark and I do in a project we work on:

SystemClock

Inspired by this pull request, I implemented freezing a SystemClock in ergebnis/clock#143.

Helper

We have a Helper trait for sharing functionality across tests:

<?php

declare(strict_types=1);

namespace App\Tests\Util;

use Ergebnis\Clock;
use Faker\Factory;
use Faker\Generator;

trait Helper
{
    final protected static function faker(string $locale = 'de_DE'): Generator
    {
        static $fakers = [];

        if (!\array_key_exists($locale, $fakers)) {
            $faker = Factory::create($locale);

            $faker->seed(9001);

            $faker->addProvider(new Faker\Provider\Number($faker));

            $fakers[$locale] = $faker;
        }

        return $fakers[$locale];
    }
    
    final protected static function frozenClock(): Clock\Clock
    {
        $now = \DateTimeImmutable::createFromMutable(self::faker()->dateTime);

        return new Clock\FrozenClock($now);
    }

    final protected static function systemClock(): Clock\Clock
    {
        return new Clock\SystemClock(new \DateTimeZone('Europe/Berlin'));
    }
}

Tests

In tests that use the Helper trait, we can now create and use clocks as we like:

<?php

$systemClock = self::systemClock();

// frozen to arbitrary time
$frozenClock = self::frozenClock();

// frozen to current time
$anotherFrozenClock = self::systemClock()->freeze();

[slepic]

@localheinz oh why is that a separate repo?

[localheinz]

@slepic

I once needed lcobucci/clock with support for PHP 7.0, which it didn’t have anymore at the time, and reimplemented it (slightly different).

I have dropped support for PHP 7.0 since.

Don’t get me wrong, I am not advertising here - only suggesting a slightly different approach that might be as useful.

[slepic]

I think we can close this one too, since #35 resolved the problem... thanks for coop