getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

Date Field does not store the picked date properly #2416

Closed joernroeder closed 3 years ago

joernroeder commented 4 years ago

Describe the bug

As reported by a client and replicated in a plainkit the date field does not save the selected value (on GMT+1) after updating to the latest version.

To Reproduce
Steps to reproduce the behavior:

  1. Get the plainkit and modify the following files:

default.yml

title: Default Page
preset: page
fields:
  date:
    label: Date
    type: date
    default: today
  text:
    label: Text
    type: textarea
    size: large

home.txt

Title: Home

----

Date: 2020-02-21

----

Text: 
  1. Go to the home page in the panel
  2. The date field is prepopulated with the correct date 2020-02-21
  3. Choose the 22th feb via the calendar on the right hand side of the date field
  4. Open the inspector and save the page.
  5. Inspect the params of the PATCH request (it send 2020-02-21T23:00:00.000Z in my case)
  6. Confirm that home.txt is still stores 2020-02-21
  7. Reload the page and the date field shows 2020-02-21 again

Expected behavior

the text file should store the selected date. 2020-02-22

Screenshots

kirby-datefield-bug

Kirby Version
v3.3.3

PlainKit used plainkit-master-datefield-bug.zip

afbora commented 4 years ago

It looks very much like this issue #1813

I can't reproduce with your files as default. But i get this error only when the server time zone (GMT+2, saved 2020-02-21 23:00) is before/less my OS (may be browser)'s timezone (GMT+3, posted 2020-02-22 00:00). So it seems that this error occurs when the server and the user have different time zones 🤔

You can temporarily solve the issue using date_default_timezone_set()

@distantnative What do you think?

joernroeder commented 4 years ago

@afbora i was able to replicate it on localhost, served via php -S localhost:5555 with the plain kit in the zip file. would be interesting whats the default timezone for the php server and if it differs from the machine

afbora commented 4 years ago

@joernroeder Just wondering that does the error persist when setting the current time zone with date_default_timezone_set in your root index.php?

joernroeder commented 4 years ago

@afbora i can give it a try but whats also weird is that the PATCH request already sends the wrong date (2020-02-21T23:00:00.000Z) down the wire, strangely by the offset to GMT (+1) 🤔

joernroeder commented 4 years ago

@afbora changing index.php to the following does not change the PATCH request payload on save.

index.php

<?php

date_default_timezone_set('Europe/Berlin');

require 'kirby/bootstrap.php';

echo (new Kirby)->render();

Request Payload is still:

{"date":"2020-02-21T23:00:00.000Z","text":""}
afbora commented 4 years ago

@joernroeder Yes, the request is always doing by GMT+0. Still wrong date in txt file on date_default_timezone_set test?

texnixe commented 4 years ago

Interesting, I can reproduce it in the Kit you supplied, but not in a Starterkit.

joernroeder commented 4 years ago

@afbora @texnixe setting the timezone solved the issue for me and the date in the text file now corresponds to the selected one. (even tho it gets send as GMT+0)

joernroeder commented 4 years ago

@texnixe thats weird. I've downloaded it from the repo this morning. are you using the same server to serve the kits?

texnixe commented 4 years ago

Yes, I tested locally using Valet. Have to try and find what the difference is. I'll try again with both a fresh Starter- and Plainkit later.

texnixe commented 4 years ago

I don't know, it's really weird. Trying to understand under what circumstances this happens or not but don't really.

yanickvw commented 4 years ago

I noticed the same behaviour since version 3.3.3 (plain kit - MAMP). Setting the timezone in the index.php file solved the issue.

joernroeder commented 4 years ago

@yanickvw syncing timezones fixed it for me as well. but I am wondering what would happen I edit it from a different timezone 🤔

afbora commented 4 years ago

@distantnative @lukasbestle @bastianallgeier As I said in the other issue #1813, why don't we add a time zone option to the core like that?

return [
    'debug' => true,
    'timezone' => 'Europe/Istanbul',
];
public function setTimezone($timezone = null): void
{
    if ($timezone === true) {
        $timezone = ini_get('date.timezone');
    }

    if (empty($timezone) === false) {
        date_default_timezone_set($timezone);
    }
}
afbora commented 4 years ago

@texnixe I can reproduce only with different timezones not about default option for me.

texnixe commented 4 years ago

It seems to have been something else, not related to the default option. I haven't really found out yet why it works in the Starterkit but not in the Plainkit setup provided by @joernroeder .

joernroeder commented 4 years ago

@afbora why is it labeled as information missing? Is there anything I can do to push this further?

afbora commented 4 years ago

@joernroeder We were unable to fully reproduce the issue. I'm not really sure timezone issue. Were you able to look again? @texnixe

joernroeder commented 4 years ago

hm, as mentioned before, I was able to reproduce it in a plainkit downloaded from github and linked that above. I might clone the repo again, to see if i'm able to reproduce with 3.3.4. I still think there is some quirks behind, especially as the addition of a default timezone seems to fix it.

afbora commented 4 years ago

I suggested adding the timezone option to the core before, it would be nice. Even so @lukasbestle and @distantnative's thoughts can give us ideas about the issue and possible solutions.

lukasbestle commented 4 years ago

I think it would be nice to fix this in a way that doesn't require a manual timezone setting. After all the API request from the Panel contains the timezone and is therefore absolute, so the backend "only" needs to ensure that the value gets properly converted based on whatever the current server timezone is. That way it will work no matter if the server and client timezones match or not.

The other question is: Why did this break in 3.3.3?

afbora commented 4 years ago

Btw when i change timezone as Australia/Sydney (-660), date field seems like that for all (3.2.5, 3.3.0, 3.3.1, 3.3.2, 3.3.3) 🤔

image (1)

Content

----

Date: 2018-10-11 00:15

----
jonasfeige commented 4 years ago

Running into the same issue on Kirby 3.3.4 using Laravel Valet. Currently in Lisbon (GMT+0). Setting the timezone in the index.php did not work for me, only when I manually change my machine’s timezone back to Berlin(GMT+1) does the date field work as expected.

moevbiz commented 4 years ago

I had the same issue. Setting the timezone in index.php to Europe/Berlin fixed it. I am in Austria, the website will be hosted on a server in France and the panel will be mostly used from Switzerland – let's see what's gonna happen… :~)

Jayshua commented 4 years ago

I am in Austria, the website will be hosted on a server in France and the panel will be mostly used from Switzerland – let's see what's gonna happen… :~)

This is why I generally discourage relying on the server's timezone. IMHO it should always be UTC. A few points:

Here's what I do/suggest kirby does

Store the date/time exactly as the user entered it without changes. This normally means always treating it as a string and never passing it to any date related functions or libraries since those usually try to do some timezone conversion. I never use Kirby's date picker for the reasons above, just a regular text input with a regex.

Always store the intended timezone as well. I always have this default to the panel editor's timezone, but I also let them change it if they need to. (That's usually important for my purposes because I deal with a lot of event planning in different timezones. Changing the timezone might be less important for other use cases, but storing it is still important to correctly handle changes to timezone rules.)

Usually there are only a few relevant timezones for my panel editors, so I give them a multiselect with a handful of pre-selected timezones to choose from rather than the entire 500+ list of all timezones.

Since Kirby sees translation and internationalization as important features, I think it should also have a correct and reliable way to deal with timezones and dates built-in. As far as I know this requires always storing the timezone along with the date, except when dealing with a date who's timezone is relative to some other entity. (Like my birthday.)

plagasul commented 4 years ago

I am having the same problem and had found the issue #2566 instead of this one, where I added some information. In that issue @hdodov interestingly points out that adding time: true to the date field solved the issue, may that be a hint to what this broke or how to fix it ?

I did not test with plain or starterkit, only with several websites on the same server. 3.3.1 does not seem to produce this problem in my case.

Setting the timezone in index.php 'solves' the issue in the sense that the server timezone becomes irrelevant, but not in the sense that accessing the panel from a different timezone to what is specified will still cause the issue, or so it seems from my tests.

iskrisis commented 4 years ago

Can confirm. This messed up live website after kirby update both valet and server. Editors figured it out and started to put one day more... and laughed at my incompetence.

Forcing timezone in index fixes it.

SirNovi commented 4 years ago

Same Issue on 3.4.0 locally with Valet. Agree with Jayshua "Store the date/time exactly as the user entered it without changes."

bastianallgeier commented 3 years ago

@distantnative is this still a problem with the new date field?

distantnative commented 3 years ago

If so it might be due to different reasons. I'm closing this everyone - please report and open a new issue if you see similar problems with the new date field.