Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Datetime select list widget has options for time interval on date only mode:
In addition, with the default settings, the time portion is shown on the edit page:
Proposed resolution
- Update settings form to not display time related elements for date-only fields
- Update settings summary to not display time related elements for date-only fields
- Update widget to not display time related elements for date-only fields
Remaining tasks
None.
User interface changes
Time related elements are removed for date-only fields.
API changes
None.
Data model changes
None. See #29
Beta phase evaluation
Issue category | Bug because the datetime select widget exposes unrelated elements in time-only mode. |
---|---|
Issue priority | Normal because it is not a significant impact on the end user. |
Prioritized changes | The main goal of this issue is fixing a bug, and allowed during beta. |
Disruption | None |
Comment | File | Size | Author |
---|---|---|---|
#30 | Screen Shot 2015-08-06 at 8.25.32 PM.png | 38.52 KB | mpdonadio |
#29 | patch_w_save.txt | 1.38 KB | mpdonadio |
#29 | patch_w_defaults.txt | 1.38 KB | mpdonadio |
#29 | head_w_save.txt | 1.38 KB | mpdonadio |
#29 | head_w_save_plus_patch.txt | 1.38 KB | mpdonadio |
Comments
Comment #1
NickDickinsonWildeComment #2
NickDickinsonWildeAs discussed on IRC, changed the ! placeholders to @ (1/3 of those lines had to change for this fix already and the others were over 80 chars long so I had changed them anyways since they were right there). Also changed a couple other long lines to from array_merge to the faster $arr += []; - yes that could be thought of as out of scope but also very small and within a few lines of the other changes.
Comment #4
NickDickinsonWildemissing trailing new line
Comment #5
clairedesbois@gmail.comHello,
I have tested your patch. It works for a only date but for a date time, select lists of hours and minutes have disappeared during the creation of new entity.
Comment #7
clairedesbois@gmail.comJust for illustrate:
Without the patch:
With the patch:
Comment #8
clairedesbois@gmail.comComment #9
clairedesbois@gmail.comComment #10
clairedesbois@gmail.comComment #11
clairedesbois@gmail.comComment #12
mpdonadioIf this is a bug, then we need test coverage. There should be a -test-only patch that demonstrates the problem (ie, fails), and then a patch that includes the fix + the new test that passes.
Also, when posting new patches, please post an interdiff so we can see what is different between versions.
The wraps for 80 cols are unnecessary here, and the [] placement isn't correct per coding standard anyway. Just keep everything on one line.
I'll review everything more fully when a test in in place, and this really needs to be read applied anyway for it to make sense.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commentedUpdating the issue tag to include a hashless DrupalNorth2015 on behalf of the Drupal North sprinting group.
Comment #14
mparker17Removing old hashed tag.
Comment #15
justAChris CreditAttribution: justAChris as a volunteer commentedWriting tests
Comment #16
justAChris CreditAttribution: justAChris as a volunteer commentedAdded tests to check for the increment select box for both Date only (doesn't appear) and Date and time (does appear) types.
Test only patch should have 3 fails, combined patch should resolve one of these fails, confirming that the previous patch removes the increment option for Date only. Remaining two fails were exposed during testing and although is not in the initial scope of this issue, is highly related, and likely touches the same lines of code. I think we should address these here, but correct me if i'm wrong.
The test fails can be reproduced manually by:
User should not have to click on settings widget to get correct behavior on field.
Included patches do not provide an update to resolve these or address [#12], so one of the original developers can pick this back up if they want.
Comment #21
justAChris CreditAttribution: justAChris as a volunteer commentedSetting to needs work per comments [#16] & [#12]
Comment #22
mpdonadioThe tests in #16 looks valid, as does a manual test. Fixing this is within scope of this issue. Pretty sure the problem is that DateTimeDatelistWidget::defaultSettings() is creating invalid settings for the date-only fields. Mimicking what goes on with the default values in ::settingsForm() should be sufficient.
Comment #23
mpdonadioComment #24
justAChris CreditAttribution: justAChris as a volunteer commentedRemoved wrap per comment #12, updated variable replacement in string to conform to coding standards.
Can't make changes to DateTimeDatelistWidget::defaultSettings() as suggestion in comment #22 since it is a static method and I don't think can access the datetime_type at that point, correct me if i'm wrong. I've updated DateTimeDatelistWidget::formElement() to mimic the ::settingsForm() logic instead.
This probably isn't the cleanest solution with all of these conditionals floating around. Painfully, maybe the Datetime type should extend a new Date only class, this would be a bit of refactoring though.
Comment #25
mpdonadioReviewed patch and don't have any issues with the code. Also tested this manually. Looks good to me.
I am setting RTBC on this, but I am not sure if this will need an update hook + test to handle the 'time_type' setting. I have read this several times, and also done a decent amount of manual testing before/after patch with different scenarios and don't think we do. But, I am going to defer to @alexpott or @catch on this.
Comment #26
alexpott@mpdonadio re update hook - but does anything actually change in the stored field configuration?
Comment #27
mpdonadioHad brief conversation with @alexpott about this. I am going to verify the values in storage before and after to determine whether we need an upgrade path or not. Will hopefully have answer tonight or tomorrow.
Postponing briefly so it gets out of the RTBC queue for the committers.
Comment #28
mpdonadioComment #29
mpdonadioOK, this was fun. This is what I did.
Checkout HEAD. Create fied_date_only. Change widget to select. Save widget settings. Save config as head_w_default.txt
Apply patch. drush cr. Save config as head_w_default_plus_patch.txt
Checkout HEAD. Create fied_date_only. Change widget to select. Click gear. Save widget settings. Save config as head_w_save.txt
Apply patch. drush cr. Save config as head_w_save_plus_patch.txt
Checkout HEAD. Apply patch. Create fied_date_only. Change widget to select. Save widget settings. Save config as patch_w_default.txt
Checkout HEAD. Apply patch. Create fied_date_only. Change widget to select. Click gear. Save widget settings. Save widget settings. Save config as patch_w_save.txt
It looks like we don't need an upgrade path. The widget settings behave the same with a build from HEAD, build from HEAD then apply patch, and build from HEAD+patch.
However, the widget settings are a bit wonky because of the way default values work (the static method can't get at the instance settings). However, the patch addresses this by taking the instance type into account instead of just the widget settings. So, I am setting this back to RTBC. Ideally, someone should double check the above to make sure I didn't botch it.
Comment #30
mpdonadioFound additional issue that this fixes while testing. The additional item it addresses is directly related and in scope.
Comment #31
mpdonadioComment #32
alexpottThanks for checking that @mpdonadio.
Committed 935fd2c and pushed to 8.0.x. Thanks!