symfony
symfony copied to clipboard
symfony
[Clock] ClockAwareTrait: Cannot modify readonly property $clock
Symfony version(s) affected
6.3.0
Description
When using the ClockAwareTrait and invoking $this->now() in the constructor of my class, it initializes the private readonly ClockInterface $clock before the setClock method is invoked.
When the setClock method is invoked, php crashes with the error:
Cannot modify readonly property $clock
It was a bit unintuitive as we had a class that wasn't registered as service in Symfony before, and the $this->now() worked fine in the constructor, but as soon as we registered the class as service, we got this strange error. Took a bit to figure out why.
How to reproduce
<?php
use Symfony\Component\Clock\ClockAwareTrait;
use Symfony\Component\Clock\MockClock;
class MyTest
{
use ClockAwareTrait;
public function __construct()
{
$this->now();
}
}
$class = new MyTest();
$class->setClock(new MockClock());
Possible Solution
The problem is probably caused by incorrectly using clock in the constructor, but it would be nice if the ClockAwareTrait could be more detailed in the problem. Possible solutions in ClockAwareTrait:
Solution 1:
#[Required]
public function setClock(ClockInterface $clock): void
{
if (isset($this->clock)) {
throw new \RuntimeException('Do not invoke `$this->now()` in the constructor of your service');
}
$this->clock = $clock;
}
Solution 2:
#[Required]
public function setClock(ClockInterface $clock): void
{
if (isset($this->clock)) {
return;
}
$this->clock = $clock;
}
Additional Context
No response
Hello @frankdekker,
I was able to reproduce the issue locally and can confirm that it exists.
I believe the trait should handle this scenario more gracefully. After comparing your proposed solutions, I feel that while the second one would prevent crashes due to the read-only property, it could still introduce hidden bugs. If the wrong clock is used, developers might not realize that their clock instance isn't being updated, essentially sweeping the issue under the rug.
On the other hand, the first solution—while potentially breaking code that misuses the now method in the constructor—would fail early, providing users with clearer feedback about the root cause. This approach makes debugging easier, prevents unintended behavior, and enhances the overall robustness of the code.
Maybe the option to resolve that will be to remove readonly from the property.
Now it's impossible to set the MockClock object in tests without replacing the constructor argument (as replace clock by ->setClock(new MockClock($now))).
- Why does your class access the clock in its constructor?
- You access the clock already before any calling code has the chance to call
setClock(). Why do you use the trait at all?
I think you should fix your code. Making the property readonly is on purpose as it prevents making the service mutable.
In that case, I think it should be possible to override the $now property in MockClock object. In integration tests, when we test a specific service and want to simulate a specific point in time, we can't override Clock and can't override the $now property (it's only possible to modify now, but it's always relative to the current now value).
we can't override Clock and can't override the $now property (it's only possible to modify now, but it's always relative to the current now value).
not sure what's the blocker here: modifying doesn't have to be relative if you pass an absolute $modifier, isn't it?