fisharebest / webtrees

Online genealogy
https://webtrees.net
GNU General Public License v3.0
463 stars 299 forks source link

Timestamp class needed local input and UTC output modes. #4931

Closed miqrogroove closed 6 months ago

miqrogroove commented 9 months ago

Fixes #4549

Clarifies the existing fromString() factory is only appropriate for UTC input.

Provides the fromLocalString() factory for user input, such as log filters.

Provides fromZoneString() factory for de-duplication and abstraction of the other two factories.

Provides Timestamp::toUTCDateTimeString() for SQL output, so timestamps won't be converted to local time and compared to stored UTC values.

Updates the PendingChangesService and SiteLogsService to use the correct times and timezones.

fisharebest commented 9 months ago

@miqrogroove - thanks for this. I think you have clearly identified the missing functionality.

But there's a few changes that could simplify it a bit.

We have two ways to display timestamps. format/isoFormat() and toDateTime(). The first is for display, and should always be converted to the user's timezone. The second is for creating database queries, and should always be in UTC.

Let me look at the code again, and I'll make some specific proposals.

miqrogroove commented 9 months ago

We have two ways to display timestamps. format/isoFormat() and toDateTime(). The first is for display, and should always be converted to the user's timezone. The second is for creating database queries, and should always be in UTC.

This would be an interesting design concept for toDateTimeString(), however that criteria is currently invalid. The only reason toDateTimeString outputs UTC dates is because the factory treats all inputs as UTC. If you don't want to use the Carbon timezone logic, then the inputs would need proper conversion prior to using the factory. So we need to decide if that's happening inside the factory (as in my patch) or not.

If you just want to update toDateTimeString() to always output UTC, then we can substitute the code of my new function toUTCDateTimeString() where the old one is.

Similarly, if you want to update toDateString() to always output the UTC date, we would need to rewrite that function.

fisharebest commented 7 months ago

I think the TimeStamp class is fairly broken, and it might be easier to start again with the next major release (2.2).

Fixing the reported issue using native PHP functions was pretty straightforward.

I want to consider replacing the dependency on Carbon with native DateTimeImmutable and IntlDateFormatter classes.

FrankWarius commented 7 months ago

when redesigning TImeStamp keep in mind the difference between SQL and TSQL see https://learn.microsoft.com/en-us/sql/t-sql/functions/date-and-time-data-types-and-functions-transact-sql?view=sql-server-ver16

TSQL DateTime has a (non changeable) precision of 3 digits / SQL 0 TSQL DateTime2 has a precision of 8 digits / SQL TimeStamp 6

see also https://github.com/fisharebest/webtrees/issues/4682

miqrogroove commented 7 months ago

Sounds reasonable. I would suggest avoiding usage of DateInterval because it's unnecessary here. The native modify() method is much more concise.

miqrogroove commented 6 months ago

Resolved by https://github.com/fisharebest/webtrees/commit/c9024634792322db5ca60d2b0a40ed045fe9aeb1