Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info']

Patch for this is attached but only minimally tested. System and Field tests all pass. Due to issue #394464: Field (not instance) settings not saving on field configuration form, you need to set cardinality to -1 in the field_config table manually for the field you want to test this on.

my cvs client is not generating proper patch files, so i'm temporarily using a non-cvs diff program. trying to see if adding the cvs headers to the patch manually makes it apply.

with the status change this time.

Can't apply patch? WTF? If someone more experienced with patching on cvs can help take this patch file and convert it into something that the test-bot knows how to apply, I'd really appreciate it. Otherwise, when I get my cvs client to work properly, I'll try again.

With the status change this time.

re-rolled against field.form.inc v1.5

What is a "widget"? Drupal/FAPI doesn't have widgets. FAPI has form elements and instances of form elements are called form items.

Also, this line is utter evil and will cause imbalance in the universe etc.:

+  $output_js = isset($javascript['setting']) ? '<script type="text/javascript">jQuery.extend(Drupal.settings, ' . drupal_to_js(call_user_func_array('array_merge_recursive', $javascript['setting'])) . ');</script>' : '';

I've explained the reasons for that here: #372401: Merge only needed FileField Drupal.settings. I'd say this patch should not get into core until #360081: Pass Settings Locally to JS Behaviors gets in.

I added #399982: Simplify and standardize returning an AHAH response for discussing the line of utter evil.

@yched: please see if the ahah_response module is of any use to you regarding D6 CCK. Obviously, you wouldn't want to add it as a dependency for CCK, but perhaps including a "if (module_exists('ahah_response'))" section to the code would be nice. Also, if you want to just embed the contents of the ahah_response module directly into content.module, please feel free.

I know there's already been a half-dozen blockers in this issue, but before we do this I really think we should wait for #370537: Allow suppress of form errors (hack to make "more" buttons work properly). Right now the *entire* form gets validated when you click the "more" button (in poll.module for example), which isn't ideal. You don't want to be pestered that a title wasn't entered before you can ask for a second textfield.

i agree with comment #17. also, looks like #360081: Pass Settings Locally to JS Behaviors has landed (except for the documentation), so once #370537: Allow suppress of form errors (hack to make "more" buttons work properly) lands, I'll submit another patch.

Yay for the new AJAX framework in core! Is #370537: Allow suppress of form errors (hack to make "more" buttons work properly) still a dependency for us, or should we proceed with finishing this issue regardless of that one?

Yes #370537: Allow suppress of form errors (hack to make "more" buttons work properly) is still a dependency unless we want validation errors popping up (like "Title field is required") every time someone hits the "Add another item" button.

This still needs work. I'm eager to see what yched came up with, and what things look like once #370537: Allow suppress of form errors (hack to make "more" buttons work properly) lands. However, I wanted to start playing with the new AJAX framework and see what happens. With this patch, you can set the "body" field of the "page" content type to "unlimited", and it actually works (if you put in a title, see comment #21)! With a much simpler field_add_more_js()! But there's some strange stuff going on:

• The code that prevents a duplicate wrapper actually results in the inability to click "add another item" more than once, but it works with that code commented out. Seems like something in the new AJAX framework is doing something different with respect to wrappers.
• If you "add another item" without a title, causing a message to appear, but then fill in a title, and "add another item" again, the status message from the prior invocation doesn't get removed. I think this is another issue related to how wrappers work with the new AJAX framework.

I'm not sure when I'll have time to investigate this more thoroughly, so I wanted to share this, in case one of you wants to look into it in the meantime.

Oh, testing bot...

Is there any way we can improve the framework so that form controls *other* than submit can trigger the correct stuff and not require their own configured path and hand-rolled callback?

I was chatting with @merlinofchaos about this earlier today, and he was suggesting that even though the nasty HTML world doesn't give us a clicked_button on a select or anything, we could have the ajax.js stuff the data about which field triggered the call, and we'd be home free.

It would make AJAX replacement *so* much easier for everybody that uses various controls to trigger.

Does that fit within the scope of this issue?

We should move that discussion over here though, since the FileField in core issue is quite lengthy already and this problem is not really a problem of FileField.

I'll try to have a look at this later on.

I'm really interested in this too, as I'm working on an improvement to the generic AJAX callback, which blew to smithereens will all of these other AJAX items. #556438: AJAX/AHAH 'callback' support only works for 'submit' and 'button' elements - Should work for all triggering elements.

Is it true that I can work around this by moving all forms into the .module? Obviously that's not a real solution for Drupal, but it might help me to make forward progress, since my forms are only in a test module, and there's nothing to prevent them all going into the .module.

Please note that the broken AJAX callbacks not including the file of the original form is now tackled in #559526: All AJAX Behaviors Broken for Forms in .inc files.

Re-rolled, so I can actually test #559526: All AJAX Behaviors Broken for Forms in .inc files (which also questions this separation of issues a bit)

So, this patch should actually make it work.

However, needs review by Form API masters, because I fear that an empty $form_state['storage'] is supposed to mean (and not trigger) something. :/ ('storage' is normally empty for most forms)

This works for me.

Some tests might still need some updating. Let's see.

However, this patch will most likely fail b/c of #423992: Remove show_blocks page variable

Also removed those debugging changes.

Cool, only 2 fails in Upload functionality remaining. :)

This turns out to be a total clusterfuck.

Upload module tests are failing now, because #ajax sets a $form_state['storage'] that is non-empty, which triggers the following magic in drupal_build_form():

  // Most simple, single-step forms will be finished by this point --
  // drupal_process_form() usually redirects to another page (or to
  // a 'fresh' copy of the form) once processing is complete. If one
  // of the form's handlers has set $form_state['redirect'] to FALSE,
  // the form will simply be re-rendered with the values still in its
  // fields.
  //
  // If $form_state['storage'] or $form_state['rebuild'] has been set
  // and the form has been submitted, we know that we're in a complex
  // multi-part process of some sort and the form's workflow is NOT
  // complete. We need to construct a fresh copy of the form, passing
  // in the latest $form_state in addition to any other variables passed
  // into drupal_get_form().

  if ((!empty($form_state['storage']) || $form_state['rebuild']) && $form_state['submitted'] && !form_get_errors()) {
    $form = drupal_rebuild_form($form_id, $form_state);
  }

Let's see how far we get by removing the magic.

ok, summary:

- The 'add more' button is rendered on a form that may be contained in an include file of a module. When the 'add more' button is clicked, the form is submitted to 'system/ajax', which wants to rebuild the form, but fails to do so, because the form builder callback for the form resides in an not loaded include file.

- The solution sounds simple: When the form is initially built for the first time, we record the include file that is stored in the menu system. Before the form is tried to be rebuilt in the ajax callback, we retrieve that property from the stored (cached) form and auto-load the include file before any form building and processing happens.

- Using $form_state['storage']['include_file'], the solution was almost working. As visible in http://api.drupal.org/api/function/form_get_cache/7, only 'storage' is reset in $form_state when the form is being rebuilt from cache, so storing the include file in 'storage' looks like the only way when trying to store in $form_state. However, drupal_build_form() uses a non-empty $form_state['storage'] as condition to rebuild the form, because it assumes that some multi-step form handling is happening. This results in all forms being rebuilt after submission, i.e. when posting on node/add/story, you don't end up on the newly created node, but instead on node/add/story again. AJAX callbacks, however, worked with this approach.

- Using $form['#include_file'], the property was re-assigned/overwritten on subsequent AJAX requests with the (new) menu router information of the 'system/ajax' callback. This happens, because ajax_form_callback() invokes drupal_process_form() with the cached form, and afterwards drupal_rebuild_form() with a new $form_state. drupal_rebuild_form() invokes drupal_retrieve_form(), which returns an entirely new $form, which is afterwards used by drupal_rebuild_form() to cache it.

- An approach using $form (instead of $form_state) would have to live directly in form.inc (instead of ajax.inc), because the #process handler ajax_process_form() only gets a copy of $complete_form, so it cannot set #include_file at the root of the form (the #ajax property is only applied to input elements, not the form itself). I could have needed a passed-by-reference $complete_form in some contrib modules, but I'm not sure whether Form API maintainers would be ok with that. However, given the previous point, $form doesn't seem to be the proper place to store this permanent information.

Using both as the only, sane workaround?

More or less follows the logic for $form['#args'] and $form_state['args']. Those are equally copied and shuffled around upon building and rebuilding of forms.

Preventing the installer to invoke menu_get_item()...

@sun: While it's somewhat confusing to have this shuffling, and it probably indicates that FAPI can still be improved, at least this complexity is hidden within form.inc, and neither ajax.inc nor form constructors/builders need to worry about it, so I like it. One request: can you make the scalar include_file into an array include_files? Add the menu item inc as an entry to that array, but this way, allow form constructors/builders to add more entries into it. Let me know if you'd like me to work up a use-case, but it seems like a pretty small change and potentially useful feature.

So. Aside from making 'add more' work with #ajax, this patch fixes a couple other issues:

+++ includes/ajax.inc	26 Aug 2009 18:11:24 -0000
@@ -149,7 +149,7 @@ function ajax_render($commands = array()
-  $commands[] = ajax_command_error(empty($error) ? t('An error occurred while handling the request: The server received invalid input.') : $error);
+  $commands[] = ajax_command_alert(empty($error) ? t('An error occurred while handling the request: The server received invalid input.') : $error);

Was not renamed, when ajax_command_error() was renamed.

+++ includes/ajax.inc	26 Aug 2009 18:11:24 -0000
@@ -301,7 +301,7 @@ function ajax_process_form($element) {
-      'speed '   => empty($element['#ajax']['effect']) ? 'none' : $element['#ajax']['effect'],
+      'speed'    => empty($element['#ajax']['effect']) ? 'none' : $element['#ajax']['effect'],

Trailing space prevented this setting to work at all.

I will now clean up the comments and it would be nice get someone to mark this RTBC afterwards (so quicksketch can move forward with FileField in Core).

5 days to code freeze. Better review yourself.

Fixed comments.

Not sure about the list of includes... If it's a list, we'd need additional unique() handling and other cruft. Also, that doesn't make sense. We only do this #include_file shuffling, because we do not have the information in a form rebuild on a different system path. If your form needs additional includes, then your form would already need additional includes on its original path. Doesn't make sense.

plach requested to return/render the full $field_form in field_add_more_js(). Works equally.

I can confirm #56 unblocks file.module, which starts operating properly after applying.

After applying the patch, I tested using the "Add another item" with an unlimited text field and it worked very nicely. Couple of things before setting this to RTBC.........

+++ modules/field/field.module	26 Aug 2009 15:25:59 -0000
@@ -156,23 +156,6 @@ function field_init() {
 /**
- * Implement hook_menu().
- */
-function field_menu() {
-  $items = array();
-
-  // Callback for AHAH add more buttons.
-  $items['field/js_add_more'] = array(
-    'page callback' => 'field_add_more_js',
-    'access arguments' => array('access content'),
-    'type' => MENU_CALLBACK,
-    'file' => 'field.form.inc',
-  );
-
-  return $items;
-}

This is awesome, look at all those minus signs!

+++ includes/form.inc	26 Aug 2009 19:08:56 -0000
@@ -452,6 +475,15 @@ function drupal_retrieve_form($form_id, 
+  // Whenever this form is (re)built, restore the include file property from
+  // $form_state, if existent.
+  // @see drupal_build_form()
+  // @see form_get_cache()
+  if (!empty($form_state['include_file'])) {
+    $form['#include_file'] = $form_state['include_file'];
+  }

Should these be @see drupal_build_form(), form_get_cache()?

+++ modules/field/field.test	26 Aug 2009 15:25:59 -0000
@@ -1401,11 +1400,11 @@ class FieldFormTestCase extends FieldTes
-  function _fieldPostAhah($path, $edit, $submit, array $options = array(), array $headers = array()) {
+  function _fieldPostAhah($edit, $submit, array $options = array(), array $headers = array()) {
     // @TODO: the framework should make it possible to submit a form to a
     // different URL than its action or the current. For now, we can just force
     // it.
-    $this->additionalCurlOptions[CURLOPT_URL] = url($path, array('absolute' => TRUE));
+    $this->additionalCurlOptions[CURLOPT_URL] = url('system/ajax', array('absolute' => TRUE));

Where is the issue for this @TODO item? Seems we're using a new framework now, so is this TODO now taken care of?

Beer-o-mania starts in 5 days! Don't drink and patch.

Incorporated chx' and Rob Loach's feedback.

Indeed you did.

So, we still have the problem of #370537: Allow suppress of form errors (hack to make "more" buttons work properly) not being done, which means if this patch is committed first, then not filling in the title field of the node, and then clicking "add another item" results in a validation error, a bug that does not currently exist in HEAD. Are we willing to commit this anyway, and let that be that other issue's problem? Or should we separate the patch and get the fixes to form.inc and ajax.inc committed, but let the changes to field.module and field.form.inc wait until #370537: Allow suppress of form errors (hack to make "more" buttons work properly) is finished?

Here's a patch that converts include_file scalar to include_files array. If we like it, it probably needs comment changes, but before doing that, @sun, I'd like your thoughts on it. I hope by attaching it as a .txt file rather than as a .patch file, testbot skips over it, because I don't want this one to become official unless there's community support for that. This is just for communicating the idea.

Anyway, what this solves is that currently, the element process functions for CCK widgets need to be loaded for all page requests, because hook_field_widget() only runs on initial form build, not when the form is submitted and retrieved from cache. So, for example, number.module includes the functions:

• number_elements_process()
• number_float_validate()
• number_integer_validate()
• number_decimal_validate()

These functions are only needed when you're on an edit form that uses a number field. With this patch, you could move these functions to a 'number.widget.inc' file, and then add this to the top of number_field_widget():

function number_field_widget(&$form, &$form_state, $field, $instance, $items, $delta = 0) {
  $form_state['include_files'][] = drupal_get_path('module', 'number') . '/number.widget.inc';
  module_load_include('inc', 'number', 'number.widget');
  ...
}

This could potentially even be cleaned up to:

function number_field_widget(&$form, &$form_state, $field, $instance, $items, $delta = 0) {
  $form_state['include_files'][] = module_load_include('inc', 'number', 'number.widget');
  ...
}

if module_load_include() didn't return the file path with DRUPAL_ROOT prepended, but I digress.

Anyway, this is clearly a different issue than making the add more button work, but since we're introducing the capability for form.inc to include files after retrieving a cached form, why not just make it do so using an array, and get this other benefit too? Do we really need to deal with unique()? Can't we rely on require_once to just work?

D'oh. Yes, of course, we need array_unique(), since hook_field_widget() gets called for each instance, so the array can get big unnecessarily. Here's that change.

Testing bot doesn't test patches ending in .txt. Trying .patch instead.

Sorry, I read too fast. That was by design.

Webchick was asking why we need two systems (both $form['#include_file'] and $form_state['include_file']) to record this information. Here's the workflow breakdown:

- Original form is generated. $form['#include_file'] is added automatically.
- AJAX request is made, the cached form is pulled out. This cached form contains $form['#include_file'], which we immediately pull in.
- We copy $form['#include_file'] to $form_state['include_file'].
- During the AJAX request, the form is entirely rebuilt. The only reference we have to work with is $form_state['include_file'] during the rebuild, since the $form variable doesn't exist at all at the beginning of regenerating the form. So when the form is rebuilt, if $form_state['include_file'] is available, we don't don't call menu_get_item() and the original path is maintained.

That said, it does seem like both properties may be available in an unnecessary amount of places, not sure about that though, it just looks like they're redundant in an awful lot of places.

I fully support effulgentsia's change to "include_files" instead of "include_file". Let's continue with #64 on any further changes.

Sorry to be all flip-floppy, after re-reading #55 and talking with webchick, I agree that a single file should be fine. My thought is that if you're form_alter()'ing already, then you can pull in your needed include during the form_alter(). Since all hooks still need to be in module files, there will never be a situation where your hook_form_alter() wouldn't be called and give you the opportunity to include any additional files. The serious problem here is that the *original* form function can't be found.

Sooo... let's just ignore my suggestion and agree that #59 is RTBC, per sun and chx's comments.

So we discussed this with sun yesterday at length. There are many possible approaches -- an easy one is to store the include files in the form in a property and because the form is cached, when retrieving from cache it becomes possible to include the necessary file. However, the problem with this approach is that when rebuilding the form only form_state survives so we need to copy this property into form_state and reuse that. So it becomes the cycle: #include_file in $form -- $form is cached -- user AJAX submits -- $form is retrieved from cache -- include_file is copied into form_state -- $form is rebuilt -- include_file is copied from form_state to the new $form -- $form is cached...

Actually, I agree with sun in #55 that we don't really need an array of include files here. (oops, cross-posted with quicksketch) This is a review of #59.

Just a couple of questions, then I think this should be good to go. It has the thumbs-up from a form API maintainer, and removes the most god-awfully scary code I've ever seen in my entire life. CUPoC, indeed...

+++ includes/ajax.inc	26 Aug 2009 18:11:24 -0000
@@ -149,7 +149,7 @@ function ajax_render($commands = array()
-  $commands[] = ajax_command_error(empty($error) ? t('An error occurred while handling the request: The server received invalid input.') : $error);
+  $commands[] = ajax_command_alert(empty($error) ? t('An error occurred while handling the request: The server received invalid input.') : $error);

Let's make sure we file a follow-up issue for test coverage of this.

+++ includes/form.inc	26 Aug 2009 19:42:28 -0000
@@ -155,6 +155,22 @@ function drupal_build_form($form_id, &$f
+      $form['#include_file'] = '';
+      $form_state['include_file'] = '';

Let's get some more details here as to why we have to store this in both places. Seems very odd to me.

+++ modules/field/field.form.inc	26 Aug 2009 19:24:47 -0000
@@ -343,114 +340,27 @@ function field_add_more_submit($form, &$
-function field_add_more_js($bundle_name, $field_name) {

Holy scariest function I've ever seen in my life, Batman. I'm SO happy to see this awful code refactored.

+++ modules/field/field.form.inc	26 Aug 2009 19:24:47 -0000
@@ -343,114 +340,27 @@ function field_add_more_submit($form, &$
+  // Navigate to the right part of the form.
+  $form_path = $form['#fields'][$field_name]['form_path'];
   $field_form = $form;
   foreach ($form_path as $key) {
     $field_form = $field_form[$key];
   }

I'm not quite following how there are multiple form paths. If there are, should we call this property 'form_paths'?

5 days to code freeze. Better review yourself.

I'm fine with sticking to #59, especially to get this issue to move along (and by the way, awesome job sun, chx, and others who figured out this clever way of solving the problem in a way fully encapsulated within form.inc).

However, I disagree with quicksketch's reasoning that hook_form_alter() solves the use-case I described. The use-case I'm describing is a widget adds an element with a type that will need to be expanded during the form_builder() stage using the #process function specified in hook_elements(). Currently, that function needs to be in the .module file (or if separate, still included for every page request). While a widget module can implement a hook_form_alter() that scans the form for whether the widget is used and then include the file where that widget's element's #process function lives, that seems much clunkier to me than allowing the hook_widget() function to add to $form_state['include_files'].

@quicksketch: I'll trust your call on this one: does it make sense for me to open a new issue so this can be discussed more as a separate patch once this one gets committed, or should I drop it?

Sorry, cross-posted with webchick. Reverting to her status.

Let's get some more details here as to why we have to store this in both places. Seems very odd to me.

Even better, let's not store in both places unless it's necessary. This version doesn't set any properties unless they're needed. So $form_state['include_file'] is never set unless you're pulling a form from the cache and you'll need it during the rebuild.

I'm not quite following how there are multiple form paths. If there are, should we call this property 'form_paths'?

I think the variable name is correct, only perhaps "parents" would be more accurate. When field module is referring to the "path" it's the array of parents to the form element. So a singular term is correct for the word "path" but we usually use "parents" in such situations. My guess is that the original field patch didn't use #form_parents because it'd be very easily confused with #parents.

effulgentsia, ahh I see what you're saying now. Indeed a single include file would not provide the flexibility to add an additional #process property through hook_elements(), and then have that file pulled in on subsequent rebuilds.

So to illustrate say you had something like this:

// In mymodule.module file:
function mymodule_elements() {
  $elements = array();
  $elements['text_textfield'] = array('#process' => 'mymodule_expand'),
  return $elements;
}

// In mymodule.widget.inc:
function mymodule_expand($element, &$form_state, $form) {
  $form_state['include_files'][] = drupal_get_path('module', 'mymodule') . '/mymodule.widget.inc';
  // Do extra stuff to $element.
  return $element
}

So the idea is that this would make it so that "mymodule.widget.inc" would get pulled in during the AJAX request. However, from the patch that we had in #63, this would not actually be the case. The reason being that $form['#include_files'] is the variable that needs to be set (since it's the one put into the cache), not $form_state['include_files'] which will be discarded when the form is submitted (I think). Hence all the more reason for ditching duplicate values, because you just don't know which one is the important one. :P You can't even set $form['#include_files'] because #process functions get a *copy* of the form, not the original one. I think the multiple file handling will require more thought.

All told, I think we should just get this in, remove all this crufty code and save re-factoring for a followup.

Ok, great. Despite some of the confusion, I do agree that this is a really clever solution that hides the WTF code nice and safe with its other bretheren WTF code in form.inc, without passing the responsibility for tracking these paths to every author of every form that may or may not need to be AJAXified ever. So huge kudos for that.

I committed #72 to HEAD to keep things moving. Marking needs work for additional refactoring to support what effulgentsia outlines in #70.

0) Although I can understand the provided reasoning for hiding $form_state['include_files'], I declared that property in any case to safe developers from WTF nightmares, because a property sometimes exists and sometimes not. The other internal Form API properties on $form_state are equally always set (even when being NULL). See http://api.drupal.org/api/function/form_state_defaults/7 (and I now realize that I should have placed those default values in there ;)

1) As stated in #55, the #include_file property is only used when a form is rebuilt from cache. It is not taken into account on initial form build.

2) If a generic #include_files property would make sense, then it can only make sense at the form builder level. I.e. a property that can be recursively assigned to all form element types. form_builder() would collect all those values and load the include files for all builds of a form, including the initial build, and before any form element processing happens.

3) I tried to explain another idea to chx yesterday, which unfortunately didn't come through: Enhancing $form_state with 'build_info'. The new 'include_file' property is persistent build information data that is required to build a form. We have another property that works in the same manner, on which the current implementation is actually based: $form_state['args']. That info is equally copied around from $form_state to $form to $form_state when a form is rebuilt from cache.

4) Proof of concept attached.

This time for real.

Hrm... is OpenID actually maintained at all? That's D5 code. :-/

However, those changes actually highlight the confusion about $form['#args'] and $form_state['args'].

This one will pass.

Thoughts?

In an effort to bring this issue to closure:

• I created a separate issue for my request from #62: #561226: Forms (elements) need to able to specify include files to be loaded for building
• I added a comment to #551694: Standardize the return of status messages in AJAX callbacks for dealing with the validation status message bug from #22
• There's already an issue, #370537: Allow suppress of form errors (hack to make "more" buttons work properly), for not being told you need a title, when clicking "add another item"

I think this lets this issue be used for continuing on #79, and once that's resolved, be closed.

+1 from me on #79.

Tagging.

I think this would be a nice clean-up in the Form API. To express some possible issues in advance:

- $form_state['build_info'] could be abused to store anything persistently for a form that is cached. Not sure whether that is an issue.

- The stored data in $form_state['build_info'], as of now, maps 100% to the menu system (i.e. passed args to drupal_get_form()/drupal_build_form() and the menu router item include file). Using a key that refers to "menu" might be worth to consider. But then again, we might want to store other persistent build information later on.

@chx: Could you have another quick look at the new proposal? Your feedback is essential here :)

@ sun's patch in #79: I like it!
Storing a $form_state['build_info'] in the cache whenever the form is cached really is the cleanest solution here. Great! When working on several FAPI issues earlier in D7 development, I also touched how we handle the form callback arguments for cached forms, and this always felt a little ugly (we used to set $form['#args'] = $form_state['args'] just to get the arguments cached together with the form). Having $form_state['build_info'] being cached automatically is much much cleaner.

I just skimmed the code and didn't spot anything I don't like. Will try to actually test it later today. Sun, could you reroll to have a green bot?

HEAD was broken.

meh, seems like bot doesn't want to re-test

Looks good to me. Any takers on setting this to RTBC, or else posting your feedback?

Apparently, chx asked some questions in IRC about this patch some time ago - but I didn't had time to reply to them, and I don't log #drupal, so I no longer know what he asked. :(

Introducing a new tag for feature freeze: API clean-up.

Re-rolled. I'm still need to know chx's questions/objections to be able to answer or incorporate them... :-/

this one is quite ok.

If the idea is that anything in 'build_info' gets cached, shouldn't we call this index something like 'cached_data' or similar? 'build_info' is not very descriptive. At the very least we would need this very clearly commented somewhere in form.inc.

Also, chx, could you elaborate on what your concerns were and how/why this patch fixes them?

No, we already have that "anything in 'xyz' gets cached" in $form_state['storage']. However, a non-empty $form_state['storage'] automatically triggers a rebuild of the form, which is not intended in many cases. So it is currently impossible to cache any other build information for a form that simply wants or needs to be cached - without storage.

Or in other words: Form API currently allows all forms to be cached and completely rebuild them from cache in a subsequent request. Since we do not have any dedicated storage of information required to properly rebuild the form from cache (like arguments passed to the original form, or this new 'include_file' information to auto-load the include file the form is contained in) - *without* triggering a forced rebuild of the form (which happens when $form_state['storage'] is used), the idea is to split the cached $form_state into 'storage' and 'build_info'.

This allows us to remove some unorthodox variable copying from $form to $form_state to $form to $form_state we currently do to ensure that a form can be properly rebuilt from cache.

Re-rolled and reverting status. I think I explained the clean-up that is happening here.

Re-rolled for new code in CVS HEAD that uses $form_state['args'], which is moved into $form_state['build_info']['args'] here.

#583730: How many ways are there to cache a form? depends on this patch now.

Can we get the last patch tested?

It is green now - but let's wait for this one ;)

+++ includes/form.inc	21 Sep 2009 18:10:22 -0000
@@ -322,19 +318,19 @@ function drupal_rebuild_form($form_id, &
 function form_get_cache($form_build_id, &$form_state) {
   if ($cached = cache_get('form_' . $form_build_id, 'cache_form')) {
     $form = $cached->data;
...
       if ($cached = cache_get('storage_' . $form_build_id, 'cache_form')) {
...
+        // Re-populate $form_state for subsequent rebuilds.
+        $form_state['build_info'] = $cached->data['build_info'];
+        $form_state['storage'] = $cached->data['storage'];

We additionally want to rename the cache id prefix from 'storage_' to 'form_state_'.

This review is powered by Dreditor.

Better title. Yes, these could have been 3 issues... ;)

This looks good to me. I think it would be good to document the build_info vs form_state stuff a bit more explicitly because I think it would help people that are thinking about this (or reading the code). Thoughts on that?

Good call. Added the requested docs.

However, a non-empty $form_state['storage'] automatically triggers a rebuild of the form, which is not intended in many cases.

(channeling Nate) If that's the case, then why have two competing variables storing the same thing? Why not change the behaviour so that if you /want/ a rebuild, you can get it by explicitly setting $form_state['rebuild'] = TRUE?

@webchick/quicksketch: That's beyond my knowledge about $form_state['storage'] and apparently, it's undocumented why it contains the magic rebuild. I guess it's a built-in convenience feature, but I'm not sure.

But anyway, there is a subtle difference: $form_state['storage'] is supposed to be populated and used by the form builder/handlers, i.e. the implementations of Form API. Implementations are free to unset($form_state['storage']), because it is supposed to be their own, private storage.

What's currently missing is the counter-part: A private storage for internal build information for Form API itself, not the implementation. To also cache that in case the $form_state gets cached. This patch introduces that.

I'd love to continue these Form API clean-ups, because there are many more weird, wonky, and non-obvious things in there. But I suggest to do it in smaller steps. Actually, the question you are raising would make much more sense to discuss in #583730: How many ways are there to cache a form?, since we have 'storage', 'rebuild', AND 'cache', AND 'no_cache' in reality. :)

HEAD is broken

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

CNR for testbot.

We need this API change for #367006-18: [meta] Field attach API integration for entity forms is ungrokable. Yes, I know that sounds strange, but you can read the comment I linked to grok why.

+ I'd like to point out that this basically was RTBC as of #105. It was only HEAD being broken that downgraded it, but when HEAD got fixed, the #105 patch worked again.

General documentation followup on AJAX changes is moving to #610068: Document AJAX no-js and use-ajax. Many issues can be combined there.

Are there docs changes or features introduced in this patch that should be addressed?

@rfay: That's hard to tell. This issue basically consists of 3 (hence the title), while the 2 latter are regressions/fixes of the first, but only touch Form API internals. The first may be relevant for AJAX docs though.

I can't replicate the dblog test failure in #105. Perhaps a momentarily broken HEAD? Setting to RTBC and re-running bot.

Yes, I also want to re-raise my support for this patch.

We really need a way to be able to cache information in $form_state whenever the form is cached, but without forcing the form to be cached. Currently, only the latter is possible (with $form_state['storage']) while the first is not. This e.g. forces us to set an #args property on $form with the sole purpose of having it cached with the form (#args clearly does not belong into $form but in $form_state, and what we have currently is an ugly workaround around exactly the feature introduced by this patch).

And to solve #367006: [meta] Field attach API integration for entity forms is ungrokable at least somehow - I fear a proper solution for that issue won't be possible now that we're in API freeze - we need this, or we need to set properties on $form that belong into $form_state.

The only nitpick I have is that I'd call 'build_info' just 'build' for shortness and readability, but this is a matter of taste I guess and I don't care enough to remove RTBC. Let's get this sucker in and I'll try to submit a patch for 367006 (I got some more or less ready code lying around).

Ok, let's do this. It has the thumbs-up from chx as well as the folks with the most familiarity with AJAX/Field API. Docs look good.

Committed to HEAD.

Reading through this issue, at one point an optional #include_file key was available. This was dumped in favor of more automated handling via the menu system. While I really like the simplicity of what was committed, I have run into a situation that this does not solve.

If I am reading the code correctly, this patch solves the case for a form that is on a page whose page callback is drupal_get_form and who has 'file' and 'path' set for the menu router item.

What this does not solve is the case where you manually call drupal_get_form, such as loading the user password form into a jQuery UI Dialog, or say if panels wanted to put a create node form in a block. This means you cannot use system/ajax and a callback, you much create a special menu item that includes the appropriate file (user.pages.inc for example) and then calls ajax_form_callback itself. You would then need to make sure all ajax callbacks in the form are swapped in favor of your custom 'path'. Not a big deal on a one-off basis, but annoying because is not easily apparent that things won't work.

If I am an intermediate developer and I do module_load_include and then drupal_get_form, I might think that it will work, but if there is any ajax functionality in the form this will not be the case.

Manually invoking drupal_get_form() sounds like an special-case scenario, which we may have forgotten indeed.

Please copy a summary of that comment over to #561226: Forms (elements) need to able to specify include files to be loaded for building and bump that to critical.

Talking with carlos8f, hook_forms could become analogous to hook_theme (although this is going into dangerous registry-like thinking :)

Where hook_theme has 'path' and 'file', hook_forms could have 'path' and 'file'. Just before executing the form callback, drupal_retrieve_form could include the proper file if needed.

From looking at ajax_form_callback and plain ole' drupal_get_form, this should satisfy most use cases. It also removes the need for menu router items with a page callback of 'drupal_get_form' to have 'file' and 'path' if the form is in an include file. It also makes it easier for contrib developers to use forms that are in include files.

Pros:
- Simple
- Follows pattern set in hook_menu, and hook_theme
- Allows forms with ajax to be rendered where-ever they want and still work.

Cons:
- Burdens module developers with the responsibility to implement hook_forms if they use a lot of include files.
- Patch would have to touch every core module that put forms in include files.

Since this is all 'in theory', I want to implement this and support it in the user module so I can test and report back.

Question:
hook_forms supports a callback and a wrapper_callback (or no custom callback at all). In this case do we need 'callback_file', 'callback_path', 'callback_wrapper_file', callback_wrapper_path' and then if there is no custom callbacks do we use 'file' and 'path', or use 'callback_file' and 'callback_path'. Uggg.

That last comment is totally irrelevant for this issue. See #597280: Introduce form registry to fix various problems and increase security for the D8 way to move forward.