IBX-801: Fixed timezone for DateTime fields in content name pattern

[SerheyDolgushev]

Question	Answer
JIRA issue	IBX-801
Type	bug
Target eZ Platform version	v3.3.6
BC breaks	no
Doc needed	no

Steps to reproduce

1. Make sure PHP date.timezone is set to America/New_York (or any other but not UTC)
2. Create "Event" content type with the following field:
   • Name (Identifier: name, Type: ezstring)
   • Date (Identifier: date, Type: ezdatetime)
3. Make sure "Event" content type has <name> <date> "Content name pattern".
4. Create a new "Event" content type:
   • Name: Meeting
   • Date: August 31, 2021 14:00

Expected result
The new event content item should have "Meeting Tue 2021-31-08 14:00:00" name.

Actual result
The new event content item's name is "Meeting Tue 2021-31-08 18:00:00".

Checklist:

• Provided PR description.
• Tested the solution manually.
• Provided automated test coverage.
• Checked that target branch is set correctly (master for features, the oldest supported for bugs).
• Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
• Asked for a review (ping @ezsystems/php-dev-team).

[SerheyDolgushev]

@ezsystems/php-dev-team

[Steveb-p]

I was pretty sure this was already fixed in #181, #185 and #187.

Anyway, the root issue is that DateTime object recovered for Field does not respect the server settings. Overwriting this value here mutates the object, so other parts of the application that access this object might behave differently depending on the order of method calls.

EDIT: I think that

ezplatform-kernel/eZ/Publish/Core/FieldType/DateAndTime/Value.php

Line 69 in eed4480

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

should be changed instead.

[SerheyDolgushev]

@Steveb-p 51bed6f should resolve your concerns. Otherwise, do you have a better solution on your mind?

[Steveb-p]

As per edited part, I think that

ezplatform-kernel/eZ/Publish/Core/FieldType/DateAndTime/Value.php

Line 69 in eed4480

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

should be changed to respect server timezone setting.

@mateuszdebinski did we have any issues that prevented us from changing this particular line of code? I remember we were talking about replacing instances where timezone is passed via DateTime constructor.

[SerheyDolgushev]

@Steveb-p followed your advice. Also please keep in mind https://www.php.net/manual/en/class.datetime.php#99309. Seem like fromTimestamp matches the described case. As DateTimeZone from the constructor is ignored in this case. And separate setTimezone call is required.

[Steveb-p]

And separate setTimezone call is required.

It's not required if DateTime is initialized without parameters and then timestamp is changed using DateTime::setTimestamp().

[SerheyDolgushev]

And separate setTimezone call is required.

It's not required if DateTime is initialized without parameters and then timestamp is changed using DateTime::setTimestamp().

Sorry, I just want to make this statement more clear:

DateTimeZone was ignored in the following case:

new DateTime("@{$timestamp}", new DateTimeZone(date_default_timezone_get()));

[Steveb-p]

@SerheyDolgushev yes, although I personally prefer

    public static function fromTimestamp($timestamp)
    {
        try {
-            return new static(new DateTime("@{$timestamp}"));
+            $datetime = new DateTime();
+            $datetime->setTimestamp($timestamp);
+
+            return new static($datetime);
        } catch (Exception $e) {
            throw new InvalidArgumentValue('$timestamp', $timestamp, __CLASS__, $e);
        }
    }

solution as it doesn't involve manually creating DateTimeZone with date_default_timezone_get() function.

[SerheyDolgushev]

But DateTimeZone needs to be manually created anyway for the fromString method? Or isn't it?

[Steveb-p]

But DateTimeZone needs to be manually created anyway for the fromString method? Or isn't it?

If DateTime is constructed with a regular string ("yesterday", "2020-01-01"), then timezone is applied normally.
If DateTime is constructed using timestamp (using @ prefix), then PHP sets +0000 as timezone.

So no, only creating a DateTime using timestamp presents this behaviour.

This can be circumvented by setting timestamp using setTimestamp() method (i.e. post construction) or as you are presenting, by building the correct timezone manually.

As I said, I personally prefer the setTimestamp() approach, because you don't need to create DateTimeZone instance, nor you need to call date_default_timezone_get().

[SerheyDolgushev]

@Steveb-p done

[Steveb-p]

LGTM, but @mateuszdebinski should take a look, and it has to go through QA.

I'm pretty sure there was a reason why we didn't immediately fix those, but can't remember why right now.

[ViniTou]

Should we target master branch only with that? It seems that publish 7.5 or at least 1.3 platform is a valid one.

[SerheyDolgushev]

Should we target master branch only with that? It seems that publish 7.5 or at least 1.3 platform is a valid one.

Rebased against 1.3

[alongosz]

Should we target master branch only with that? It seems that publish 7.5 or at least 1.3 platform is a valid one.

@ViniTou is right. This fix belongs to ezpublish-kernel:7.5, therefore can't be applied here.
Moreover test coverage is required, as we discussed many many times.

[mateuszdebinski]

we talked about introducing for DateAndTime as well, but it wasn't done, we probably missed it.
Ez platform 2.5 works a bit differently so ezpublish-kernel doesn't need this patch - everything works fine there.

From what I remember, corrections had to be made in several tests - including those for SOLR.

QA should also verify the search by date because I know that there were problems with this in previous changes as well

[alongosz]

Ez platform 2.5 works a bit differently so ezpublish-kernel doesn't need this patch - everything works fine there.

@mateuszdebinski could you elaborate on that? How is it possible?

[mateuszdebinski]

Ez platform 2.5 works a bit differently so ezpublish-kernel doesn't need this patch - everything works fine there.

@mateuszdebinski could you elaborate on that? How is it possible?

@alongosz eZ Platform 2.5 does not create a draft and then edit it like Ibexa DXP 3, therefore all timestamp offsets are generated by js, which retrieves timezone information and modifies the timestamp for UTC accordingly, which is then sent to the server and saved, therefore this the problem does not exist in 2.5

[Steveb-p]

Ez platform 2.5 works a bit differently so ezpublish-kernel doesn't need this patch - everything works fine there.

@mateuszdebinski could you elaborate on that? How is it possible?

@alongosz eZ Platform 2.5 does not create a draft and then edit it like Ibexa DXP 3, therefore all timestamp offsets are generated by js, which retrieves timezone information and modifies the timestamp for UTC accordingly, which is then sent to the server and saved, therefore this the problem does not exist in 2.5

If frontend adds timezone offset into timestamp in 2.5, then this issue becomes even worse. If timestamps are stored in database as they're received (not as date strings) then this would lead to an issue with data becoming invalid when migrating between 2.5 -> 3.3, since now we would add offsets on the backend.

And then we would store the timestamps properly in 3.3, which would lead to timestamps saved in 2.5 to be impossible to distinguish between new ones and therefore impossible to properly migrate.

[alongosz]

Ez platform 2.5 works a bit differently so ezpublish-kernel doesn't need this patch - everything works fine there.

@mateuszdebinski could you elaborate on that? How is it possible?

@alongosz eZ Platform 2.5 does not create a draft and then edit it like Ibexa DXP 3, therefore all timestamp offsets are generated by js, which retrieves timezone information and modifies the timestamp for UTC accordingly, which is then sent to the server and saved, therefore this the problem does not exist in 2.5

@mateuszdebinski the fact that the issue is not visible does not mean it does not exist. From the API perspective the code works the same, regardless of UI. API is also part of 2.5. Does applying the fix for 2.5 require changes to UI layer?

[SerheyDolgushev]

@ezsystems/php-dev-team sorry, any update on this one?

[Steveb-p]

@SerheyDolgushev thanks for the reminder.

@mateuszdebinski I summon thee.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[mateuszdebinski]

@mateuszdebinski the fact that the issue is not visible does not mean it does not exist. From the API perspective the code works the same, regardless of UI. API is also part of 2.5. Does applying the fix for 2.5 require changes to UI layer?

@alongosz Not only does UI require changes if we want to implement changes to Date and DateAndTime fields in eZ Platform 2.5 because in DB we have a timestamp with offset. If we change only API and UI then show incorrect data in created contents.
First, we need to know are we want changes in 2.5 if yes then need to prepare a command to correct the timestamp in all created contents with contain these fields.

@Steveb-p we need to adapt the current tests to this change in this PR before sending to QA

[alongosz]

@mateuszdebinski the fact that the issue is not visible does not mean it does not exist. From the API perspective the code works the same, regardless of UI. API is also part of 2.5. Does applying the fix for 2.5 require changes to UI layer?

@alongosz Not only does UI require changes if we want to implement changes to Date and DateAndTime fields in eZ Platform 2.5 because in DB we have a timestamp with offset. If we change only API and UI then show incorrect data in created contents.
First, we need to know are we want changes in 2.5 if yes then need to prepare a command to correct the timestamp in all created contents with contain these fields.

@Steveb-p we need to adapt the current tests to this change in this PR before sending to QA

Timestamp value is stored as Unix Epoch Timestamp in ezcontentobject_attribute.data_int database field (as an integer). Timestamps do not contain timezone, or as one could say, they're always set as UTC+00. The way of storing this information has been like that since eZ Publish 3.x (I guess). How is it that it behaves differently in 2.5 and in 3.3? What am I missing?

[Steveb-p]

@mateuszdebinski the fact that the issue is not visible does not mean it does not exist. From the API perspective the code works the same, regardless of UI. API is also part of 2.5. Does applying the fix for 2.5 require changes to UI layer?

@alongosz Not only does UI require changes if we want to implement changes to Date and DateAndTime fields in eZ Platform 2.5 because in DB we have a timestamp with offset. If we change only API and UI then show incorrect data in created contents.
First, we need to know are we want changes in 2.5 if yes then need to prepare a command to correct the timestamp in all created contents with contain these fields.
@Steveb-p we need to adapt the current tests to this change in this PR before sending to QA

Timestamp value is stored as Unix Epoch Timestamp in ezcontentobject_attribute.data_int database field (as an integer). Timestamps do not contain timezone, or as one could say, they're always set as UTC+00. The way of storing this information has been like that since eZ Publish 3.x (I guess). How is it that it behaves differently in 2.5 and in 3.3? What am I missing?

@alongosz someone borked the code and the offset got added into 2.5 timestamp-based fields. This means that technically they are timestamps, but in reality, they are timestamps with an offset.

Which basically means that we / users would have to:

1. Remove the already stored offset from timestamps in database.
2. Apply patch / upgrade version.

Otherwise we will have no way of knowing which field contains a "corrupted" timestamp, and which are fine (coming from patched version).

EDIT: To rephrase, someone was facing an issue that DateTime created from a timestamp value formatted "incorrectly". Instead of fixing the root issue of DateTime receiving +0000 offset - not the default server timezone - someone chose to push the date by adding an offset to the timestamp, making the issue worse, but "invisible" at first glance.

EDIT2: Basically this:

all timestamp offsets are generated by js, which retrieves timezone information and modifies the timestamp for UTC accordingly, which is then sent to the server and saved

[alongosz]

@mateuszdebinski ok, based on internal sync with @Steveb-p I'll allow skipping 2.5 for now, given the full stack there works and it would affect more people, therefore more risky.
What we need here is:

1. Align tests
2. Provide 3.3 upgrade script because the issue of incorrectly stored timestamps as you described is valid for 3.3 instances as well. It should be SQL upgrade script, not a command.

[SerheyDolgushev]

@mateuszdebinski ok, based on internal sync with @Steveb-p I'll allow skipping 2.5 for now, given the full stack there works and it would affect more people, therefore more risky.
What we need here is:

1. Align tests
2. Provide 3.3 upgrade script because the issue of incorrectly stored timestamps as you described is valid for 3.3 instances as well. It should be SQL upgrade script, not a command.

I assume all of the required changes are going to be implemented by @ezsystems/php-dev-team.