Add basic timezone support for form datetime input. #11517

dereuromark · 2017-12-07T14:43:46Z

Basic stab at #10877

Currently all forms are broken if you follow recommended UTC for DB and localized forms (e.g. UTC+1 Berlin time).
The time in forms is always behind upon transformation to output, and when you save it, it becomes even more behind as it converts it wrong a 2nd time.
Using a defaultOutputTimezone config (and optionally even a timezone dropdown maybe) you can have localized forms and keep the expected timezoned datetime.

If there is only one timezone (output), one can just use config here:

	'App' => [
		'defaultLocale' => env('APP_DEFAULT_LOCALE', 'de_DE'),
		'defaultOutputTimezone' => env('APP_DEFAULT_OUTPUT_TIMEZONE', 'Europe/Berlin'),

I kindly ask for help to finalize this for 3.6.

stickler-ci · 2017-12-07T14:44:28Z

src/Database/Type/DateTimeType.php

+     *
+     * @return bool
+     */
+    protected function _isEmpty($value) {


Opening brace should be on a new line

stickler-ci · 2017-12-07T14:44:29Z

src/Database/Type/DateTimeType.php

        return PDO::PARAM_STR;
    }
-}
+}


Expected 1 newline at end of file; 0 found

codecov-io · 2017-12-07T15:29:20Z

Codecov Report

Merging #11517 into 3.next will increase coverage by 0.28%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             3.next   #11517      +/-   ##
============================================
+ Coverage     92.17%   92.46%   +0.28%     
- Complexity    13177    16245    +3068     
============================================
  Files           459      462       +3     
  Lines         33708    39672    +5964     
============================================
+ Hits          31070    36682    +5612     
- Misses         2638     2990     +352

Impacted Files	Coverage Δ	Complexity Δ
src/View/Helper/FormHelper.php	`96.13% <ø> (+0.3%)`	`431 <0> (+61)`	⬆️
src/Database/Type/DateTimeType.php	`97% <100%> (+0.22%)`	`53 <1> (+3)`	⬆️
src/View/Widget/DateTimeWidget.php	`96.39% <71.42%> (-1.69%)`	`77 <1> (+3)`
src/Collection/functions.php	`50% <0%> (-50%)`	`0% <0%> (ø)`
src/basics.php	`30.76% <0%> (-13.68%)`	`0% <0%> (ø)`
src/I18n/functions.php	`72.41% <0%> (-11.59%)`	`0% <0%> (ø)`
src/Core/functions.php	`78.5% <0%> (-9%)`	`0% <0%> (ø)`
src/TestSuite/TestSuite.php	`92.3% <0%> (-7.7%)`	`6% <0%> (ø)`
src/ORM/Exception/PersistenceFailedException.php	`95% <0%> (-5%)`	`10% <0%> (+8%)`
src/Datasource/ResultSetDecorator.php	`71.42% <0%> (-3.58%)`	`2% <0%> (ø)`
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 866a920...2c80a4e. Read the comment docs.

saeideng · 2017-12-07T20:29:02Z

looks this is only for showing , not converting request data from user

dereuromark · 2017-12-07T21:38:15Z

Both, it converts correctly for display in the input form (dropdowns by default), and it converts back before validating/saving.
Not yet sure about any edge cases yet though.
But seems BC so far, if you do not use any timezoning here.

saeideng · 2017-12-07T21:48:00Z

do you think we can add calender option in future ?(non-Gregorian)

saeideng · 2017-12-07T22:08:00Z

how happen if someone remove hidden timezone field ?

markstory · 2017-12-08T01:50:43Z

src/View/Widget/DateTimeWidget.php

     * - `second` - Set to true to enable the seconds input. Defaults to false.
     * - `meridian` - Set to true to enable the meridian input. Defaults to false.
     *   The meridian will be enabled automatically if you choose a 12 hour format.
+     * - `timezone` - A hidden field for timezone handling when working with UTC internally.


Not just UTC really. Any timezone could be stored in the hidden input.

The point is that you store anything but UTC usually :) The UTC is the server timezone for DB storing it normalized, and the timezone is either passed in the array from user input (session data?) or coming from config by default (e.g. german based website using global gmt+1 output).
So the idea is UTC => GMT+1 for display => UTC for form handling.
Thus "working with UTC internally" - which seems to be the best practice when dealing with localized datetime across multiple timezones.

I agree UTC is what we should recommend, but people will do all sorts of interesting things.

markstory · 2017-12-08T01:52:47Z

src/View/Widget/DateTimeWidget.php

+            'templateVars' => [],
+        ];
+
+        return $this->_select->render($options, $context);


Why are we making a select and not a hidden input? Seems like a hidden input is what we want here.

I agree. How would that look like here? Using a widget in a widget?

That's what $this->_select is as well. The WidgetRegistry stitches various widget dependencies together.

markstory · 2017-12-08T01:55:29Z

src/Database/Type/DateTimeType.php

+        $internalTimezone = date_default_timezone_get();
+        if ($tz && $tz !== $internalTimezone) {
+            $time = $time->timezone($internalTimezone);
+        }


Does the time type need this kind of change as well?

I would think so, so far I only checked datetime. If anyone has the chance to help with other types, that would be more than welcome.

ADmad · 2017-12-08T05:34:27Z

src/View/Widget/DateTimeWidget.php

+                $date = new DateTime('now', $timezone);
            } elseif (is_int($value) || is_numeric($value)) {
-                $date = new DateTime('@' . $value);
+                $date = new DateTime('@' . $value, $timezone);


Passing $timezone here is redundant. From the PHP manual:

The $timezone parameter and the current timezone are ignored when the $time parameter either is a UNIX timestamp (e.g. @946684800) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00).

ADmad · 2017-12-08T05:43:36Z

src/Database/Type/DateTimeType.php

+
+        $internalTimezone = date_default_timezone_get();
+        if ($tz && $tz !== $internalTimezone) {
+            $time = $time->timezone($internalTimezone);


Doesn't this change mean the timezone should be again set to default timezone before converting the object to string in toDatabase() method?

My tests seemed to yield the right conversion, so maybe we do not? But I am happy to be proven wrong.

It is unused for unix timestmaps.

josegonzalez · 2018-02-15T05:20:31Z

Made a few small fixes, but didn't add any tests.

Skip hidden input generation if default output timezone is not set or explicitly disabled through widget data.

dereuromark · 2018-11-28T23:59:37Z

Anyone able to help out, getting this ready for 3.7 maybe?
We could also look into https://github.com/JeanValJeann/cakephp-timezone and what they did.

github-actions · 2019-09-16T00:06:34Z

This pull request is stale because it has been open 30 days with no activity. Remove the stale label or comment on this issue, or it will be closed in 15 days

Add basic timezone support for form datetime input.

d295054

dereuromark added the enhancement label Dec 7, 2017

dereuromark added this to the 3.6.0 milestone Dec 7, 2017

stickler-ci reviewed Dec 7, 2017

View reviewed changes

dereuromark added 2 commits December 7, 2017 15:45

Fix CS.

8cc5736

Fix typehint for phpstan.

f5ea005

Fix doc blocks.

97a3944

markstory reviewed Dec 8, 2017

View reviewed changes

markstory added helpers need tests labels Dec 8, 2017

markstory reviewed Dec 8, 2017

View reviewed changes

ADmad reviewed Dec 8, 2017

View reviewed changes

markstory mentioned this pull request Feb 12, 2018

ORM does not automatically convert timezones to the application timezone upon saving #11712

Closed

3 tasks

josegonzalez added 2 commits February 15, 2018 00:18

docs: switch the UTC reference

bee905d

chore: drop extra parameter

1178a0d

It is unused for unix timestmaps.

Generate hidden input for timezone.

2c80a4e

Skip hidden input generation if default output timezone is not set or explicitly disabled through widget data.

markstory modified the milestones: 3.6.0, 3.7.0 Apr 15, 2018

markstory modified the milestones: 3.7.0, Future Dec 9, 2018

dereuromark mentioned this pull request Feb 28, 2019

Take db timezone into account when creating datetime objects. #13005

Merged

github-actions bot added the stale label Sep 16, 2019

github-actions bot closed this Oct 2, 2019

othercorey deleted the 3.6-form-timezone branch December 16, 2019 04:42

Add basic timezone support for form datetime input. #11517

Add basic timezone support for form datetime input. #11517

Uh oh!

Conversation

dereuromark commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

saeideng commented Dec 7, 2017

Uh oh!

dereuromark commented Dec 7, 2017

Uh oh!

saeideng commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saeideng commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josegonzalez commented Feb 15, 2018

Uh oh!

dereuromark commented Nov 28, 2018

Uh oh!

github-actions bot commented Sep 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dereuromark commented Dec 7, 2017 •

edited

Loading

codecov-io commented Dec 7, 2017 •

edited

Loading

saeideng commented Dec 7, 2017 •

edited

Loading