-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/clock interface implementation #1939
base: main
Are you sure you want to change the base?
Feature/clock interface implementation #1939
Conversation
@@ -357,7 +361,7 @@ public function addRecord(int|Level $level, string $message, array $context = [] | |||
$recordInitialized = \count($this->processors) === 0; | |||
|
|||
$record = new LogRecord( | |||
datetime: $datetime ?? new JsonSerializableDateTimeImmutable($this->microsecondTimestamps, $this->timezone), | |||
datetime: $datetime ?? ($this->clock instanceof ClockInterface ? $this->clock->now() : new JsonSerializableDateTimeImmutable($this->microsecondTimestamps, $this->timezone)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datetime: $datetime ?? ($this->clock instanceof ClockInterface ? $this->clock->now() : new JsonSerializableDateTimeImmutable($this->microsecondTimestamps, $this->timezone)), | |
datetime: $datetime ?? $this->clock?->now() ?? new JsonSerializableDateTimeImmutable($this->microsecondTimestamps, $this->timezone), |
* @phpstan-param array<(callable(LogRecord): LogRecord)|ProcessorInterface> $processors | ||
*/ | ||
public function __construct(string $name, array $handlers = [], array $processors = [], DateTimeZone|null $timezone = null) | ||
public function __construct(string $name, array $handlers = [], array $processors = [], DateTimeZone|null $timezone = null, ClockInterface|null $clock = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should accept any clock in here or not.. Because a non-monolog clock will output regular DateTime instances, that won't play well with json serialization, and this might even get autowired in some cases, so I find this quite worrying, unless we allow a standard clock to be decorated by the LoggerClock (e.g. with $loggerClock->setClock(ClockInterface $clock)
it could use a regular ClockInterface to get the time and copy it into a JsonSerializableDateTimeImmutable instance).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LogRecord does not guarantee a JsonSerializableDateTimeImmutable is used, and $dateTime
in addRecord
does not enforce it either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's not enforced in the LogRecord, but it is enforced in addRecord, so it is de-facto always using that datetime class
monolog/src/Monolog/Logger.php
Line 332 in 548eeb3
public function addRecord(int|Level $level, string $message, array $context = [], JsonSerializableDateTimeImmutable|null $datetime = null): bool |
After @stof review I did some changes on the PR.
This PR introduces a new feature to allow users to specify a custom time for log records in Monolog. The enhancement involves integrating a Clock service that implements the PSR-20 ClockInterface,