Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New PSR for ClockInterface #1224
New PSR for ClockInterface #1224
Changes from 50 commits
31f93b6
faa28e1
0065811
d6c75b2
9974dfb
0c66162
abb943b
0df6d63
072514e
2bc8e21
fed0313
9c3d13f
82f07eb
c8bcefe
5415f5f
3386929
4395fc3
d1bf6cf
506b1fc
380089c
5c17173
650f6d2
05f2fba
18585b5
8d3e388
0ad441a
078ec7b
2e13cb5
a79da23
0d7157c
4f13a8e
fa9323d
17f364a
8a701b2
509163c
8b34135
6bd1916
0338682
2734e16
c279903
6ada868
4b53d45
92c4244
3a8c4b1
87c4002
5e05b77
9f8115b
1ced885
12e1c1c
2de88c5
9401671
3688544
9ef42e5
8826633
8e9aace
66fac09
81cb449
3caffc3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Note: Symfony does not have a TimeMock library. Symfony provide a package named
symfony/phpunit-bridge
with aSymfony\Bridge\PhpUnit\ClockMock
class:For reference, see
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.
@cseufert
Can you adjust this, please?
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 have updated this to be more accurate
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.
Not sure, do we need a trailing
;
or.
here? If so, perhaps consistently use one?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.
Not sure, perhaps @ashnazg can weight in on this?
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 definitely feel very uncomfortable with this
TimeZoneAwareClock
as the first implementation shown as an example.It feels like the normal case to implement it would be to provide a timezone, while this is a very bad practice to promote. time-zone-based code leads to a code you can't scale easily worldwide, you can't easily move consistently to an other place, not easily re-use.
The
Non-Goals
says it let the timezones handling up to implementation, but this example is yet very opinionated spreading the idea that picking any timezone is OK.We should show first the
UTCClock
then simply show that timezone can be changed on the returned object:And we should encourage to use the first one to do any back-end stuff or calculation, while reserving the second kind only for the very output to be displayed to an end user.
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.
@kylekatarnls
Why not inject the timezone via constructor?
See https://github.com/ergebnis/clock/blob/2.2.0/src/SystemClock.php#L16-L37.
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 don't see the point this is exactly the same as the current proposal for
TimeZoneAwareClock
and showing this to people is the same as promoting "You can use any timezone for your clock". AND. THAT'S. NOT. GOOD.As the person who might have to call
->now()
on an object implementingClockInterface
I want to get something reliable, not something in unknown, dynamic or even just some hard coded city timezone name.If someone want a timezone they can do it outside of the clock using
->setTimezone()
and as this PSR says it's not aimed to say how to deal with timezone, then it should not show clocks that handle it internally.Sure users still will be able to do it. But we should encourage them to do it outside the clock. So you give the clock only the responsibility to tick the moment, then give to some other service the responsibility to tell you what numbers are given to this tick in a given 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.
You get something very reliable: A
DateTimeImmutable
Object that represents the current time!What you will never be able to tell is, whether the current time was observed in Cairo, Sydney, Lima or London. That is something that your own business logic needs to handle anyhow.
The whole
ClockInterface
will be completely timezone agnostic and we are currently in the process of removing that missleading example!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.
Throwing my opinion in here since we’re still going back and forth on timezones.
+1 to encouraging UTC and leaving timezone manipulation to app implementation. IMHO if someone needs this they can create their own interface, the PSR should enforce best practice whenever possible.
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.
This PSR is timezone-agnostic.
Therefore we should not encourage anything in regards to UTC or any other timezone.
BestPractices in timezone-handling is to use
DateTimeImmutable
which we are enforcing. The internals of thatDateTieImmutable
-Instance are not our business.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.
Perfect, thanks @heiglandreas
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.
Should this not be
return clone($this->now);
I know, it's a minor detail, but any call to
now()
should return a newDateTimeImmutable
Object, even in a frozen clock.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.
@heiglandreas
Why?
It's a value object, so why bother returning different instances with the same value?
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.
In unit test:
Now if you try to compare
$someObject
old state property values with new ones using===
, the test will assert the object is untouched. That may lead to wrong results.So I guess the
(clone) $this->now
can make it a bit safer.Mainly, the idea is to make the FrozenClock behave as closely as possible like the real one.
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.
That was my reasoning. Each call to
now()
should return a new ValueObject IMO (whether the content is the same is a different story)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.
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.
@cseufert
What’s the order here?
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.
totally random really, any suggestions?
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.
Alphabetically would be my first choice
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.
Alphabetical by last name. Move Luís Cobucci to the beginning and we're done ;-)
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.
Done