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

Migrate away from Kernel::System::Time in OTOBO core #374

Closed
bschmalhofer opened this issue Aug 18, 2020 · 2 comments
Closed

Migrate away from Kernel::System::Time in OTOBO core #374

bschmalhofer opened this issue Aug 18, 2020 · 2 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@bschmalhofer
Copy link
Contributor

bschmalhofer commented Aug 18, 2020

The hard question is whether Kernel::System::Time is used in other packages. However it safe to migrate away from Kernel::System::Time in OTOBO core.

Originally posted by @bschmalhofer in #69 (comment)

@bschmalhofer bschmalhofer self-assigned this Aug 18, 2020
@bschmalhofer bschmalhofer added the enhancement New feature or request label Aug 18, 2020
@bschmalhofer bschmalhofer added this to the OTOBO 10.1 milestone Aug 18, 2020
@bschmalhofer
Copy link
Contributor Author

One thing needs to be considered. Kernel::System::Time objects can be cached by the ObjectManager. Kernel::System::DateTimeobjects are not cached. This could lead to different times, as construction without params uses the creation time implicitly.

bschmalhofer added a commit that referenced this issue Aug 18, 2020
Clear up confusion between days and seconds
bschmalhofer added a commit that referenced this issue Aug 18, 2020
As no time functions are called directly.
Only Kernel::System::DateTime is used, which relies on DateTime
bschmalhofer added a commit that referenced this issue Aug 18, 2020
and some beautifications
bschmalhofer added a commit that referenced this issue Aug 18, 2020
This is fine here. As the ObjectManager cache is cleared for each request.
bschmalhofer added a commit that referenced this issue Aug 19, 2020
…m::DateTime

Create the DateTime object when the time is actually needed,
as CurrentTimestamp() would have created a new DateTime object anyways.
bschmalhofer added a commit that referenced this issue Aug 19, 2020
This reverts commit 740018f.
The two calls were not equivalent. Reverting.
bschmalhofer added a commit that referenced this issue Aug 22, 2020
WorkingTime() can be considered a useful method.
bschmalhofer added a commit that referenced this issue Aug 22, 2020
Just because it can be considered as confusing.
bschmalhofer added a commit that referenced this issue Aug 22, 2020
…me()

Currently failing because UTC is assumed.
bschmalhofer added a commit that referenced this issue Aug 22, 2020
bschmalhofer added a commit that referenced this issue Aug 24, 2020
@bschmalhofer
Copy link
Contributor Author

Migration has been done for modules and test scripts. Tests look fine. Some findings are in separate issues: #388 and #389. Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant