There is some logic in place to avoid running the validation on a date popup more than once because validating the widget also changes its data. The logic does not work because it is looking for the data in the wrong place.
Details: At the end of date_popup a call to form_set_value changes the data in to a string inside the form state. From the comments of form_set_value:
It does not change the value in $element['#value'], only in $form_state['values'], which is where submitted values are always stored.
At the beginning of date_popup_validate this string is checked for in $element["#value"]. Since it wasn't set there, the rest of the validation will run and it will produce an incorrect form error because it can not handle the modified data.
I will post a patch for it.
Comment | File | Size | Author |
---|---|---|---|
#12 | date-validation-error-2199587-12.patch | 623 bytes | oadaeh |
#7 | date-validation-error-2199587-7.patch | 649 bytes | oadaeh |
#1 | date_popup-second_validation-2199587-1.patch | 735 bytes | jmuzz |
Comments
Comment #1
jmuzz CreditAttribution: jmuzz commentedComment #2
jmuzz CreditAttribution: jmuzz commentedThis is part of a solution for #1289062: Required fields in collection not validated when empty. I tried to add it in the related issues field but it was ignored when I submitted.
Comment #3
vijaycs85@jmuzz, Thanks for reporting issue and your patch. Setting this to review to testbot pick up this patch. However it would be great if:
1. Formal steps in issue summary to reproduce/manual test patch.
2. Test only that proves current implementation is wrong and this patch fixes it.
Comment #4
jmuzz CreditAttribution: jmuzz commentedOh yeah I meant to set it to needs review.
I don't know a way to test it other than by using the patch I posted in that field collections issue. In what circumstances would a field 'normally' need to be validated twice during one submission?
Comment #5
dafeder#1 worked for me. I was getting the "second validation" error when I submitted a custom form with a "date_popup" element. Node forms were working fine.
Comment #6
cafuego CreditAttribution: cafuego commentedThis also fixes a double validation problem with the node_expire module. I'm tempted to apply the patch as-is.
Comment #7
oadaeh CreditAttribution: oadaeh commentedWhat I'm seeing is this: when submitting a multistep form with a programmatically created date field, the field's values are validated twice. When a date field successfully passes validation, the validated date is saved back to the $form_state['values'] array. The first time through, everything is okay, except that the date goes from being an array to a string. The second time through, the date string no longer represents a "valid" value and fails validation.
Probably the reason the problem doesn't manifest itself so much is that a Field API created date field (or one created in the Drupal UI) is a multi-nested array, where as a programmatically created date field is only(usually?) a single-nested array. The multi-nested arrays pass through the dual validation check fine, because they are still arrays the second time through.
While it may work to fix this problem, I believe the previously proposed patch reverts a fix for other functionality.
I've attached a patch that works for me and basically resaves the new string date as an array to $form_state['values'].
Comment #10
Kristen PolPatch #7 fixes the issue for me but it isn't passing tests so I've resubmitted it to try one more time.
Comment #12
oadaeh CreditAttribution: oadaeh commentedOkay, so more testing in a live situation proved that my fixed caused some unwanted side effects.
@jmuzz's recommended fix does work, but I still think removing the other code reverts a fix for another bug (that I was unable to locate, but is mentioned in the code comments), and it's removal is unnecessary for this issue.
I've attached a patch that adds his fix (w/o the code removal) and a coment explaining why it necessary.
Locally, there are test failures with the Date Migration and Timezone & Granularity tests, but they also fail without the attached patch.
Comment #13
oadaeh CreditAttribution: oadaeh commentedSetting the status.
Comment #14
Kristen PolThis fixed the problems I was seeing with other date fields as well as the original multi-step form issue. Thanks!
Comment #15
cafuego CreditAttribution: cafuego commentedOk, I'll apply this patch to date on an unrelated project that was fixed by #1 and see if behat is happy with it. If so, it's going in :-)
Comment #16
cafuego CreditAttribution: cafuego commentedAaaand applied. Nice work!