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 issue 1902 #1938

Closed

Conversation

omerimzali
Copy link
Contributor

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, enabling more flexible and testable timestamp management.

Clock Integration: Added a Clock class that provides timestamps using JsonSerializableDateTimeImmutable. This allows for consistent and customizable time handling.

Logger Modification: Updated the Logger class to accept an optional ClockInterface parameter. The addRecord method now uses this clock to fetch timestamps when specified.

@Seldaek #1902 I saw this issue and I'm sending this PR

@@ -164,21 +166,28 @@ class Logger implements LoggerInterface, ResettableInterface
*/
private bool $detectCycles = true;

/**
* @var ClockInterface|null
Copy link
Contributor

Choose a reason for hiding this comment

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

this @var brings no value as it only repeat the native property type.

use Throwable;
use Stringable;
use WeakMap;


Copy link
Contributor

Choose a reason for hiding this comment

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

should be removed. We don't need 2 empty lines.

use Psr\Clock\ClockInterface;
use Monolog\JsonSerializableDateTimeImmutable;

class Clock implements ClockInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using a less specific name than Clock to avoid having it appearing high in IDE completion in projects using Monolog (while Monolog is not meant to become a library providing a clock for generic purposes that would involve making it more customizable)

@@ -329,7 +338,7 @@ public function useLoggingLoopDetection(bool $detectCycles): self
*
* @phpstan-param value-of<Level::VALUES>|Level $level
*/
public function addRecord(int|Level $level, string $message, array $context = [], JsonSerializableDateTimeImmutable|null $datetime = null): bool
public function addRecord(int|Level $level, string $message, array $context = [], bool $useClock = false): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This signature change is a BC break, which is a no-go in a minor version (and a bad idea in a major version if we don't provide a migration path through deprecations first)

@@ -356,8 +365,9 @@ public function addRecord(int|Level $level, string $message, array $context = []
try {
$recordInitialized = \count($this->processors) === 0;

$datetime = $useClock ? $this->clock->now() : new JsonSerializableDateTimeImmutable($this->microsecondTimestamps, $this->timezone);
Copy link
Contributor

Choose a reason for hiding this comment

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

phpstan is right about the issue reported here: if the caller passes true for $useClock, this will break when the clock is null.

And this is also quite useless when using the PSR-3 interface as those would never pass true there.

@omerimzali
Copy link
Contributor Author

I'm closing the PR I'll create another one according to your review thank you @stof

@omerimzali omerimzali closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants