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

feat(appframework): ⌚ Make ITimeFactory extend \PSR\Clock\ClockInterface #35872

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Dec 22, 2022

Summary

TODO

Checklist

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Given how easy it is to get a mutable object form the immutable one I wouldn't include the helper but I'm okay if we do.

Nice addition to the PSR API family 👍

@ChristophWurst
Copy link
Member

Might or might not make sense to deprecate getTime right away.

@nickvergessen
Copy link
Member Author

Might or might not make sense to deprecate getTime right away.

Same for the getDateTime() I think

return \DateTime::createFromImmutable($this->now());
}

public function withTimeZone(\DateTimeZone $timezone): static {
Copy link
Member Author

Choose a reason for hiding this comment

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

: static is 8.0+ only, but currently linting is still run against 7.4 while we plan to drop it.
Could remove the typing for now, or we keep it and make sure the 7.4 lint job is removed already

@nickvergessen nickvergessen force-pushed the feature/noid/psr-clock-interface branch from c4e4d91 to 2b12b75 Compare January 2, 2023 10:48
@nickvergessen nickvergessen marked this pull request as ready for review January 2, 2023 11:25
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Jan 9, 2023
@ChristophWurst
Copy link
Member

I would love to see this documented at https://docs.nextcloud.com/server/latest/developer_manual/digging_deeper/psr.html :)

@nickvergessen
Copy link
Member Author

Will do, after it's merged

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

:shipit:

@nickvergessen nickvergessen force-pushed the feature/noid/psr-clock-interface branch from 2b12b75 to acf0412 Compare January 23, 2023 10:35
@nickvergessen
Copy link
Member Author

Rebased, now that 7.4 is dropped

@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@nickvergessen
Copy link
Member Author

Dang, this slipped through. To preserve stability in OCP, I'm moving it to 27

@nickvergessen
Copy link
Member Author

Docs in #37039 (comment) and nextcloud/documentation#9722

@nickvergessen nickvergessen removed the pending documentation This pull request needs an associated documentation update label Mar 6, 2023
* @since 8.0.0
* @since 26.0.0 Extends the \Psr\Clock\ClockInterface interface
Copy link
Member

@ChristophWurst ChristophWurst Oct 11, 2023

Choose a reason for hiding this comment

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

are you sure about that? ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

YOu mean extends vs implements?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure it's correct here, but yeah on the actual implementation the comment could be adjusted

Copy link
Member

Choose a reason for hiding this comment

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

The since 26. I just had Mail integration tests fail because I used now for 26. It's only there for 27+ :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yeah see comment above:

Dang, this slipped through. To preserve stability in OCP, I'm moving it to 27

Sorry about that :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix in #40865

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants