Form constructors cannot enable form caching or form rebuilding [#648170]

Comment	File	Size	Author
#13	drupal.form-cache-cache.13.patch	2.58 KB	sun

#10	cache.patch	2.34 KB	fago

Comment #1

sun

German

Karlsruhe

CreditAttribution: sun commented 2 December 2009 at 01:10

Title:

Form constructors cannot enable form caching

» Form constructors cannot enable form caching or form rebuilding

Same applies to form rebuilding.

Log in or register to post comments

Comment #2

fago

German

Vienna

CreditAttribution: fago commented 2 December 2009 at 11:09

What about caching the flags and clearing them out on rebuild? That should do it.

Log in or register to post comments

Comment #3

mattyoung CreditAttribution: mattyoung commented 2 December 2009 at 14:19

~

Log in or register to post comments

Comment #4

sun

German

Karlsruhe

CreditAttribution: sun commented 2 December 2009 at 15:40

@fago: Not sure what this has to do with rebuilds?

Log in or register to post comments

Comment #5

fago

German

Vienna

CreditAttribution: fago commented 2 December 2009 at 17:38

If we don't clear them out on rebuild we would assume 'rebuild' = TRUE for the next step, even if no one sets it. But the default currently is 'rebuild' = FALSE. This if we want to keep the current behaviour we have to set them back to the defaults when we rebuild the form.

Log in or register to post comments

Comment #6

sun

German

Karlsruhe

CreditAttribution: sun commented 2 December 2009 at 17:58

What I meant is: We currently do not reset 'cache' nor 'rebuild' anywhere.

Let's start with $form_state['cache']... this is only ever set in form_state_defaults(), but form_state_defaults() is not invoked after the form constructor is invoked.

So the question is why is it reset at all, no?

Log in or register to post comments

Comment #7

fago

German

Vienna

CreditAttribution: fago commented 2 December 2009 at 18:57

Because the $form_state isn't persistent across page requests and the only thing cached is storage and build_info until #641356: Cache more of $form_state when form caching is enabled lands. Once the form is cached, the constructor won't run on the next request thus it won't be able to set $form_state['cache'].

Well for $form_state['cache'] persisting it should be all we need to do, as once we rebuilt the form once we automatically cache for the rebuilds. Thus it doesn't matter whether $form_state['cache'] defaults to on or not now.

But $form_state['rebuild'] has to be reset to the default on rebuild, once it's persisted. Else the default would change to ON for the subsequent rebuilds.

Log in or register to post comments

Comment #8

fago

German

Vienna

CreditAttribution: fago commented 4 December 2009 at 13:14

@$form_state['cache']:
As the form workflow docu page visualizes good, it doesn't matter whether we cache $form_state['cache'] for usual rebuilds. It does only matter in case of a validation failure, which issues no proper rebuild, thus the form won't be cached in that case. Persisting $form_state['cache'] will fix that.

@$form_state['rebuild']:
Let's persist it and clear the flag on rebuild (see #7 for why), then setting it during construct should be fine.

Log in or register to post comments

Comment #9

sun

German

Karlsruhe

CreditAttribution: sun commented 16 February 2010 at 18:05

I have an arbitrary form with $form_state['cache'] = TRUE during construction. That 'cache' flag is reset to FALSE (at least) in case of a validation error. Therefore, the cached $form_state is not updated after processing, and I entirely lose the data my validation handler tries to store in $form_state.

We absolutely want to persist the 'cache' flag. Do you have a concrete plan of attack already? :)

Regarding 'rebuild' -- before we proceed, does it even remotely sense to define the 'rebuild' flag in the form constructor? Such a form won't submit ever?

Log in or register to post comments

Comment #10

fago

German

Vienna

CreditAttribution: fago commented 17 February 2010 at 15:13

Status:

Active

» Needs review

File	Size
cache.patch	2.34 KB

Yep, patch attached. It does only fixes the 'cache' flag, I didn't touch 'rebuild'. It also fixed some hacks previously necessary in the form testing module.

@rebuild:
Well I think it makes sense. Still the form submit callback is executed, so the form can be submitted just fine. Instead of having the default redirect just loading the form again from scratch it would just rebuild it for the "last" step too. However currently 'rebuild' => TRUE is used to indicate form constructors to be in the rebuild phase, so we can't set it back to default without changing that (API change!). As I don't think its important to support that, I just left it as is.

Log in or register to post comments

Comment #11

fago

German

Vienna

CreditAttribution: fago commented 17 February 2010 at 15:14

oh and I fixed an obsolete comment.

Log in or register to post comments

Comment #12

sun

German

Karlsruhe

CreditAttribution: sun commented 17 February 2010 at 15:28

Thanks! I will go ahead and try whether this patch fixes my concrete problem in contrib.

+++ modules/simpletest/tests/form_test.module
@@ -144,6 +144,11 @@ function form_test_validate_form($form, &$form_state) {
+  $form_state['cache'] = TRUE;
+  
   return $form;

Minor: Trailing white-space here.

Powered by Dreditor.