Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

omerimzali
Copy link
Contributor

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,

@@ -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)),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
Copy link
Owner

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).

Copy link
Contributor

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.

Copy link
Owner

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

public function addRecord(int|Level $level, string $message, array $context = [], JsonSerializableDateTimeImmutable|null $datetime = null): bool

@Seldaek Seldaek added the Feature label Feb 5, 2025
@Seldaek Seldaek added this to the 3.x milestone Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants