Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter()

This #builder_function weirdness will drive me insane one day

:-/.
Note, though, that those lines in node_form_submit_build_node() :

  unset($form_state['submit_handlers']);
  form_execute_handlers('submit', $form, $form_state);

were there in D6 (the function was used for the preview workflow), and were just left as is when Field API introduced the infamous #builder_function. I can't say I ever really understood their purpose...

re sun #10 - yeah sorry if I sounded defensive, I wasn't ;-).

Subscribing.

MYMODULE_node_form_submit() is not documented so why this code invoke it?

+  // This form uses button-level submit handlers only, which manually invoke
+  // node_form_submit_build_node(), which in turn executes any form-level submit
+  // handlers prior to invoking node_submit(). For programmatically defined node
+  // types, a TYPE_node_form_submit() handler is therefore expected to be
+  // invoked, if it exists.
+  $form['#validate'][] = 'node_form_validate';
+  $form['#submit'] = array();
+  if (function_exists($node->type . '_node_form_submit')) {
+    $form['#submit'][] = $node->type . '_node_form_submit';
+  }

@sun I just cares about node-types with names like 'image-gallery' and modules probably could have name TYPE but not define a node type so it lead to unpredictable results.

#20: drupal.base-form-id.20.patch queued for re-testing.

As per #17:

Note that #735800: node form triggers form level submit functions on button level submits, without validation also deals with node_form() and should go in first, as it likely simplifies this patch.

#735800: node form triggers form level submit functions on button level submits, without validation landed. Can we get a re-roll of #20?

#25: drupal.base-form-id.25.patch queued for re-testing.

What have I... I'm sorry. Delete this post.

tag

Shouldn't this be priority "Critical" since it is a beta blocker?

This looks like a nice code clean-up, but don't see why it should hold up beta. Removing tag.

Generally, a really nice clean-up + fix!

$type . '_comment_form'

I know we use that already for node forms, but this is a dumb way of prefixing. It should be comment_form_$type, only that way we can preserve name clashes. Ideally we should fix that for node forms too. mymodule_node_form is reserved for mymodule, not for the node module.

+  // node_form() uses button-level submit handlers for the main form actions,
+  // which invoke form-level submit handlers, if any are defined. Upon preview
+  // and final submission, we need to renumber poll choices.
+  $form['#submit'] = array('poll_node_form_submit');

Sounds good, but shouldn't we better use $form['#submit'][] instead? Perhaps someone managed to already set that.

Then, if we fix #validate and #submit to take the base form id into account, we also need to fix #theme.

This is a re-roll only. Trying to get a passing patch, then will review.

Does this have any dependencies it's waiting on, or can it be reviewed on its own at this point?

+1

Ok, I'll try to review this soon. I would be absolutely +1 for trying to do #798062: Rename TYPE_node_form to node_form_TYPE in D7, but I think that'll be a tough sell to webchick. Regardless, I think we should do comment_TYPE_form and not TYPE_comment_form. TYPE_node_form is wrong. We may have to live with it for one more Drupal version, but we should not feel pressured to propagate the wrongness.

Also, I just happened to find #317424: Dynamic form ids for hook_forms, and moved it to D8, but wanted to link to it from here, to hopefully get that one onto people's radar for when D8 work begins.

This is really, really nice! The following nits are pretty minor.

+++ includes/form.inc	1 Sep 2010 17:30:16 -0000
@@ -389,10 +389,12 @@ function drupal_build_form($form_id, &$f
-    drupal_theme_initialize();
-    $registry = theme_get_registry();
-    if (isset($registry[$form_id])) {
-      $form['#theme'] = $form_id;
+    // 'form' itself is a theme wrapper, not a theme function. Therefore, theme
+    // functions only have to care for rendering the inner form elements, not
+    // the form itself.
+    $form['#theme'] = array($form_id);
+    if (isset($form_state['build_info']['base_form_id'])) {
+      $form['#theme'][] = $form_state['build_info']['base_form_id'];
     }

This is the hunk responsible for the test failure. There are form ids for which there are no base form ids, that do not have a theme hook. Or, you can have form ids with a base form id, and neither one has a theme hook. We need to keep the call to theme_get_registry(), or else add a new theme function like theme_form_contents(), or theme_element(), whose implementation is return drupal_render_children($element) to use as a fallback.

+++ includes/theme.inc	1 Sep 2010 17:28:16 -0000
@@ -755,6 +755,14 @@ function theme($hook, $variables = array
+      // Iteratively strip everything after the last '__' delimiter, until an
+      // implementation is found.
+      while ($pos = strrpos($candidate, '__')) {
+        $candidate = substr($candidate, 0, $pos);
+        if (isset($hooks[$candidate])) {
+          break;
+        }
+      }

We don't really need this, do we? When theme() is passed an array, that array should be complete, so the caller can have full control over fallback order.

+++ modules/comment/comment.module	1 Sep 2010 17:28:16 -0000
@@ -1751,7 +1749,19 @@ function comment_get_display_page($cid, 
+function comment_forms() {
+  $forms = array();
+  foreach (node_type_get_types() as $type) {
+    $forms['comment_form__' . $type->type]['callback'] = 'comment_form';
+  }
+  return $forms;

comment_form__TYPE is clever for automatic theme hook fallback, except unnecessary if we set $form['#theme'] to an array, as we already are in this patch. While I like it for consistency with template file naming (i.e., comment-form--article.tpl.php being essentially the same concept as what already exists for node--article.tpl.php), I'm concerned about introducing function names like comment_form__article_validate() or hook_form_comment_form__article_alter(). Maybe in D8, we can standardize on a convention to end the entire function name with __SPECIFIER, but if in D7, we have suffixes like validate(), submit(), and alter(), then perhaps we should keep to single underscore? I lean slightly to comment_article_form(), since I think of 'form' as a kind of suffix (convention only; no magic meaning like validate, submit, and alter), and one that helps avoid name collision should an administrator decide to use 'validate', 'submit', or 'alter' as a node type name. I.e., is comment_form_validate() the form builder for the the comment form on a 'validate' content type, or is it the validate handler for the base form id? I think a form id of 'comment_validate_form' steers clear of collision danger, even if it's a little ugly. Having written that, I really want D8 to move closer to real OOP: I think we're pushing polymorphism via function name creativity to its breaking point.

+++ modules/system/system.api.php	1 Sep 2010 17:28:17 -0000
@@ -1354,6 +1354,40 @@ function hook_form_FORM_ID_alter(&$form,
+ * @param $form
+ *   Nested array of form elements that comprise the form.
+ * @param $form_state
+ *   A keyed array containing the current state of the form.
+ *
+ * @see hook_form_FORM_ID_alter()
+ * @see drupal_prepare_form()
+ */
+function hook_form_BASE_FORM_ID_alter(&$form, &$form_state) {

This can receive a $form_id too, so let's document that. While hook_form_FORM_ID_alter() receives $form_id as well, that's never useful so its okay to omit that from the hook definition, but for this one, I could see a use-case for wanting $form_id.

Powered by Dreditor.

+++ modules/poll/poll.module	1 Sep 2010 17:28:17 -0000
@@ -247,7 +247,13 @@ function poll_form($node, &$form_state) 
+  // node_form() uses button-level submit handlers for the main form actions,
+  // which invoke form-level submit handlers, if any are defined. Upon preview
+  // and final submission, we need to renumber poll choices.
+  $form['#submit'][] = 'poll_node_form_submit';

Do we need to introduce this BC break (i.e., a requirement for node modules to explicitly add their submit handler instead of having it be auto-discovered)? Can we instead have node_form() do it?

Yay, poll module, for once again providing a use-case for catching BC breaks :)

Powered by Dreditor.

Thanks. I agree with #68.3.

I'm still not sure about #68.5. Just to be clear, we're not talking about hooks, we're talking about FORM_ID_submit() and BASE_FORM_ID_submit(). These are auto-suggested by default for all forms with the former taking precedence over the latter, unless the form builder function explicitly sets its own #validate and #submit. Now, in HEAD, node_form() does not set $form['#submit'], and therefore, poll_node_form_submit gets auto-discovered because $form_id = 'poll_node_form'. This patch adds $form['#submit'] = array() to node_form(), because you want to stop form.inc from auto-setting it to node_form_submit should $form_id . '_submit' not be there. But, why not have it to do $form['#submit'] = function_exists($form_id . '_submit') ? array($form_id . '_submit') : array() instead? Wouldn't that be backwards compatible with HEAD, and remove the need for the poll.module hunk in this patch?

I agree that the cognitive disconnect involved in node_form_validate() being the form-level handler, node_form_submit() being a button-level handler, and TYPE_node_form_submit() being a form-level handler is a WTF. But it's what we have, because we didn't manage to remove this legacy WTF from D7. Do we really want to open that can of worms in this issue though?

Other than above, this looks ready to me. Another review from fago, and perhaps a test, would be nice.

Status change in #70 wasn't intentional. My suggestion in #70 is just a suggestion, and is open to debate.

This looks great. Summary:

1) Review this patch in context, rather than just reading it: most of the +/- signs are just big blocks of code being out-dented via removal of an if condition.

2) There is no BC break in here, except the one that is the purpose of the issue: the automatic calling of hook_form_BASE_FORM_ID_alter() between hook_form_alter() and hook_form_FORM_ID_alter(), and the fallback to BASE_FORM_ID_(submit|validate)() if FORM_ID_(submit|validate)() doesn't exist (and if the form doesn't explicitly set #validate/#submit). While this is a theoretical BC break for modules that implement hook_forms() without wanting this behavior, it is much more likely for this behavior to be desired than undesired. Furthermore, since we have not yet fixed #766146: Support multiple forms with same $form_id on the same page, and may not in D7, this patch allows modules that need multiples instances of the same base form to coexist on a page (e.g., multiple "add to cart" buttons within an e-commerce site) to be implemented much easier than the hacks currently being done in the d6 versions of those modules.

3) The per-type comment form control made possible by this patch is a great use-case to demonstrate the value of the patch, and is an excellent, non-bc-breaking addition to comment.module.

4) The non-bc-breaking replacement of an if statement for #node_edit_form, with a hook_node_form_alter() is a really nice cleanup.

subscribe

+++ includes/form.inc	4 Sep 2010 00:16:55 -0000
@@ -389,6 +389,9 @@ function drupal_build_form($form_id, &$f
   if (!isset($form['#theme'])) {
+    // theme_form() itself is in #theme_wrappers and not #theme. Therefore, the
+    // #theme function only has to care for rendering the inner form elements,
+    // not the form itself.
     drupal_theme_initialize();
     $registry = theme_get_registry();
     if (isset($registry[$form_id])) {

Oops. Sorry. I missed this in my #76 review. We need to add elseif(isset($registry[BASE_FORM_ID])) at the bottom of this. Otherwise, comment-form.tpl.php will no longer be used if it exists in the theme, and that's a regression relative to head. I suggest elseif rather than making #theme an array, since you already have $registry.

Powered by Dreditor.

thanks

Wow, great improvements! However I don't like the comment_form__TYPE naming thing - function names like comment_form__article_alter() are *really* ugly and a major WTF. From the above examples I like comment_article_form most too as the _form suffix is already common.

For that - I'm not sure about how to deal with the problem of possible conflicting module names with type names, but strictly speeking comment module has already claimed comment_*. Thus module comment_FOO already violates the namespace - so it is up to the module to avoid conflicts with the comment module. Anyway, avoiding that scenario is preferable.

So what about introducing another prefix to the node type for the form ids? We can stick with "comment_form" as base form id and use "comment_node_TYPE_form"?

So what about introducing another prefix to the node type for the form ids? We can stick with "comment_form" as base form id and use "comment_node_TYPE_form"?

+1. Setting to "needs review" rather than "needs work" in case sun wants to argue against it. But I like this because "comment_node_TYPE" is the bundle name as per comment_entity_info(). Also, I've been thinking that we will need a contrib module that allows comments to be attached to other entity types, not just nodes, so this leaves room for that.

This works for me if it works for fago.

Additionally, since for comments specifically, we have comment__node_TYPE and comment_wrapper__node_TYPE for theme hook names, we could have comment_form() set #theme to 'comment_form__node_TYPE' if that exists in the registry. So we can continue to follow the __ pattern for theme hook names, where that pattern is consistent, while keeping the FAPI functions 'alter', 'validate', 'submit', and the builder callback, all using single underscore.

Sorry, one more comment about this. Are we concerned about a site having one node type named "Foo" and another node type named "Foo Submit", such that a function comment_form_node_foo_submit() would run as both the submit handler for the comment form on a 'foo' node, and as the form builder function for the comment form on a 'foo_submit' node? This same problem exists with #79, but can be solved by fago's suggestion in #81 (though maybe #81 comes with its own risks).

@ a) comment_form_node_TYPE vs. b) comment_node_TYPE_form:

1. Resulting function names:

comment_form_node_article_validate()
hook_form_comment_form_node_article_alter()

vs

comment_node_article_form_validate()
hook_form_comment_node_article_form_alter()

I'd prefer the second as a _form suffix is already commonly used and makes it more clear that associated handlers/hooks deal about a form as it leads to suffixes like form_alter(), form_validate(), form_submit().

2. Possible collisions:

As noted for a) we have the possible collision of type names and builder function for TYPE + TYPE_(submit|validate) - which is rather weird as the user could trigger that just by configuration. For b) the only collision I can see is a possible module called "comment_node_TYPE" implementing the form comment_node_TYPE_form - but with the 'node' prefix I'd say we are on the safe side here.

Consequently I'd prefer comment_node_TYPE_form - thus always keeping _form at the end of the form id. I do not feel very strong about that, but I think it is the better way to go.

+++ modules/comment/comment.module	6 Sep 2010 23:55:36 -0000
@@ -1787,6 +1797,11 @@ function comment_form($form, &$form_stat
+  $form['#theme'] = 'comment_form__node_' . $node->type;

This is triggering watchdogs. For now, we need to wrap this in a theme registry check the way form.inc does it. Separately, I'll open an issue for us to register theme implementations of 'comment_form', 'node_form', and other core forms.

Re #88: I don't really care all that much either, and I see pros and cons each way, but I slightly agree with fago. What I don't like about fago's preference is that you lose the ability to grep for 'comment_form' and catch everything, but I agree with fago in thinking about 'form' as a kind of suffix by convention, and it does seem more protected from namespace collision. As long as we're setting #theme the way we are in comment_form(), then ultimately, I'm fine with either.

+++ modules/comment/comment.module	7 Sep 2010 16:01:44 -0000
@@ -1787,6 +1797,11 @@ function comment_form($form, &$form_stat
+  $form['#theme'] = array('comment_form__node_' . $node->type, 'comment_form');

Sorry. Been short on time. But this is 99% done, so let's finish it. This is still triggering a watchdog, so I don't think it should land as-is. I honestly don't know why theme() triggers watchdogs when a theme key isn't found, especially since drupal_render() is perfectly capable of defaulting to a reasonable implementation (i.e., drupal_render_children()) when theme() returns an empty string. I agree that getting consensus on that is for a separate issue, but my initial feeling is that it should not depend on whether #theme is a string or an array, but rather on whether theme() is called for a render array or not. Alternatively, perhaps allowing #theme to be an array whose last entry is NULL might be a way of indicating that hey, these are all just suggestions, don't panic if none of them are implemented.

Anyway, assuming we don't want to bog down this issue with changes to theme() or drupal_render() let's just add the registry check to comment_form(), or perhaps have drupal_build_form() remove $form['#theme'] if its entries are not implemented, since it already has the registry, so we can avoid polluting comment.module with a theme_get_registry() call. In any case, I think it's better to have some workaround like that until we fix theme() or drupal_render() than it is to try to convince Dries or webchick to commit a patch that is triggering unnecessary watchdogs.

Other than that, I think this patch is ready to go.

That watchdog stuff was added for DX, to avoid the situation where you call theme('fooo', $bar, $baz); where the theme function is actually called theme_foo() and get absolutely nothing back. See #412730: Theme system should optionally report when a theme key is not found.

Reviewed once more. Committed to CVS HEAD.

Thanks Dries! Now that all that remains is documentation, no longer "major" (sorry webchick, i can sense you cringing at that).

RAWR! ;)