Repository navigation
Add ZonedClock, and remove now() methods and DefaultClock - #133
joelbutcher wants to merge 9 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
============================================
+ Coverage 99.24% 99.27% +0.03%
+ Complexity 1143 1131 -12
============================================
Files 48 48
Lines 2517 2488 -29
============================================
- Hits 2498 2470 -28
+ Misses 19 18 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thank you for your PR, @joelbutcher. Some early comments / questions:
@solodkiy What do you think of this PR as a whole, and do you have an opinion on these points? |
|
Thanks @BenMorel On your questions, here are my thoughts: ✅ PSR-20 is removed
$clock = ZonedClock::system(TimeZone $timezone);
// the same as doing:
$clock = new ZonedClock(new SystemClock, $timezone);
$clock = ZonedClock::fixed(Instant $instant, TimeZone $timezone);
// primarily for tests, basically doing this:
$clock = new ZonedClock(new FixedClock($instant), $timezone);Removing
Renaming to
If we go with these, |
|
Hi @joelbutcher, SystemClock I also think that Maybe we can provide an easy way to create a $clock = new SystemClock()->withTimeZone(TimeZone::fromSystem());Side note: interestingly, there is an RFC being discussed that adds a native Clock that returns an Instant with no timezone. If this RFC ever gets accepted, then we would be heading in the same direction as native classes. Removing Let's experiment with this.
Agreed.
Let's do this (can be its own PR, mergeable immediately).
Yes, if we're taking that direction, then let's remove all As far as I can see, only Renaming to
Let's go! Please remove the
If we're removing I would love to keep the isPast/isFuture functionality though; I believe it's a plus for readability to be able to do: $clock->isInFuture($dateTime);vs: $dateTime->isAfter($clock->getInstant());But that means that Unless you have better ideas ( DefaultClock
Let's remove it in this PR! |
|
Thanks @BenMorel, I've pushed all of this to the branch. SystemClock It still implements Interesting RFC, nice to see native PHP heading the same way. Removing All
Renames
One leftover:
I removed them from For a later PR, one way to get the readability back without touching the $timeline = new Timeline($clock);
$timeline->isInFuture($expiresAt); // Instant|ZonedDateTime
$timeline->isInPast($createdAt);Implementers of
One gap: wrapping a plain
I lean towards the second, since it keeps all three types readable, but it does mean single-zone and multi-zone apps use it slightly differently. What do you think? DefaultClock Removed in this PR. PHPUnit, PHPStan and ECS all pass, and I've updated the PR title and description to match. |
Yes please!
Interesting |
|
@joelbutcher Can you please rebase as well? |
| /** | ||
| * Returns a clock that reads the current time from this clock, in the given time zone. | ||
| */ | ||
| public function withTimeZone(TimeZone $timeZone): ZonedClock; |
There was a problem hiding this comment.
Should we rename this to inTimeZone()? Withers usually return the same type, whereas this one returns a different type.
There was a problem hiding this comment.
Agreed that with*() reads wrong here. I went with atTimeZone() rather than inTimeZone(), since that's what the library already uses for the same kind of conversion: Instant::atTimeZone() and LocalDateTime::atTimeZone() also turn one type into another one bound to a zone. Happy to switch to inTimeZone() if you prefer.
Clock now only provides the current Instant, like Java's InstantSource and Noda Time's IClock. The new ZonedClock wraps a Clock and a TimeZone, like Noda Time's ZonedClock and Java's Clock, and implements PSR-20. - Clock gains withTimeZone(TimeZone): ZonedClock - ZonedClock implements Psr\Clock\ClockInterface: now() returns a DateTimeImmutable in its time zone, nanoseconds rounded down - ZonedClock provides getCurrentZonedDateTime(), getCurrentLocalDateTime(), getCurrentLocalDate() and getCurrentLocalTime() - New runtime dependency: psr/clock ^1.0 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EUshm9CLUVJZrwuV4jA8q4
now(TimeZone $timeZone, ?Clock $clock = null) becomes now(ZonedClock $clock)
on DayOfWeek, LocalDate, LocalDateTime, LocalTime, MonthDay, Quarter, Year,
YearMonth, YearWeek and ZonedDateTime, as do LocalDateTime::isFuture() and
isPast().
The time zone stays required, as a ZonedClock cannot exist without one,
and the clock becomes explicit:
LocalDate::now($this->clock);
LocalDate::now($clock->withTimeZone($timeZone));
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EUshm9CLUVJZrwuV4jA8q4
PSR-20 support will be discussed separately, including whether it should be implemented directly on ZonedClock or through an adapter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiFf5jYdYJDg1jRMHBoFRY
- Rename Clock::getTime() to getInstant() - Remove the Current prefix from ZonedClock getters - Remove all now() methods: use a ZonedClock (or Clock::getInstant()) instead - Remove isPast() / isFuture() from Instant, LocalDateTime and ZonedDateTime - Remove DefaultClock; Stopwatch now requires a Clock Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…() to setInstant() atTimeZone() matches Instant::atTimeZone() and LocalDateTime::atTimeZone(), which also return a different type. Also update a ZonedClockTest expectation for the full seconds now emitted by ZonedDateTime::toISOString() (brick#131), and point the MonthDay::now() migration to LocalDate::getMonthDay() (brick#134). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5ee870f to
bd6f8a4
Compare
|
Done, and rebased on
PHPUnit (7641 tests), PHPStan and ECS all pass. |
|
I'm quite happy with the state of this PR now, and would appreciate feedback from @solodkiy, @gnutix, @zjbarg, @nkamuo, @brunocassol here. TL;DR:
$clock = new SystemClock();
$zonedClock = $clock->atTimeZone(TimeZone::utc());
$today = $zonedClock->getLocalDate();Biggest advantages I see:
Thank you 🙏 |
|
Claude tried the PR on our Symfony project (multi-tenant, one database and one time zone per tenant, a mix of modern DDD code and a legacy codebase), by upgrading to this branch and migrating everything until the whole test suite was green again. It works for us. The migration took about 300 What it did
Small frictions it noticed
Thanks for your work, it's a nice improvement! |
|
Thank you, @gnutix, that's a very useful feedback! |
|
Let me know if you want me to run some Claudes on the "small frictions" (just give me some direction what you'd like for these first). |
Parses an ISO 8601 date-time with an offset, such as 2026-10-10T08:00:00Z, which makes fixed instants in tests readable without going through ZonedDateTime::parse(...)->getInstant() or a raw epoch second. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQhVM8scWxQrMU3djngjpv
Creates a ZonedClock fixed at a given ZonedDateTime, in its time zone, so tests no longer need to name the time zone twice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQhVM8scWxQrMU3djngjpv
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQhVM8scWxQrMU3djngjpv
|
@gnutix I've addressed most of those frictions. RE: "Today in UTC": The code is actually $clock->withTimeZone(TimeZone::utc())->getCurrentLocalDate()The long form is only needed when the clock you have is set to a different time zone. An app that works in UTC can inject a UTC |
|
@BenMorel I've synced the branch with main, though CI is failing which seems to be related to the Ubuntu's image upgrade to 26.04, which moved legacy timezone names like |
ubuntu-latest now points to Ubuntu 26.04, which no longer installs tzdata-legacy. Without it, PHP doesn't know legacy time zone names such as CET and Asia/Rangoon, which the tests use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQhVM8scWxQrMU3djngjpv
|
Quick follow-up on my two comments above:
|
Supersedes #132, following the direction agreed in that thread and in the review below.
Summary
Clocknow only provides the currentInstant, like Java 17'sInstantSourceand Noda Time'sIClock. A newZonedClockwraps aClockand aTimeZone, like Noda Time'sZonedClock, and is the single way to get the current date or time:Dependencies now go one way: clocks depend on value objects, and value objects never depend on clocks.
Clock::getTime()is renamed togetInstant(), andClockgainsatTimeZone(TimeZone): ZonedClock. It's named likeInstant::atTimeZone()andLocalDateTime::atTimeZone(), which also return a different type.ZonedClockis afinal readonlyclass. It implementsClock, and has:getClock()andgetTimeZone()atTimeZone(), which keeps the same wrapped clockgetZonedDateTime(),getLocalDateTime(),getLocalDate()andgetLocalTime()now()methods are removed, fromDayOfWeek,Instant,LocalDate,LocalDateTime,LocalTime,MonthDay,Quarter,Year,YearMonth,YearWeekandZonedDateTime.isFuture()andisPast()are removed fromInstant,LocalDateTimeandZonedDateTime.DefaultClockis removed, andStopwatchnow requires aClock.SystemClockstays a plainClock, so the system time zone is never used implicitly.Why
ZonedClockcan't exist without one, so there's no implicit default: not UTC, and notphp.ini. This keeps the explicit time zones valued in LocalDate::utc() method #111 and LocalDate::now($timezone /*= system TimeZone*/) #113.ZonedClockconfigured once.Clockand bind it per call withatTimeZone().getTime()was ambiguous. It meantInstantonClockbutLocalTimeonZonedDateTime.getInstant()and theZonedClockgetters name their return types.Breaking changes
This is fine before 1.0. The CHANGELOG lists a migration for each method:
Clock::getTime()Clock::getInstant()LocalDate::now($timeZone, $clock)$clock->atTimeZone($timeZone)->getLocalDate()YearMonth::now(...),YearWeek::now(...),MonthDay::now(...),Quarter::now(...),DayOfWeek::now(...)$zonedClock->getLocalDate()->getYearMonth()etc.Year::now(...)Year::of($zonedClock->getLocalDate()->getYear())Instant::now($clock)$clock->getInstant()$instant->isFuture($clock)$instant->isAfter($clock->getInstant())DefaultClock::freeze($instant)new FixedClock($instant)DefaultClock::travelTo($instant)new OffsetClock($clock, Duration::between($clock->getInstant(), $instant))DefaultClock::travelBy($duration)new OffsetClock($clock, $duration)DefaultClock::scale($n)new ScaleClock($clock, $n)new Stopwatch()new Stopwatch($clock)$fixedClock->setTime($instant)$fixedClock->setInstant($instant)Custom
Clockimplementations must renamegetTime()togetInstant()and implementatTimeZone().Related
LocalDate::getMonthDay(), which makesMonthDayreachable as$zonedClock->getLocalDate()->getMonthDay().Follow-ups
Psr\Clock\ClockInterfaceonZonedClock, or provide an adapter (both directions).isPast()/isFuture()that doesn't add methods to theClockinterface (see the discussion below).TimeZone::fromSystem().Tests
ZonedClockTest, with expected values checked againstdate(1). It includes Tokyo and Los Angeles falling on different dates, and the Europe/London DST change on 2026-03-29.atTimeZone()tests for every clock.now(),isFuture(),isPast()andDefaultClockAPIs are removed.🤖 Generated with Claude Code