8000 [Clock] Add $modifier argument to the now() helper by nicolas-grekas · Pull Request #51365 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Clock] Add $modifier argument to the now() helper #51365

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 21, 2023
Merged

[Clock] Add $modifier argument to the now() helper #51365

merged 1 commit into from
Aug 21, 2023

Conversation

nicolas-grekas
Copy link
Member
@nicolas-grekas nicolas-grekas commented Aug 12, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

With this change, one can replace all new \DateTimeImmutable($foo) by now($foo).

@carsonbot carsonbot added this to the 6.4 milestone Aug 12, 2023
OskarStark
OskarStark approved these changes Aug 12, 2023
View reviewed changes
8000
Copy link
Contributor
@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Should we add a testcase?

@nicolas-grekas
Copy link
Member Author

Test case added, thanks for the review.

@xabbuh
Copy link
Member
xabbuh commented Aug 15, 2023

I am not convinced that a function named now() should return another date than the current date (according to the underlying clock). Shouldn't this rather be a different function?

@chalasr
Copy link
Member
chalasr commented Aug 15, 2023

I agree with @xabbuh

@wouterj
Copy link
Member
wouterj commented Aug 15, 2023

It reads nicely if we allow only relative modifiers: now('+10 minutes')
But I agree with @xabbuh and @chalasr for absolute modifiers now('2023-01-01') does feel wrong.

@chalasr
Copy link
Member
chalasr commented Aug 15, 2023 •
edited
Loading

now()->modify('+10minutes') reads even better imho. Let's just keep now() return now?

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Aug 16, 2023 •
edited
Loading

PR updated: it now leverages symfony/polyfill#440 so that we throw a DateMalformedStringException when the provided modifier is invalid. I also updated MockClock to throw this exception / DateInvalidTimeZoneException when appropriate.

now('+10 minutes')

Yes, this reads perfectly fine

now('2023-01-01') does feel wrong.

I don't see any issue with that personally. This is trivial to understand to me.
Introducing another function would feel worse, especially considering we cannot differentiate between relative and absolute modifiers.

now()->modify('+10minutes') reads even better imho.

It doesn't to me, especially considering the fact this misses proper error handling. Also considering this is supposed to replace new \DateTimeImmutable('+10minutes'): if the number of keystrokes is not clearly reduced, this fails the target to me.

Please reconsider :)

@OskarStark
Copy link
Contributor

I still like the modifier and the reading, while I understand the concerns, I would go with this PR 👍🏻

@stof
Copy link
Member
stof commented Aug 16, 2023

I'm wondering whether the change to MockClock should really be bundled in the same PR (and so in the same commit in the final history). This would hide entirely the behavior change in MockClock even for people reading the full git history and not just the changelog (which filters out minor changes).

@chalasr
Copy link
Member
chalasr commented Aug 17, 2023

I won't block but I think it's not worth it

@antonkomarev
Copy link

I think that explicitly calling a modify method is a much more obvious way to get a modified date from now.

@nicolas-grekas nicolas-grekas changed the title [Clock] Add $modifier argument to now() helper [Clock] Add $modifier argument to the now() helper Aug 17, 2023
@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Aug 17, 2023 •
edited
Loading

PR updated with error handling moved to #51412

explicitly calling a modify method

there are two major differences with now()->modify($foo):

GromNaN
GromNaN approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member
@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

This is a nice and concise syntax.
Developers can still call now()->modify('+10 minutes) it they prefer.

I like that we can pass the timezone as modifier.

@nicolas-grekas nicolas-grekas merged commit 3265ec2 into symfony:6.4 Aug 21, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Aug 21, 2023
Closed
@nicolas-grekas nicolas-grekas deleted the now-modify branch August 23, 2023 16:56
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh xabbuh left review comments

@chalasr chalasr chalasr left review comments

@GromNaN GromNaN GromNaN approved these changes

@OskarStark OskarStark OskarStark approved these changes

Assignees
No one assigned
Labels
Clock Feature Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
9 participants
@nicolas-grekas @xabbuh @chalasr @wouterj @OskarStark @stof @antonkomarev @GromNaN @carsonbot
0