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

Provide PSR-20 Clock implementation #443

Merged
merged 9 commits into from
Nov 9, 2023
Merged

Provide PSR-20 Clock implementation #443

merged 9 commits into from
Nov 9, 2023

Conversation

odan
Copy link
Contributor

@odan odan commented Oct 24, 2023

This PR provides a PSR-20 compliant Clock implementation for the Chronos package.

The PSR-20 Clock interface defines a standard way to interact with system time, allowing for better testability and flexibility.

@markstory markstory added this to the 3.x milestone Oct 24, 2023
@markstory markstory requested a review from othercorey October 24, 2023 14:02
@othercorey
Copy link
Member

I am away right now but I think the term ChronosClock is misleading and confusing compared to the other classes. Can we expose this some other way?

@odan
Copy link
Contributor Author

odan commented Oct 26, 2023

The idea was to name it ChronosClock because it returns a Chronos instance, that is also a TimeTimeImmutable instance.

Other class names: Clock, ClockFactory or DateTimeImmutableFactory.

@odan
Copy link
Contributor Author

odan commented Oct 26, 2023

After thinking about it, I guess my approach doesn't make sense from a Chronos perspective, because if you are already using Chronos, you may not need the ClockInterface at all.

@markstory
Copy link
Member

After thinking about it, I guess my approach doesn't make sense from a Chronos perspective, because if you are already using Chronos, you may not need the ClockInterface at all.

I think it is another way for us to participate in the community standards within the larger PHP ecosystem. That alone has value I think.

@othercorey
Copy link
Member

What if we implemented it with an anonymous class that's returned by a static helper. That would provide easy access to the interface, right?

@odan
Copy link
Contributor Author

odan commented Oct 30, 2023

Error: Type DateTimeImmutable is not used in this file.

phpcs complains about this problem even though Chronos is inherited from DateTimeImmutable.

What if we implemented it with an anonymous class that's returned by a static helper. That would provide easy access to the interface, right?

A static method cannot then be used like an interface, for example when using dependency injection.

@othercorey
Copy link
Member

ChronosFactory or ClockFactory should work then.

@markstory
Copy link
Member

There is one last lint error to fix and then we should be able to move forward with this.

@othercorey
Copy link
Member

The class needs to be renamed to ClockFactory.

* Redistributions of files must retain the above copyright notice.
*
* @copyright Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
* @copyright Copyright (c) Daniel Opitz
Copy link
Member

Choose a reason for hiding this comment

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

We don't need separate copyright, just the cakephp one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this, because it was also added here:

https://github.com/cakephp/chronos/blob/3.x/src/Chronos.php#L11
https://github.com/cakephp/chronos/blob/3.x/src/DifferenceFormatter.php#L11

Maybe we remove the other redundant copyright tags as well?

Copy link
Member

Choose a reason for hiding this comment

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

The very original Chronos was forked from a different project so Mark inherited a copyright for some of the files. It's very close to not applying now, but definitely not new code.

@othercorey othercorey merged commit da6b49c into cakephp:3.x Nov 9, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants