Problem/Motivation
The magic code used to determine whether a date is "all day" has caused major problems.
Proposed resolution
Magic code needs to be removed in favor of a simple value stored in the database to indicate a date is considered "all day".
Remaining tasks
Add a new column to the module's base field definition.Rework the all-day logic to use the new field.Add test coverage for the update script.Fix the "text field" widget when using a "Date" field.Update the new update script to verify the columns were actually removed during the prep work.Code cleanup.Test coverage for all all-day field types.More test coverage.Regression testing, including testing other modules.Write a change notice.Update scripts for converting existing data go into #3201878.- Anything else?
User interface changes
None.
API changes
TBD.
Data model changes
TBD.
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | date-n3202962-33.patch | 53.54 KB | damienmckenna |
| #33 | date-n3202962-33.interdiff.txt | 1018 bytes | damienmckenna |
Comments
Comment #2
damienmckennaComment #3
sgdev commentedI agree. One thing to consider is the way an "All day" value is stored includes a full datestamp, but it essentially ignores the time portion:
Stored Start: 2020-01-01 00:00:00 --> 2020-01-01
Stored End: 2020-01-01 00:00:00 --> 2020-01-01 (would expect 2020-01-01 23:59:59, but module "chops off" time)
So in that sense, it is a rather "simple" value, but one that attempts to repurpose a full datestamp into a value that only uses the day.
Whatever the approach, it will need to take into account that date fields can be configured in the interface to *not* store seconds, minutes, and even hours (always store as 00). In our implementations, users only enter hours and minutes with a timepicker, and are restricted to only be able to select :00, :15, :30, and :45.
Comment #4
damienmckennaWIP patch to add the necessary field schema changes.
Comment #5
damienmckennaThis needs further work to update the internal APIs, and then fix existing data.
Comment #6
damienmckennaA test for the update script. Note that this kinda cheats because the field is not fully removed before the update script attempts to add it back again. Also, none of the internal APIs have been updated yet.
Comment #7
damienmckennaComment #8
damienmckennaComment #9
damienmckennaWIP, didn't want to loose it.
Comment #10
damienmckennaWIP to use hook_field_widget_form_alter() to add the all-day option.
Comment #12
damienmckennaFurther WIP.
Comment #13
damienmckennaComment #15
steinmb commentedDamien
I just wanted to say, thank you for working on this, and all you hard work.
Comment #16
mustanggb commentedBig thanks from me too, started to look at 2.11 for the PHP 7.3/7.4 fixes, but should probably hold off until 2.12 lands.
By the way I know there is a warning in the release notes: https://www.drupal.org/project/date/releases/7.x-2.11
But it's probably worth adding one at the top of the project page as well: https://www.drupal.org/project/date
Comment #17
damienmckennaFurther work.
Comment #18
damienmckennaI realized that now we have a specific value for indicating "this is an all-day thing" we don't need most of the code in theme_date_all_day().
Comment #20
damienmckennaThe one code test works, now to fix the update script, clean up the code, write more tests, and look at backwards compatibility. Little things ;-)
Comment #21
damienmckennaI realized it was looking for the wrong field name - the suffix is "all_day" rather than "is_all_day". It also didn't need to fully clear the caches, just rebuild the schema.
Comment #22
damienmckennaTests are green! Woot!
Next off:
Comment #23
damienmckennaSpotted another bug - when you try to set a date to "all day" and it is using the "text field" widget it gives an error if the value includes a time portion, e.g. "07/22/2021 - 17:30" might be the default value from the widget but it fails, it actually requires the value "07/22/2021" in order to pass validation.
Comment #24
damienmckennaThis includes the following changes:
I know the first one will fail, hopefully the others are ok.
Comment #26
damienmckennaThe field validation needed to be changed for the all_day validation to work. That one test works correctly, let's test the whole thing.
Comment #27
damienmckennaRefactoring the tests to rep for more test coverage.
Comment #28
damienmckennaNew content type.
Comment #29
damienmckennaMore test coverage.
Comment #30
damienmckennaSo that fixes the text field widget.
Now to add test coverage for the timestamp field and its widgets.
Comment #31
damienmckennaTest coverage for the timestamp field.
Comment #33
damienmckennaDoh. Typoo.
Comment #34
damienmckennaDo we need more test coverage, especially unit test coverage, of the all-day functionality? Any thoughts?
Comment #35
damienmckennaFYI #3201878 will focus on updating existing data to the new system.
Comment #36
joseph.olstadSo to test this, what is the test plan and what to look for ?
install from fresh vanilla, set up all day value
compare timezone settings and stored results in the db tables ? and also the ui, test dates in views also?
I have a 7.80 environment I could run this on.
probably will have a closer look at the automated tests, could use that as a test plan.
Comment #37
damienmckennaWe need to identify whether other modules break because of the change. We also need to test it with e.g. timezone changes.
Comment #38
damienmckennaI also wonder if this should require a new branch, i.e. to take over the 7.x-3.x branch?
Comment #39
joseph.olstadya I would be a proponant of taking over the 7.x-3.x branch and cutting a release, allow a few people to try it in the wild, then if no major issues, set it as the recommended release branch as 7.x-3.0
Comment #40
damienmckennaI created a draft change notice.
Comment #41
joseph.olstadchange request looks good thanks @DamienMcKenna
Comment #42
damienmckennaWill do new issues for some of the pieces.
I've committed this as-is to the 7.x-3.x branch.
Comment #43
damienmckennaComment #44
damienmckennaComment #47
damienmckennaComment #48
lwalley commentedFor reference, there is a remaining UX issue when editing date popup with time field with all day checked on 7.x-3.x branch, see #3274909: Error when time is missing from an all-day event, in summary UX issue is:
User checks All Day which hides time field via JS, then on node save an error occurs saying time field is required.
Workaround: user must uncheck All Day to show time field, add time e.g. 00:00 then re-check All Day and then save will succeed without error.