Forms (elements) need to able to specify include files to be loaded for building

subscribing

Not sure about this. Basically, it has the potential to bring back parts of the function auto-loading of the now removed registry. I.e. form builder/processing code that is rarely used, can safely live in an atomic include file.

meh.

This still does not make sense.

Man!

Why didn't you simply state that you want to put that info into hook_element_info() ? :-D

And whoever still thinks that there is no possible performance improvement in loading less code, even with opcode cache enabled,

make sure to have a look at http://drupal.org/project/js

If there was no performance gain in loading less code, then I surely would not have written that thingy...

I don't see a really compelling reason to commit this at this stage, no. I don't see anything that indicates how not having this property in D7 is a regression from previous Drupal versions.

If we had benchmarks that showed a tremendous improvement in RAM usage or something, maybe. Can you provide such benchmarks?

Arguably, we need something like this in drupal_render() because cached elements need a way to indicate code that should be loaded even when we have cached an element. It would be #attached_file just like we have #attached_css and so on.

Hm. But this code runs in form_builder() before any #type or form element processing happens.

If we need something similar to this for drupal_render(), then we better come up with a common property for both, and apply changes to both form_builder() and drupal_render() in this patch.

Bumping.

I had found and commented on http://drupal.org/node/367567#comment-2418442 before I found this.

[regarding the patch in #367567]

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.

The reason some type of inclusion mechanism is useful to include in core, is because we have this great new cTools inspired ajax functionality, but some weird cases where it will not work and the average site developer won't understand why. Sure you can write your own menu callback ala Drupal 6, but most will find the use of ['#ajax']['callback'] much easier because it loads the form from the cache, assembles form_state, and calls your function automagically. It feels like a big WTF waiting to happen if you form breaks just because you put it in an include file.

[Edit]
An example use case is the Dialog module that I am trying to contribute towards. We are adding support for user_login, user_register, and user_pass to be available via a jQuery UI Dialog. user_pass is defined in user.pages.inc. Our form_altered ajax functionality will not work in this case, unless we go deeper than we should have to and code a completely custom menu callback.
[/Edit]

well, the actual problem to be resolved is:

- You manually include/build/render a node_form on the site's front page.
- To do this, you will manually module_load_include('inc', 'node', 'node.pages') before drupal_get_form('foo_node_form').
- If that form contains an "Add more" AJAX-button...
- then a request to system/ajax will be triggered, but since the form was built on the front page, no $form_state['build_info']['file'] was cached, so form building within the AJAX request will #fail, because node.pages.inc is not loaded.

Does that count as bug report?

Exactly! :)

I added a comment to the other thread that probably belonged here. You could treat hook_forms like hook_theme and add 'path' and 'file' to its support. Since drupal_retrieve_form is called in both the drupal_get_form scenario and the system/ajax scenario, it should work.

If node_form() is an API function (IMO it is), then it should be moved to node.module like node_load() and node_save(). Having to manually include foo.pages.inc to access API functions sounds like a bug, modules should maintain their own pages/admin files, and other modules shouldn't have to know where they're kept. I I'm not sure it's worth an API change to hook_forms() to do this - what if you want to use a form that's not defined in hook_forms, then we're back to square one or defining every single form in hook_forms(), so I'd lean towards documenting this and moving obvious forms into .module.

Agreed on critical status though.

I wouldn't agree with that. The menu router validly defines node.pages.inc as include file, and therefore it's perfectly valid to have the form constructor and dependent/subsequent functions in the include file, too. Requiring form constructors to live in the .module file would mean that almost every $module.admin.inc and $module.pages.inc would have to go. A soft requirement of "this form of the module is popular, so keep it in the .module" won't work out, as you cannot predict, which forms eventually may be re-used/rendered/ajaxified by other modules.

By putting them in includes which are loaded only via the menu system we're saying "these are not APi functions, they're for my pages / admininistration tasks only", if that's not the case, then the whole idea of module.pages.inc and module.admin.inc is false.

I did a similar patch at #759344: support building forms defined in multiple includes. I re-rolled it for this issue. Instead of putting it in the $elements, it uses the form state where we already have the possibility to include single file. I think this is better, as one needs this functionality often unrelated to single elements - AJAX callbacks or validation/submit handler don't belong to any element at all.

Citing my original issue:

When building complex, modular forms its often the case that one needs to load multiple includes for building form. Examples I had for such forms are the Rules Action Edit forms in 6.x and 7.x or the Pageroute form in 6.x.

However modules have no opportunity to load their includes once the form is cached. Previously I solved that by adding an own #after_build callback that loads necessary includes specified in $form['#includes'] or such - an unnecessarily complex and weird workaround.

We now have $form_state['build_info']['file'] in core, which is a nice start but won't suffice as soon there is another include file needed - which is usually the case for modular forms. Attached is a simple patch that improves that to support an array of defined include files, thus any module can register its files in $form_state['build_info']['files'].

ad #13:
Do we really need this functionality for drupal_render() too? The #theme and theme wrapper support loading includes through the theme system nevertheless. But yes, still there is #pre and #post render, but are such callbacks besides the form API common practice? Don't know.

The attached patch, fixes the existing form API including possibility to work with multiple files. It doesn't introduce this feature for renderable elements in general though.

grml, forgot to specify --no-prefix.

Patch looks better although I still think this is solving the wrong problem. However this should come with tests.

>Patch looks better although I still think this is solving the wrong problem.
hm, I'm not sure it is. My patch doesn't solve the node_form() issue being in node.pages.inc at all. I agree it would be nice to have the form API being able to load that form, but it's not critical to me as you can easily load the include on your own.

What is critical to me is that it isn't possible at all to build a form in several includes, possible in a modular way cleanly as once the form structure is cached, the includes won't be loaded and the handlers living in the include aren't available. For that we need a way to tell the form API "load that include" and this is what may patch does.

>However this should come with tests.
Well if it wouldn't work a couple of other test would fail as they already rely on the form API to load the include specified in hook_menu. But I agree that this needs a dedicated test.

Code sprint tag

I've added a test case checking file inclusion of files specified in hook_menu() and also for custom specified files via $form_state.

seems like something went wrong with the test bot. I dont see a re-test link, so same patch here. Hopefully will get a result.

Has tests now, tests pass, patch makes sense, it's an API change but a limited one and the bug is caused by the limitation in the API.

#30: fapi.files_2.patch queued for re-testing.

#30: fapi.files_2.patch queued for re-testing.

#30: fapi.files_2.patch queued for re-testing.

#30: fapi.files_2.patch queued for re-testing.

Back to RTBC.

+++ modules/simpletest/tests/form_test.module
@@ -1082,3 +1099,20 @@ function form_test_two_instances() {
+function form_test_load_include_custom(&$form, &$form_state) {
+  $form['button'] = array(
+    '#type' => 'submit',
+    '#value' => t('Save'),
+    '#submit' => array('form_test_load_include_submit'),
+  );
+  // Specify the include file and enable form caching. That way the form is
+  // cached when it is submitted, but needs to find the specified submit handler
+  // in the include.
+  $form_state['build_info']['files'][] = array('module' => 'form_test', 'name' => 'form_test.file');
+  $form_state['cache'] = TRUE;
+  return $form;
+}

Am I the only one who thinks that looks ugly? To me, it would make more sense to optionally specify the file to be included along with the handler. For example:

+++ modules/simpletest/tests/form_test.module
@@ -1082,3 +1099,20 @@ function form_test_two_instances() {
+function form_test_load_include_custom(&$form, &$form_state) {
+  $form['button'] = array(
+    '#type' => 'submit',
+    '#value' => t('Save'),
+    '#submit' => array('handler' => 'form_test_load_include_submit', 'file' => array('module' => 'form_test', 'name' => 'form_test.file')),
+  );

I understand this is probably outside the

+1 on Dries's suggestion. This brings it more in-line with the menu API which has this same sort of "page split" functionality.

This issue is not limited to submit handlers.

I agree that the suggested syntax looks reasonable, but in practice I'd still prefer to state the include file once per form and not for each different handler. So all stuff belonging to a form can be easily put into a separate include file, add the file once during form construction (or alteration, etc.) and you are done. With the suggested approach you'd have to care about including the file for each specified handler, what is more cumbersome.

Also the current approach only changes a string to an array, I don't see a reason to change the API any more drastically than that, marking back to RTBC. A proper API change should be done in a separate issue IMO, and against D8.

#30: fapi.files_2.patch queued for re-testing.

The test-case form took $form by reference what can produce warnings, fixed that.

+++ modules/simpletest/tests/form.test
@@ -1158,3 +1158,35 @@ class FormsArbitraryRebuildTestCase extends DrupalWebTestCase {
+class FormsFileInclusionTestCase extends DrupalWebTestCase {

Missing PHPDoc for class.

+++ modules/simpletest/tests/form_test.file.inc
@@ -0,0 +1,40 @@
+/**
+ *  Ajax callback for the file inclusion via menu test. We don't need to return
+ *  anything as the messages are added automatically.
+ */
+function form_test_load_include_menu_ajax($form) {

Doubled leading spaces.

+++ modules/simpletest/tests/form_test.file.inc
@@ -0,0 +1,40 @@
\ No newline at end of file

This should never happen.

Powered by Dreditor.

thanks, fixed that.

thanks!

Just a reroll to update patch to current head.

Reviewed, and looks good after the discussion above. Committed to CVS HEAD.

Is this change documented somewhere?

Could someone please write an issue summary? It appears that the final result on this one is far from the initial statement. We went with $form_state['build_info']['files'] to handle the problem, right?

I'm going to assume that the minor doxy added above drupal_build_form() is all the documentation we need, and that this doesn't break BC?

I don't think that there is any code in the wild that uses and/or manually overrides the $form_state['build_info']['file(s)'] property...

DANG!

oh lord. $form_state['build_info'] was introduced and supposed to be a private area for things that Form API, and only Form API itself, needs to store, remember, reload, and process. Having the common documentation (in front of internals):
"Don't. Don't, don't touch this."

By suggesting developers to add their include files into $form_state['build_info']['files'], we broke that special key purpose.

Probably as simple as moving the 'files' key somewhere else, perhaps even onto the top-level.

oh lord. $form_state['build_info'] was introduced and supposed to be a private area for things that Form API, and only Form API itself, needs to store, remember, reload, and process. Having the common documentation (in front of internals):
"Don't. Don't, don't touch this."

Are you sure about that? I have not found any docs related to that.

Instead the only mention about 'build_info' I found was in the docs of drupal_build_form(), which says:

* The following parameters may be set in $form_state to affect how the form
* is rendered:

.. and then it lists 'build_info'. Indeed drupal_build_form() needs to have build_info (args) set before it is called - usually drupal_get_form() or form_get_cache() do that. But as to the docs any caller may do so.

*if* $form_state['build_info'] is intended to be visible or used by users, then it needs to be added to the form_api section near the top of form.inc. That's where we try to make sure everything is "clear" about $form_state.

ok, here is patch that explains 'build_info' there:

uhm, we already have documentation about build_info in http://api.drupal.org/api/function/drupal_build_form/7 ...?

Also, build_info was initially introduced in #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'], which not only contains in-depth discussion of this "internal" key, but also original considerations surrounding the 'files' property we've changed here.

@sun, I think the conversation here is that $form_state['build_info'] is not necessarily an internal key. In that case, it definitely needs to be included in the list of keys in the form_api section, which attempts to summarize all the keys for a form API user.

Still open is the question of whether we should have put the $form_state['build_info']['files'] at the top level, or somewhere that is not in a private key.

This patch does need an @link to drupal_build_form though, if it has more information.

ad #58:
Yes, we have build_info docs @drupal_build_form(). #56 explains why we want to add it at the top again.

Still open is the question of whether we should have put the $form_state['build_info']['files'] at the top level, or somewhere that is not in a private key.

$form_state['build_info']['files'] makes much sense too me, as the files are needed to build the form - thus they are build_info. If a module issued change to the form requires another include to be loaded, it makes much sense to me that it adds an include to build_info.

Also, build_info was initially introduced in #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'], which not only contains in-depth discussion of this "internal" key, but also original considerations surrounding the 'files' property we've changed here.

$form_state['build_info'] can't be really internal, as drupal_build_form() documents it as possible key to set.
@issue: Could you summarize it for us and tell us what the cause to not allow modules to append their files to $form_state['build_info']['files'] should be? Thanks.

Fiddling with include files to load in $form_state may become a common task in D7.

My comment from that other issue may explain it to some extent:

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.

The caching part no longer applies in the meantime, as we're caching almost everything in $form_state except of a few special keys listed in http://api.drupal.org/api/function/form_state_keys_no_cache/7. However, the storage part still applies:

$form_state['build_info'] currently contains 'args', which you shouldn't manipulate, or otherwise form processing will likely break badly. Very soon, it will hopefully also contain an 'instance' key, which allows Form API to #766146: Support multiple forms with same $form_id on the same page. There are potentially more of this kind of private properties and the original idea that we should try hard to maintain is that, normally, no one wants to touch or fiddle or manipulate any of those properties, and hence, $form_state['build_info'], ensure that this is considered "private".

I'd recommend to move $form_state['build_info']['files'] into $form_state['files']. Let's see whether this patch cuts it.

Also incorporated a revised version of the suggested Form API group docs. However, I have to amend that we're running into a documentation nightmare in form.inc currently. $form_state seems to get double-documented right now. I'm tempted to remove that snippet from this patch and defer re-organization of Form API documentation to a discussion and hopefully heated debate over in #859970: Cleanup form_api $form_state docs - keys in 2 places, missing some.

Establishing $form_state['build_info'] as holder for private stuff makes sense to me. However the above patch is incomplete, as still drupal_build_form() documents it to be set for the passed $form_state. A caller should never have to set private stuff though. Thus I think we either should 1) remove the documented support for handing in a manually created $form_state there or 2) provide a helper function that takes arguments and creates a form state.

>+ * - build_info: Do not touch; internal information stored by Form API to be
+ * able to build and rebuild the form:

What about reading from it? At least color module does that:

./modules/color/color.module:  if (isset($form_state['build_info']['args'][0]) && ($theme = $form_state['build_info']['args'][0]) && color_get_info($theme) && function_exists('gd_info')) {

Makes sense to me as part from a form_alter().

@moving to $form_state['files'].
$form_state['build_info']['files'] suits very well, as it is info that is needed to build the form. What about keeping it there and provide a helper function to add a file. This helper could also include the file in the first place, as FAPI will only include it for you once loaded from cache, thus this would be a nice little helper. Also the helper could only include it if its not in the form_state yet - useful for includes added by elements which might occur multiple times.

Very good suggestion - makes totally sense for me.

#63: drupal.form-state-files.63.patch queued for re-testing.

Oh, sry for letting this lie so long. Somehow this issue got out of my radar.

@patch:

>$type_or_filename
I'd consider overloaded function parameters bad style. I think just the module_load_include() parameter should be sufficient here, so I changed it to that. Also I've gone over the docs and improved them. Updated patch attached.

+++ includes/form.inc
@@ -363,7 +368,12 @@ function form_state_defaults() {
+    // @todo 'args' is usually set, so no other default 'build_info' keys are
+    //   appended via += form_state_defaults().

IIRC, I added this @todo...? Anyway, I guess it still applies, but not sure whether it makes sense to keep it here (probably worth to keep it) and/or in which other location we're actually facing the problem. If unsure, let's just keep it.

+++ includes/form.inc
@@ -538,6 +548,52 @@ function form_state_keys_no_cache() {
+ * @param $name
+ *   Optionally, specify the base file name (without the $type extension).

Should be "(optional) ". See http://drupal.org/node/1354 for details.

+++ includes/form.inc
@@ -538,6 +548,52 @@ function form_state_keys_no_cache() {
+  if (empty($name)) {

Should be !isset().

+++ includes/form.inc
@@ -538,6 +548,52 @@ function form_state_keys_no_cache() {
+  if (!isset($form_state['build_info']['files']["$module:$name.$type"])) {
+    $form_state['build_info']['files']["$module:$name.$type"] = array(
+      'type' => $type,
+      'module' => $module,
+      'name' => $name,
+    );
+    return module_load_include($type, $module, $name);
+  }
+  return FALSE;

It seems that our intention is to provide a meaningful return value TRUE or FALSE that indicates whether a file has been included. However, in that case, it would make sense to wrap the addition of the file to $form_state into another condition:

if ($return = module_load_include($type, $module, $name)) {
  ...
}
return $return;

That way, we don't try to include files for cached forms that couldn't be included for the uncached form already.

EDIT: Additionally, thinking through this, then the final return FALSE should actually be return TRUE -- because the include file has been successfully included...?

+++ includes/form.inc
@@ -641,6 +697,7 @@ function drupal_retrieve_form($form_id, &$form_state) {
+      // Do not use form_load_include() here, as the file is already loaded.
       $form_state['build_info']['files']['menu'] = $item['include_file'];

It's sad that we cannot use the same structure/syntax that form_load_include() uses. However, it would be good to add a comment that form_get_cache() also understands string values containing relative include file paths.

Powered by Dreditor.

>IIRC, I added this @todo...? Anyway, I guess it still applies, but not sure whether it makes sense to keep it here (probably worth to keep it) and/or in which other location we're actually facing the problem. If unsure, let's just keep it.

I can't think of a better place for it, so I've just kept it.

>Should be "(optional) ". See http://drupal.org/node/1354 for details.
I see. I just copied the comment from module_load_include() - thus I've also fixed it there.

>+ if (empty($name)) {
Again, that comes from module_load_include(). As its documentation (as copied) says something else, I've fixed it there too.

>However, in that case, it would make sense to wrap the addition of the file to $form_state into another condition:
Makes sense, done.

>EDIT: Additionally, thinking through this, then the final return FALSE should actually be return TRUE -- because the include file has been successfully included...?
Generally, yes. However module_load_include() does it different and I don't think there is any cause to change that right now. So let's better be consistent and do it the same way for form_load_include().

>It's sad that we cannot use the same structure/syntax that form_load_include() uses. However, it would be good to add a comment that form_get_cache() also understands string values containing relative include file paths.
Added that comment.

updated patch attached.

Thanks!

Hopefully, the code comments sufficiently describe why this API addition is needed. In short:

An already committed patch introduced $form_state['build_info']['files'] that contains a variable list of include files to load when a form is built from cache. However, the purpose of 'build_info' was to have a "private" storage space for Form API itself that should not be manually touched or altered by modules. The addition of the 'files' property in 'build_info' unintentionally broke that purpose, since modules have to add their files manually to $form_state['build_info']['files'].

To resolve this conflict and restore the originally intended private space, this patch introduces a form_load_include() function, which behaves identically to module_load_include() and takes over the required changes to $form_state['build_info']['files'].

Thanks, in addition to that form_load_include() is a nice helper, as the usual workflow right now is to include the file manually + write it into (a read-only space of) $form_state manually. form_load_include() now just takes care of everything.

This seems like a generally good idea, but it changes the recommended development practices for people building forms, and we can't do that this late in the cycle. Bumping to D8.

This is required for D7.

When modules use hook_hook_info() to place certain (form) hook implementations into include files, then those form hook implementations might not be loaded when other hook implementations are loaded.

Concrete example I'm running into:

1. File module implements Field API hooks in file.field.inc.
2. The file field widget is a Field API hook implementation: file_field_widget_form()
3. Since file fields implement AJAX, the form containing the file field is cached by Form API.
4. Therefore, file_field_widget_form() is not invoked again; the built form is reloaded from cache.
5. However, file_field_widget_form() defines a #process callback, file_field_widget_process(), which also lives in file.field.inc.
6. Thus, you get a fatal error upon submitting the form, because file.field.inc was not loaded and file_field_widget_process() is undefined.

To resolve this, file_field_widget_form() needs to implement:

  form_load_include('inc', 'file', 'file.field');

That actual fix is going to be part of #977052: Implement hook_hook_info() for Field API hooks

Yep, there are lots of situations for which developers need that. This is an important addition developers need in order to enable their forms to work with form caching, so I very much agree that we should add this to d7.

So yes, this is a change opposed to just using module_load_include(), but
a) the old way still works (as long as no one uses #ajax or the form cache gets activated somehow else)
b) it is as easy to adopt as changing the module_load_include() call to form_load_include() and your form inclusion problems are solved.
c) the "old way" of using module_load_include() in form builders never was a "recommended development practice" as it didn't work out. So there is no change in best practices, instead we finally establish a best practice for doing forms with builders split in multiple files.

#67: form_file.patch queued for re-testing.

Re-rolled against HEAD.

I agree that this is needed if we keep hook_hook_info(). Committed to CVS HEAD.

It is by design, but it appears many people are struggling with doing custom node form callbacks using node_add(). This is used outside of the Form API code, so form_load_include() can not be used (i.e. no $form_state). They tend to either replicate a lot of core logic or manually include the node.page.inc file via hook_init() or direct includes.

So posting this here to help others that want a clean simple workaround. (This was the only core issue that came up while searching for a solution.)

Define the menu callback that points to the node modules page

  $items['node/%node/add-subpage'] = array(
    'title' => 'Add sub-page',
    'page callback' => 'handbook_add_subpage',
    'page arguments' => array(1),
    'access callback' => 'handbook_subpage_create_access',
    'access arguments' => array(1),
    'description' => 'Creates a versioned sub-page that is related to the current book page.',
    'type' => MENU_VISIBLE_IN_BREADCRUMB,
    'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,
    'file' => 'node.pages.inc',
    'file path' => drupal_get_path('module', 'node')
  );

And place your actual page callback in your module file:

function handbook_add_subpage($page) {
  drupal_set_title(t('Create a new versioned sub-page for %page_title', array('%page_title' => $page->title)), PASS_THROUGH);
  return node_add('handbook_subpage');
}