[Clock] Add ClockAwareTrait to help write time-sensitive classes #48362
Conversation
dc8a762 to
b1a36b0
Compare
|
I wouldn't use this trait myself (given the benefits over a regular constructor parameter is not that significant IMHO) and I think there is no need to rush, better discuss this for 6.3. |
b1a36b0 to
e2f7acb
Compare
e2f7acb to
af06c7d
Compare
|
Now targeting 6.3. About adding a constructor argument for the clock, I think many will find that overkill and will decide to just not make their clock injectable. That just happened to me. Relying on the ambient clock is too convenient to warrant the boilerplate of adding a clock everywhere. But making it settable through the trait does reduce this design overhead. It provides a DX closer to the current ambient one, without scarifying anything that DI provides (testability/etc). Note that I'm not saying that everybody should use the trait after this is possibly merged. I'm just proposing a way to reduce the friction of using a clock, thus improving the adoption of the concept. Another alternative to solve the DX concern would be to provide a global static clock. That's why e.g. Chronos are so convenient to use. But I'm not ready yet to give up to DI :) |
af06c7d to
db0cda9
Compare
|
As this was discussed in a private chat rather than GitHub, I'll list my feelings about adding a class like this, so everyone can take this in consideration for 6.3 :) Looking at it from impact on actual code, the approach can be compared as:
I fail to see the actual difference. In my opinion, if the DX on the left side is bad, so would the DX on the right side be. On a design point of view, the trait is contradicting a few things that I consider important. Important enough to at least raise them in this discussion: 😉
Note that these points are not only important from a design point of view, but also affect cognitive load of the class. Using constructor injection, it is immediately clear by reading a single file (a) who is responsible for providing current time and (b) how to change the clock implementation. From a Symfony maintainer perspective it is unclear to me what is special about the clock service compared to other services. The only class I recall that comes clo
8000
se to "facade methods" like used here is At last, there is nothing wrong with using |
|
Thanks for writing your thoughts.
Also, I think using |
db0cda9 to
8e2af95
Compare
|
I don't like setter injection because it allows to update the service instances after their initialization. This is too risked for side-effects, especially in tests if the DIC is reused between tests (for performances) and the clock is updated on some of them (with a different mocked value). From @wouterj's example, the clock can easily be optional as last argument of the constructor. public function __construct(
// ...
- private ClockInterface $clock
+ private readonly ClockInterface $clock = new NativeClock()
) {
} |
|
@GromNaN as the trait defines the property as readonly, you can only call the setter once (the second time will get an error from PHP) |
|
Oh, good point @stof, the problem I am raising may not exist. The service instance cannot be updated. |
|
This trait could be used for changes like #48098. Would we use it internally? |
|
It'd make sense to me yes. |
Add the trait and use
$this->now()inside the class, autowiring will do the magic 💪