Form validation handlers cannot alter $form structure

I agree that this is much needed. It has been painful having to work around this. I think it could be useful to document this change though (if only to make sure we don't revert it) and/or to write a small test for it. Otherwise looks RTBC.

+++ modules/simpletest/tests/form.test	26 Nov 2009 11:39:27 -0000
@@ -153,6 +153,63 @@ class FormsTestCase extends DrupalWebTes
+      'description' => 'Tests form processing and alteration via form validation handlers.',

'alteration' is a fun word, a bit archaic even. It's correct though so no need to change this.

I think this is RTBC.

I agree this would be convenient. Only thing is I'm pretty sure this was done as a deliberate decision so that validation was just for validating, since we used to be able to do changes to forms in #validate back in 4.7/5-ish.

It'd be nice to have chx or Eaton chime in here.

If it was an intentional decision to not pass $form by reference, I suspect it was because if a validation function makes a change to $form and there are validation errors, then the changed $form isn't cached, so when the user re-submits it, the input would be processed against a stale cached form build. If this change goes in, we would need to change the caching logic in drupal_build_form(). I suspect the intended way to accomplish the use-case described in this issue is to use $form_state['rebuild']. However, there are currently some major problems with form rebuilding. One of them is being worked on in #622922: User input keeping during multistep forms. But another one is that drupal_build_form() only calls drupal_rebuild_form() if there were no validation errors, which seems to cancel out any possibility for a validation function to trigger a rebuild. However, image_style_form_add_validate() and book_admin_edit_validate() attempt (unsuccessfully because of previous sentence) to trigger a rebuild from a validation error. So, basically, WTFs all over the place.

Seems like we need help from chx, Eaton, or another FAPI guru to explain exactly what form rebuilding was intended for, and if we need to fix it to work from validation handlers as well as submit handlers, and if not, then how else can the CAPTCHA use-case in this issue be solved.

Works for me. Thanks for the explanation. I'll keep that in mind when working on the rebuild issues.

I am not changing the status but I am mailing Heine and want to talk to him before let this through.

Suppose you change the form. if you do not change the form_state['values'] then you might get unvalidated form_state['values'] to the form submit.

However, this is not new. after_build already let's you do that. So I am blessing this if you add a warning to drupal_validate_form warning not to do that. (Optionally you can commit such a warning to the fapi docs in contrib for after_build too).

Edit: i did not need to send the mail to Heine because as I was trying to explain what caused me such unease I finally realized it and this was it.

Found a small typo:

+++ includes/form.inc	27 Nov 2009 15:07:43 -0000
@@ -714,7 +714,13 @@ function drupal_prepare_form($form_id, &
+ *   an validation error. If a validation handler alters the form structure, it
+ *   is responsible for validating the values of changed form elements in

"an validation error" should be "a validation error". I can fix that when I commit the patch though.

I believe we are good to go.

Committed to CVS HEAD. Thanks sun and chx.