Datetime select list widget has options for time interval on date only mode

Hello,

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.

Just for illustrate:

Without the patch:

With the patch:

If 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.

+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeDatelistWidget.php
@@ -128,11 +134,17 @@ function settingsForm(array $form, FormStateInterface $form_state) {
-    $summary[] = t('Date part order: !order', array('!order' => $this->getSetting('date_order')));
+    $summary[] = t('Date part order: @order',
+      ['@order' => $this->getSetting('date_order')]
+    );
     if ($this->getFieldSetting('datetime_type') == 'datetime') {
-      $summary[] = t('Time type: !time_type', array('!time_type' => $this->getSetting('time_type')));
+      $summary[] = t('Time type: @time_type',
+        ['@time_type' => $this->getSetting('time_type')]
+      );
+      $summary[] = t('Time increments: @increment',
+        ['@increment' => $this->getSetting('increment')]
+      );
     }
-    $summary[] = t('Time increments: !increment', array('!increment' => $this->getSetting('increment')));

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.

Removing old hashed tag.

Writing tests

Added 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:

1. Adding Date field to content type
2. Selecting Date only as Date type
3. Going to Manage form display tab and switching newly created field to Select list
4. Save form display updates
5. Do not click on settings widget (or save an update to these settings), saving updates to the widget will produce the correct behavior
6. Create new content, note that the field incorrectly includes selections for hour and minute

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.

Setting to needs work per comments [#16] & [#12]

The 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.

Removed 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.

Reviewed 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.

@mpdonadio re update hook - but does anything actually change in the stored field configuration?

Had 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.

OK, 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.

Found additional issue that this fixes while testing. The additional item it addresses is directly related and in scope.

Thanks for checking that @mpdonadio.

Committed 935fd2c and pushed to 8.0.x. Thanks!