IBX-964: Added offset to timestamp

[mateuszdebinski]

Question	Answer
Tickets	IBX-964
Bug fix?	yes
New feature?	no
BC breaks?	no
Tests pass?	yes
Doc needed?	/no
License	GPL-2.0

Checklist:

• Coding standards ($ composer fix-cs)
• Ready for Code Review

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[dew326]

@mateuszdebinski please merge up.

[DoNotToyWithMe]

Hi, the provided solution doesn't fix the issue.

My PHP timezone is America/Sao_Paulo

I created an article, selected September 23, 2021.

Published.

Article preview shows September 23, 2021.

Edited the Article again and now the selected date is September 22, 2021.

Could you please check the original ticket description where I pointed the changes in IBX-300 caused the issue? When we reverted the changes in:

vendor/ezsystems/ezplatform-admin-ui/src/bundle/Resources/public/js/scripts/fieldType/ezdate.js

vendor/ezsystems/ezplatform-admin-ui/src/bundle/Resources/views/themes/admin/ui/field_type/preview/content_fields.html.twig

vendor/ezsystems/ezplatform-kernel/eZ/Publish/Core/FieldType/Date/SearchField.php

vendor/ezsystems/ezplatform-kernel/eZ/Publish/Core/FieldType/Date/Value.php

It fixed completely.

[DoNotToyWithMe]

I created a new issue https://issues.ibexa.co/browse/IBX-1090

[mateuszdebinski]

@DoNotToyWithMe in user settings you must have the same time zone as in php.ini settings, otherwise, there will be discrepancies

[DoNotToyWithMe]

@mateuszbieniek originally the date attribute would be set to UTC always, this is why the original code in vendor/ezsystems/ezplatform-admin-ui/src/bundle/Resources/views/themes/admin/ui/field_type/preview/content_fields.html.twig

Converts the date in the preview template to UTC. Also the js code should do the same.

[DoNotToyWithMe]

Check the problematic commits:

08ea4f3#diff-03a88b29d0af2169f521b6db3be86937e4791cfa8d844d022acd2a3646927d96

ezsystems/ezplatform-kernel@da07b78#diff-4477c92ab138d7846a862f46780dc60c00c2cbb737b6fc3fe1b0b4479f836e05

In eZ/Publish/Core/FieldType/Date/Value.php for example:

return new static(new DateTime("@{$timestamp}"))

Was converting the value to UTC, that is expected. Then the preview template does the same:

{% set field_value = field.value.date|ez_full_date('UTC') %}

It should be UTC everywhere regardless of the user preferences.

This all started with those "fixes" in IBX-300, which end up breaking everything because it is not normalizing all users date presentation to UTC.

Note: This is specific to the "Date" type. The "Date and Time" works in a different way and might take into account the timezone.

[mateuszbieniek]

@mateuszbieniek originally the date attribute would be set to UTC always, this is why the original code in vendor/ezsystems/ezplatform-admin-ui/src/bundle/Resources/views/themes/admin/ui/field_type/preview/content_fields.html.twig

Converts the date in the preview template to UTC. Also the js code should do the same.

I think you pinged the wrong Mateusz 🙂

[DoNotToyWithMe]

Sorry

ping @mateuszdebinski

[Steveb-p]

@DoNotToyWithMe unfortunately the problem is more complex than just simply "converting the value to UTC".

The fact is that new DateTime('YYYY-MM-DD') and new DateTime("@{$timestamp}") generate objects with different timezones. This means that when calling DateTime::format you'll get two different outcomes, even if UTC timestamp is the same.

For this reason PHP code (mentioned by you: ezsystems/ezplatform-kernel@da07b78#diff-4477c92ab138d7846a862f46780dc60c00c2cbb737b6fc3fe1b0b4479f836e05) was adjusted to always generate objects with the same DateTimeZone. This is done so that every time a DateTime object is created, it's default time zone for presentation is the same (matching php.ini settings).

Unfortunate side effect of this is that we require an upgrade for users that are on 2.5 version, because at some point in time a frontend solution was introduced that added an offset to timestamp that was sent to the server. This means that values that were presented as timestamps were not, in fact, UTC timestamps - they were UTC timestamps + an offset.

Therefore, the reason why DateTime objects passed into twig templates needed timezone to be specified as UTC was exactly that they already contain (incorrectly) a time zone offset. This is clearly wrong and makes it impossible for sites that actually span multiple timezones (and allow users to switch to their own timezone, for example) to work correctly - they will not be able to properly adjust the displayed time. To do so, they would first need to extract the offset and then specify the request timezone.

Unfortunately this means that there might be places in frontend code that add the offset on their own still. I'm pretty sure we haven't located all of these.

A workaround for that is to switch the default timezone for the application (via date_default_timezone_set or in php.ini) to UTC. I'm sorry that there is no complete solution yet.

[DoNotToyWithMe]

The fact is that new DateTime('YYYY-MM-DD') and new DateTime("@{$timestamp}") generate objects with different timezones. This means that when calling DateTime::format you'll get two different outcomes, even if UTC timestamp is the same.

The idea is that the $timestamp variable passed to DateTime would be the Date unix timestamp set to be in UTC. DateTime created with that @ has the timezone set to UTC, so $date->format is always going to be the day in UTC. In the frontend code always take that into account, we all know DateTime('YYYY-MM-DD') will create the Date in the current timezone, so we either avoid that or adjust later. This was mostly a problem in the frontend where we had custom code, for example, search code for Content within certain Dates (where we would need to convert the range to UTC). In fact backend was Ok (as far as I tested previously).

The proposed workaround is worse than what we did in one of our projects where we reverted the changes in IBX-300.

[DoNotToyWithMe]

@Steveb-p To complement my answer, check the description of:

ezsystems/ezpublish-legacy@3ce2994

EZP-29279: Values of Date Field Type should be handled in UTC only (#…
…1401)

* EZP-29279: ezdate timestamps are now stored in UTC

* Added missing hours, minutes and seconds to Timestamp conversion and add more s p a c e s to the same class code

* Fixup after CR

This has been the standard behavior since legacy. I could check even earlier commits where the code tries to do that, and I think we had similar problems like IBX-300 in the past related to this field.

[Steveb-p]

@DoNotToyWithMe while I agree that the current state is unacceptable, I still stand with the notion that the solution presented in IBX-300 is the right one going forward.

tl;dr It's best to not concern ourselves with timezones at all if possible.

It goes against the previous solution precisely because it was incorrect and introduced the mess we are in right now.

If you look into Symfony code you'll see that when dealing with DateTime forms, they use current server timezone setting by default (even for timestamps!):

https://github.com/symfony/symfony/blob/f32af4683a04f440cda7ad8068d6cd05600d5e0e/src/Symfony/Component/Form/Extension/Core/DataTransformer/BaseDateTimeTransformer.php#L39-L40

https://github.com/symfony/symfony/blob/f32af4683a04f440cda7ad8068d6cd05600d5e0e/src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToTimestampTransformer.php#L67-L73

This makes all Date objects in PHP consistent (unless you actually do work with timezones). Twig templating code in this case may not care about timezones because every object it gets uses the same one.
We do not need to be timezone-aware in Twig precisely because it makes it harder to work with. In case of each Date object you need to control it's source and remember to use or not use a specific timezone declaration. That's error prone and part of the current problem.

Now, the root of the problem.

In some instances when creating or updating dates the fact that we were dealing with UTC-based DateTime object was neglected, which lead to date in question being offset by the difference between server default timezone and UTC. Solutions for this varied - some people adjusted the displayed timezone (that's the | date('UTC') approach) while others added timezone offset to the timestamp.

While the first option was inconsequential (just date discrepancy if there were other forgotten places), the second one made this a lot worse.

That's because now some timestamps in database have an offset (which they should never have!) while others don't.

That's why, unless your application actually deals with different timezones, DateTime objects should always use the same timezone. This makes it so you don't need to think about timezone conversion anywhere, and that's what IBX-300 aims for.