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

[7.0] EZP-28269: Fixed inconsistent creating of eztime fromTimestamp #2159

Merged
merged 4 commits into from Nov 23, 2017

Conversation

5 participants
@alongosz
Member

alongosz commented Nov 21, 2017

Q A
Issue EZP-28269
Related to #2158
Type Bug
BC break yes
Target branch 7.0

We've discovered inconsistent behavior of Time\Value::fromTimestamp with respect to DateAndTime\Value::fromTimestamp. The former one creates Time Value relying on PHP server timezone setting, while the latter - creates Value w/o timezone (or using UTC).

There is a difference in setting \DateTime value using constructor "@{$timestamp}" time string and calling setTimezone. The latter one takes into account server timezone.

TODO:

  • See if Travis agrees - errors are related to behat setup, not fix
  • Fix Time\Value::fromTimestamp to be timezone-independent
  • Fix unit tests.
  • Fix integration tests.
  • Add a BC note.
Show outdated Hide outdated doc/bc/changes-7.0.md Outdated
Show outdated Hide outdated doc/bc/changes-7.0.md Outdated
@andrerom

+1, but is ezsystems/repository-forms#177 needed if this goes in?

@alongosz

This comment has been minimized.

Show comment
Hide comment
@alongosz

alongosz Nov 22, 2017

Member

@andrerom Not really, ezsystems/repository-forms#177 fix stops relying on this method. It was the source of this bug, but I've pointed out to @webhdx that v1 didn't rely on that method and neither v2 should. If they rely on Value::$value, and handle displaying it in UI, which due to timezone issues is the correct way, then this fix is not needed anymore there.

But still a bug IMHO! :)

Member

alongosz commented Nov 22, 2017

@andrerom Not really, ezsystems/repository-forms#177 fix stops relying on this method. It was the source of this bug, but I've pointed out to @webhdx that v1 didn't rely on that method and neither v2 should. If they rely on Value::$value, and handle displaying it in UI, which due to timezone issues is the correct way, then this fix is not needed anymore there.

But still a bug IMHO! :)

@andrerom

This comment has been minimized.

Show comment
Hide comment
@andrerom

andrerom Nov 22, 2017

Member

But still a bug IMHO! :)

true :)
Lets do this then if you get another review ;)

Member

andrerom commented Nov 22, 2017

But still a bug IMHO! :)

true :)
Lets do this then if you get another review ;)

@lserwatka lserwatka merged commit 9ab2e6e into ezsystems:7.0 Nov 23, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
ezrobot/phpcsfixer Code review by ezrobot
Details

@alongosz alongosz deleted the alongosz:7.0-ezp-28269-fix-time-ft-from-timestamp branch Nov 23, 2017

alongosz added a commit that referenced this pull request Nov 24, 2017

[7.0] EZP-28269: Fixed inconsistent creating of eztime fromTimestamp (#…
…2159)

* EZP-28269: Aligned Time::fromTimestamp with DateAndTime::fromTimestamp

* EZP-28269: Fixed Unit Tests

* EZP-28269: Fixed eztime integration tests

* [Doc] EZP-28269: Updated 7.0 BC doc

wizhippo added a commit to wizhippo/ezpublish-kernel that referenced this pull request Apr 12, 2018

[7.0] EZP-28269: Fixed inconsistent creating of eztime fromTimestamp (e…
…zsystems#2159)

* EZP-28269: Aligned Time::fromTimestamp with DateAndTime::fromTimestamp

* EZP-28269: Fixed Unit Tests

* EZP-28269: Fixed eztime integration tests

* [Doc] EZP-28269: Updated 7.0 BC doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment