datetime_datetime_widget_validate() is an #element_validate callback used in DateTimeDefaultWidget

- it could be a method in DateTimeDefaultWidget
- it actually only transforms the submitted values, and thus could be moved to massageFormValues()

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Same for datetime_datelist_widget_validate(), actually.

yched’s picture

Status: Active » Needs review
FileSize
10.11 KB

Let's see what breaks.

Status: Needs review » Needs work

The last submitted patch, 2: 2327363-datetime_widgets_validate-2.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
10.15 KB
1.03 KB

Forgot a somewhat important part...

Status: Needs review » Needs work

The last submitted patch, 4: 2327363-datetime_widgets_validate-4.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
10.38 KB
608 bytes

Gee, I'm rusty.

yched’s picture

Berdir’s picture

Looks like an awesome cleanup, two questions...

+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeDatelistWidget.php
@@ -123,6 +108,38 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+        if ($date instanceOf DrupalDateTime && !$date->hasErrors()) {

In the node widget issue, I removed the hasErrors() check because that's afaik part of the element validation and it will never get this far if you don't have a valid date by then, but I could be wrong.

Wondering why you're not seeing the problem I had with the different structure here, maybe there are no tests for a datetime field that is not accessible?

yched’s picture

->hasErrors() : yes, that seemed a bit wonky, but I'm not too familiar with DDT and what kind of errors ->hasErrors() is about, so I left that untouched.
It does look like Core\Datetime\Element\Datetime::validateDatetime() and Core\Datetime\Element\Datelist::validateDatelist() place a form error in case ->hasErrors() is TRUE, so this check looks redundant indeed. Removed.

maybe there are no tests for a datetime field that is not accessible?

Yes, seems likely...

Status: Needs review » Needs work

The last submitted patch, 9: 2327363-datetime_widgets_validate-9.patch, failed testing.

Berdir’s picture

Ah, now you left out the instanceof check too, that might be needed, because AFAIK, that happens when validation does fail.

yched’s picture

Status: Needs work » Needs review
FileSize
10.29 KB
1.72 KB

Right, too optimistic.

Status: Needs review » Needs work

The last submitted patch, 12: 2327363-datetime_widgets_validate-12.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
10.29 KB
922 bytes

Sigh.

Berdir’s picture

Looks like those two methods are now identical apart from the field type name in the comment? Is there a reason for not making a base class for those widgets and moving it there?

yched’s picture

I kind of wondered that too, wasn't sure. Seemed a bit overkill to me, but I could go both ways.
AFK though, I cannot code it myself anymore :-)

andypost’s picture

andypost’s picture

FileSize
12.69 KB
14.34 KB

moved common parts to DateTimeWidgetBase

andypost’s picture

FileSize
700 bytes
14.36 KB

clean-up use

Berdir’s picture

This looks nice, will probably need some manual testing, not sure how good the test coverage for datetime is?

andypost’s picture

Already done that:
1) the datelist widget throws the same exception when no data selected,
2) datetime works fine

But glad to get more peers on that

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yep, seems to work as good as before. Also wasn't able to reproduce any issues with #access FALSE fields.

Berdir queued 19: 2327363-datetime-19.patch for re-testing.

yched’s picture

Context changed, bit should otherwise be a straight reroll :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2327363-datetime-19.patch, failed testing.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.36 KB

re-roll

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6bad26f and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.