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

(fix) default timezone for export #2397

Merged

Conversation

AIlkiv
Copy link
Collaborator

@AIlkiv AIlkiv commented Nov 5, 2024

In the server/lib/base.php file of the Nextcloud server, UTC is always set as the default timezone:

https://github.com/nextcloud/server/blob/4a44d6a6774d79a8c91cab5f94acafa271b6d4d6/lib/base.php#L609

The main argument for this choice seems to be to simplify log reading, as discussed in this issue: nextcloud/server#3553

However, this hardcoding of UTC affects all instances where date_default_timezone_get is used, resulting in UTC even when it would be more intuitive to use the server's timezone or the one set in config.php under default_timezone.

This pull request corrects the timestamp values in export files, aligning them with the expected timezone settings. The fix is informed by the solutions implemented in other parts of the system where similar timezone issues are handled, as seen in references like:

https://github.com/search?q=org%3Anextcloud+%22%3EgetDefaultTimeZone%28%22&type=code

@Chartman123
Copy link
Collaborator

@AIlkiv We've discussed the usage of timezones quite a few times in the past and then agreed on using UTC for the exports as far as I remember...

@susnux any other thoughts on this one?

Either way, you'll have to fix the errors first... ;)

@Chartman123 Chartman123 added the 2. developing Work in progress label Nov 5, 2024
@susnux
Copy link
Collaborator

susnux commented Nov 5, 2024

We've discussed the usage of timezones quite a few times in the past and then agreed on using UTC for the exports as far as I remember...

I fully agree, exports are data and thus should be UTC for convenience, so you can do what ever you like afterwards with it while you are processing the data.
But in this case we are already formatting the date using the user timezone and fallback to server timezone (which is always UTC in this case), so I would say to correct this existing behavior we should fix it as suggested in this PR.

@Chartman123
Copy link
Collaborator

as suggested in this PR

ok, also the import of OCA\DAV? this will introduce another dependency on an app. Is there no OCP import available for that?

lib/Service/SubmissionService.php Outdated Show resolved Hide resolved
lib/Service/SubmissionService.php Show resolved Hide resolved
@AIlkiv AIlkiv force-pushed the default-timezone-for-export-date branch from 0582821 to 500d69e Compare November 5, 2024 18:06
@AIlkiv AIlkiv force-pushed the default-timezone-for-export-date branch from 500d69e to cb611e8 Compare November 5, 2024 18:19
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@1bd6ef3). Learn more about missing BASE report.
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2397   +/-   ##
=======================================
  Coverage        ?   42.86%           
  Complexity      ?      851           
=======================================
  Files           ?       72           
  Lines           ?     3203           
  Branches        ?        0           
=======================================
  Hits            ?     1373           
  Misses          ?     1830           
  Partials        ?        0           

@AIlkiv AIlkiv requested a review from susnux November 5, 2024 19:13
@susnux susnux added bug Something isn't working 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 5, 2024
@Chartman123
Copy link
Collaborator

/backport to stable4

@backportbot backportbot bot added the backport-request Pending backport by the backport-bot label Nov 5, 2024
@Chartman123 Chartman123 merged commit 39bd5df into nextcloud:main Nov 6, 2024
48 checks passed
@backportbot backportbot bot removed the backport-request Pending backport by the backport-bot label Nov 6, 2024
@Chartman123 Chartman123 added this to the 5.0 milestone Nov 6, 2024
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 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants