verbb / formie

The most user-friendly forms plugin for Craft CMS.
Other
95 stars 73 forks source link

Date field: default, min, and max dates shift by 1 hour every time I save #1980

Closed tremby closed 2 months ago

tremby commented 3 months ago

Describe the bug

These options shift back in time by 1 hour every time I edit the form, which can sometimes shift it from one day to the previous day, so it matters even if I'm truncating just to using the date.

Steps to reproduce

I'm ignoring the min and max date fields, but the same issue exists there.

  1. Add a date/time field
  2. I'm disabling time
  3. Set a default date to a specific date, example July 1
  4. Save the form
  5. Fetch the default date; I'm doing this via Graphql. I'm getting for example 2024-07-01T15:00:00+00:00. I don't know where the 15:00 is coming from. Maybe from disabled time fields...?
  6. Doesn't matter if you refresh the control panel page; I open the datetime field options again and then click "apply", and save the form again
  7. Fetch the default date again; now I'm getting 2024-07-01T14:00:00+00:00
  8. Repeat 6/7 and now get 2024-07-01T13:00:00+00:00 etc
  9. Eventually get 2024-06-30T23:00:00+00:00

Form settings

Craft CMS version

4.9.3

Plugin version

2.1.19

Multi-site?

yes

Additional context

My time zone is Pacific time.

Looks like maybe a DST issue?

engram-design commented 3 months ago

I've been testing this today with a few different timezones, and not able to reproduce this sadly. But I also think I may know the issue. To get this early, run composer require verbb/formie:"dev-craft-4 as 2.1.20".

engram-design commented 2 months ago

Fixed in 2.1.21

tremby commented 2 months ago

This isn't fixed for me.

Video showing the issue:

https://github.com/user-attachments/assets/7c9ace02-89d9-4d6c-ab3a-9ab98a2d39cb

tremby commented 2 months ago

In this video, even though I have time disabled, the date is going forward after a few saves.

https://github.com/user-attachments/assets/2d2be4ed-8489-4fa9-9dde-2f934df79a7f

engram-design commented 2 months ago

Should be fixed this time for the next release. To get this early, run composer require verbb/formie:"dev-craft-4 as 2.1.21".

tremby commented 2 months ago

https://github.com/user-attachments/assets/b7ebccfb-e4a8-4840-ad13-bcf52df72f3b

engram-design commented 2 months ago

That should be fixed this time for the next release. To get this early, run composer require verbb/formie:"dev-craft-4 as 2.1.21".

In addition, I've made a slight change with the value for GraphQL for defaultValue, defaultDate, minDate, maxDate which I don't believe is a breaking change, but let me know if it is for you.

These values now return as Y-m-d H:i:s formatted strings rather than an ISO 8601 date. This is because these values don't store timezone information, and it's misleading to output them as such. For example, before this change dates were in format 2024-07-01T15:00:00+00:00 which is now 2024-07-01 15:00:00.

tremby commented 2 months ago

Fwiw ISO 8601 doesn't require the zone information. 2024-07-01T15:00:00 would be valid.

Giving this a whirl now.

tremby commented 2 months ago

Whatever time I input seems to be immediately reset to 00:00 when I close the field settings modal, for default, min, or max.

tremby commented 2 months ago

In addition, I've made a slight change with the value for GraphQL for defaultValue, defaultDate, minDate, maxDate

What's the difference between defaultValue and defaultDate by the way? Aren't they always the same?

engram-design commented 2 months ago

Strange. Here's a video of my testing in case I'm doing something wrong?

https://share.cleanshot.com/dhgyGbqb

And yes defaultValue and defaultDate are essentially the same, although defaultValue returns a string, and defaultDate returns a craft\gql\types\DateTime but they're both cast as strings I believe. I think there's some extra things you can do with arguments for a DateTime.

tremby commented 2 months ago

which I don't believe is a breaking change, but let me know if it is for you

It is; my code expected to see the T, but was going to ignore any time zone info. It's not a big annoyance to change on my end. But as I mentioned above ISO 8601 doesn't need the time zone so it might be less grief to use a T rather than a space.

engram-design commented 2 months ago

Happy to include the T then to save you the hassle. Have just updated the craft-4 branch with that.

tremby commented 2 months ago

Strange. Here's a video of my testing in case I'm doing something wrong?

https://share.cleanshot.com/dhgyGbqb

Here's me, after definitely being at commit 8004afa (edit: same behaviour on commit 2100501)

https://github.com/user-attachments/assets/316b94cc-b51e-4b50-b7d6-b200801a0ff5

If I had to guess why my experience is different from yours I'd guess browser or locale. I'm on recent Firefox, and I'm not sure exactly what my locale is. Might be any of en-CA, en-US, en-GB...

engram-design commented 2 months ago

I figured the same as well, and have been testing with changing the Craft user preferences, browser and OS settings. None seem to produce what you're seeing. I'll keep at it!

tremby commented 2 months ago

I noticed my AM/PM is uppercase and yours is lowercase, presumably due to Intl.DateTimeFormat or whatever you're using to render times. Any regexing/string matching going on which might care about that?

engram-design commented 2 months ago

I did think that too, but we do factor that in when we convert the value (e.g. 2:00 AM) from the jQuery Craft Timepicker field to proper 24-hour time 02:00. And just to confirm my US-based settings is still working correctly: https://share.cleanshot.com/BpDMzJhC

tremby commented 2 months ago

Thanks for pointing out that code. I dropped a breakpoint and observed it. There is indeed a locale-related bug there.

When I run it, with whatever OS or browser or jquery locale options I happen to have, the timeStr argument is 2:00 AM. That whitespace character isn't a regular space, it's not even a regular no-break space, it's character 8239, a "narrow no-break space".

You would probably get some mileage out of

-            const [time, modifier] = timeStr.split(' ');
+            const [time, modifier] = timeStr.split(/\s+/);

to split on any whitespace rather than only specifically a single space.

But IMO this solution is still brittle, because who knows what other formats the text could be in in other locales.

engram-design commented 2 months ago

How interesting, I wouldn't have picked that! Well done on hunting that down.

I've refactored our code to use Craft's jQuery Timepicker getTime function to actually gave us a non-locale-formatted value to use, rather than trying to parse it ourselves. Hopefully, that should put this issue to bed.

To get this early, run composer require verbb/formie:"dev-craft-4 as 2.1.21".

tremby commented 2 months ago

Excellent; the date and time seem stable now. Thank you.

engram-design commented 2 months ago

Fixed in 2.1.23