node form triggers form level submit functions on button level submits, without validation

subscribing + tagging

I dunno, the presumption was that before the node is saved the whole node will be validated. #limit_validation_errors is not firing form level submit handlers.

function node_form_submit_build_node($form, &$form_state) {
  // Unset any button-level handlers, execute all the form-level submit
  // functions to process the form values into an updated node.
  unset($form_state['submit_handlers']);
  form_execute_handlers('submit', $form, $form_state);

Yeah, this is one of the ugliest parts of Drupal IMO.

This code is there because node_form defines its master submit handler (the function that gets called once you click on the "Save" button) as a button-level submit handler and not as a form level submit handler. At the same time, other modules (in core e.g. menu) that form_alter the node form add their #submit handlers on the form level. For example, menu.module adds a form level #submit handler in which it sorts out its values in $form_state['values'].

Now, for multistep forms, these submit handlers need to fire so that upon rebuild, the pseudo-$node-object includes the current menu values.

So this is actually intended behavior.

However, I now think that this code is so ugly that we have to fix it for D7. Just removing the form_execute_handlers line in node_form_submit_build_node will break menu.module for non-javscript multistep flows.

What we need is a proper hook on a form that fires on *all* submit events of the node form, for both button level and form level submit handlers. This is what the ugly code above emulates. To do it properly, I propose the following:

A) Remove the #submit property on the node form's save button and instead let it fire as a form-level submit.
B) Remove that form_execute_handlers line from node_form_submit_build_node()
C) Add a hook_node_submit($form, $form_state) that fires in node_form_submit_build_node().
D) Move the code in menu.module from a #submit handler into menu_node_submit().
E1) Define (by convention) that all submit handlers on entity forms need to call the #builder_function.
Or E2) make FAPI call the #builder_function prior to any submit. This could actually work out and make #builder_function an "official" FAPI property instead of a hidden field api thing (can also be a follow up patch - currently all submit handlers already call #builder_function).

This is a big step towards #367006: [meta] Field attach API integration for entity forms is ungrokable, then. But by now I really think that this step is unavoidable.

Here's a first patch that does what I outlined above. Tests will break, as locale and book modules still have to be updated in the same way as menu.

I likely won't have time to drive this home, so if anyone wants to take over #5 go for it. Apart from fixing locale and book my point E2 above should also be feasable for D7 (let FAPI call #builder_function automatically for all submits, regardless of whether it's a button or form level submit).

The other clean alternative would IMO be a function entity_submit that gets called by all entity form submit handlers (field api add more, preview, poll add more etc). This function would call the entity's submit function (or reach out to a new submit() method in the entity controller, which would be cleaner) and invoke the hooks. Either the entity_submit function or the entity's submit callback has to cast the form values into an object (that is ugly, but we cannot change that in D7 I think because that's not as easy as it seems).

this is not specific to poll, moving and retitling

To reformulate what this patch does. We used to call the node form's form level submit functions implicitly on all submits, including button level submits. We need this kind of functionality, because in the way the node form currently works, modules need code that runs on all submits to ensure working multistep. Therefore, we make this behavior explicit by introducing a hook_node_submit(), and also align non-field modules more closely to what field API does.

API docs for hook_node_submit() will look like this:

/**
 * This hook is called whenever a node form is submitted. 
 *
 * Modules may use this hook to extract their data from $form_state['values']
 * and set it on the $node object. 
 * Note that in multistep scenarios, this hook is called for all submits. This is needed
 * as the $node object has to be updated for each step so that when
 * rebuilding the form, the current values from the most recent submission are used.
 *
 * @param $node
 * @param $form
 * @param $form_state
 */
function hook_node_submit() {
}

This could count as an API change, as modules that form_altered #submit handlers onto the node form and relied upon these handlers being called for non-form-level submits would have to be updated.

subscribe

subscribing

I just committed #735808: fix multiple field value form to work with form API persistence, by the way.

Not sure where this puts #629252: field_attach_form() should make available all field translations on submit

oh, I knew there was a related patch somewhere. Just had to deal with this node_form_submit_build_node() trickery also over in #757154: Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter()

I guess we should get this patch in first. I like this patch, and especially the possibility to ditch #builder_function in a follow-up issue.

Needs a couple of clean-ups + thorough review though.

This is a re-roll of #20 only. I don't yet understand this patch enough to offer constructive review, but seems like an important cleanup that will make other important cleanups possible, so I hope those of you who understand it keep driving it forward. If I ever grok it, I'll try to help move it along as well.

In #23, sun mentioned that this needed an additional review -- not sure if he was volunteering? ;-)

Here is my first review:

+++ modules/field/field.form.inc	6 Apr 2010 04:38:17 -0000
@@ -363,17 +363,19 @@ function field_default_form_errors($enti
+    $form['#builder_function']($form, $form_state);

Should we add a @todo stating that we want to remove this?

+++ modules/node/node.pages.inc	6 Apr 2010 04:38:18 -0000
@@ -72,6 +72,7 @@ function node_add($type) {
+  // @todo for D8: Fix to don't pass that half baken $node object around.

This reads a bit weird -- not sure I can make sense of that comment. Given that this is a D8 todo, let's be a bit more rigorous.

+++ modules/node/node.pages.inc	6 Apr 2010 04:38:18 -0000
@@ -137,6 +135,7 @@ function node_form($form, &$form_state, 
+  // @todo Drop this for D8 so only $form_state[$entity_type] is used.

Same as above.

+++ modules/poll/poll.module	6 Apr 2010 04:38:18 -0000
@@ -355,15 +355,15 @@ function poll_form($node, &$form_state) 
+  // Disable the form API persistence for the choice form elements.
+  unset($form_state['input']['choice']);
+  $form_state['rebuild'] = TRUE;

It would be nice to explain why we disable the form API persistence.

Agreed on the comments, also one of the new _rebuild() functions has no phpdoc. I haven't reviewed the patch seriously but this plus the builder function is extremely annoying to deal with in practice so agreed with the critical status. CNW for the additional clarifications.

This is a cleanup of #29. I think it's ready to fly. Follow up issues will be needed (e.g., to fix the other entity forms, and to address some of Frando's goals in #4 that later comments decided to pull out of the scope of this issue), but I think this solves this particular issue.

+++ modules/node/node.pages.inc	4 May 2010 19:50:07 -0000
@@ -402,25 +411,38 @@ function node_form_submit($form, &$form_
-  // Unset any button-level handlers, execute all the form-level submit
-  // functions to process the form values into an updated node.
+  // Process the form values into an updated node by executing all the
+  // form-level submit handlers, which requires unsetting the button-level ones.
   unset($form_state['submit_handlers']);
   form_execute_handlers('submit', $form, $form_state);

This patch adds a comment explaining this ugliness.

+++ modules/node/node.pages.inc	4 May 2010 19:50:07 -0000
@@ -402,25 +411,38 @@ function node_form_submit($form, &$form_
+  foreach (module_implements('node_submit') as $module) {
+    $function = $module . '_node_submit';
+    $function($node, $form, $form_state);
+  }

New hook: this patch converts menu.module to use it.

+++ modules/node/node.api.php	4 May 2010 19:50:06 -0000
@@ -681,6 +681,27 @@ function hook_node_validate($node, $form
+function hook_node_submit($node, $form, $form_state) {
+  // @todo Need an example.
+}

This patch uses the converted menu.module as the example.

88 critical left. Go review some!

Thanks! I agree with all the changes between #31 and #34. Given the complexity of this issue, I think a review from sun, Frando, yched, or chx is in order before it's RTBC. Any takers?

Wow, great work effulgentsia and yched. I very much like where you took this from #5.

This patch is (once again) an important step towards cleaning up the whole entity/field form mess. Great comments in the patch, btw. That whole stuff really wasn't documented at all so far..

I couldn't find any issues when reading the patch. It really makes node forms quite a big saner. I like that for now, to keep the patch simple and not break BC, we still keep #builder_function while making it optional. And with the node form we show how entity forms can be done properly and simple. Core's other entity forms can likely quite easily be converted in a follow up patches. Afterwards, we can decide if we want to break BC and remove #builder_function or leave that to D8.

Anyway, this patch rocks and simplifies some things we've been trying to fix for quite some while now. While it doesn't do everything we likely want to do here, this patch really achieves most we still can do in D7. And it has plenty of comments and the important @todos for D8. RTBC!

I'm a little turned around by all the various approaches that have been tried here. Could someone please post a summary of what this patch does, and what API changes are required of contributed entity modules to conform?

Thanks, fago. Your explanation is absolutely correct. Since this patch is complex, let me also take a stab at explaining it. Perhaps with both of our explanations together, webchick will be able to make sense of it (and webchick: if you can't, it's our fault for not explaining it well enough).

We're trying to do two things: tighten security and fix some serious WTFs. But both of these are hard, especially when also trying to not break BC, so we're trying to do them in manageable steps.

For both security and sanity, we need to get rid of #builder_function (at least for core entities, but there's an open question about whether once we've gotten rid of it for core entities, whether we still need to leave it around for maintaining BC with contrib entities that are already using it). This patch does that just for nodes (i.e., it makes node_form_submit_build_node() no longer a #builder_function, so if you want to call it, you need to do so explicitly from a submit handler, as is done in node_form_submit() and node_form_build_preview()). This makes it possible for submit handlers of buttons that use #limit_validation_errors (e.g., field_add_more_submit()) to NOT call node_form_submit_build_node(), a key step in preventing non-validated values from being used in a risky way.

Additionally, this patch changes node_form_submit_build_node() to not set $form_state['rebuild'] to TRUE. I don't think that change is strictly necessary in order to achieve the main goals of this issue, but it's a change that makes sense. Setting $form_state['rebuild'] to TRUE belongs in a submit handler. node_form_submit_build_node() is not a submit handler: it's a helper function that may or may not be called from a submit handler. It should have no knowledge of whether the submit handler that calls it wants the form to be rebuilt. For example, after this patch, the two remaining submit handlers (in core) that call node_form_submit_build_node() are node_form_submit() and node_form_build_preview(). The former does not want the form to be rebuilt (unless some bizarre error occurred) and the latter does want the form to be rebuilt. Therefore, it makes no sense for node_form_submit_build_node() to weigh in on that decision.

So, what this requires of module developers is to:
1) not call node_form_submit_build_node() from a submit handler of a button that uses #limit_validation_errors: see the poll.module hunks
2) not use node_form_submit_build_node() as a submit handler (again, it's a helper function, not a full-fledged submit handler): see the book.module hunks
3) not assume that all entity forms have a #builder_function: see the field.form.inc hunks
4) not assume that node_form_submit_build_node() sets $form_state['rebuild'] to TRUE: see the node_form_submit() and node_form_build_preview() hunks

Additionally, module developers are encouraged to, but not required to, use the new hook_node_submit() for affecting how a $node object is built from $form_state['values'], rather than adding a #submit handler for doing that. See the menu.module hunks for an example. node_form_submit_build_node() continues to have some ugly code in it to maintain BC with modules that choose to not follow this recommendation.

Finally, module developers are encouraged to, but not required to, use the new $form_state['node'] instead of $form['#node'] for accessing node information from within a form building function. node_form() retains code that makes this optional.

what API changes are required of contributed entity modules to conform?

Nothing yet. This patch is just about nodes, not any other entity type. If we manage to fix the remaining core entity types in a similar way, then we'll need to decide whether to introduce any new requirements for contrib entities or whether to retain BC for them. We're expecting the other core entities to be easier to fix. Nodes carry a lot of historical baggage that we had to deal with in this patch.

Since the changes of this patch are helping to prevent pretty obscure security issues, we are better off with breaking BC; though that can happen in a separate follow-up, after fixing other core entities accordingly.

+++ modules/node/node.pages.inc	6 May 2010 15:20:07 -0000
@@ -141,7 +150,8 @@
-  $form['#node'] = $node;
+  // @todo Legacy support. Remove in Drupal 8.
+  $form['#node'] = $form_state['node'];

Does anything break in core if #node is removed? If nothing, then let's remove it?

+++ modules/node/node.pages.inc	6 May 2010 15:20:07 -0000
@@ -408,25 +417,42 @@
-  $node = node_submit((object) $form_state['values']);
 
+  // Execute all entity-level and field-level submit hooks.
+  $node = (object) ($form_state['values'] + (array) $form_state['node']);
+  $node = node_submit($node);

Why do we merge the node from $form_state here? Either needs docs or might be removed.

Powered by Dreditor.

Re #40.1: grep HEAD for $form['#node']. Used by book, comment, locale, menu, path, and translation modules. Therefore, likely also used by many contrib modules. It would be serious kitten killing to remove it at this stage of D7.

Re #40.2: @fago or @Frando: thoughts? I'm not too clear on this either.

#42: node_form.patch queued for re-testing.

Yes, I also have the impression that $form['#node'] is used very often in contrib. While it would be nice to have it cleaned up and removed, there is no cause of removing it that late it in the cycle. Well, it would remove some bloat from the node form, but in my opinion that doesn't satisfy such a drastic change this late in the cycle.

It might be good to sprinkle some "@todo: remove in D8" comments in core. That will remind us, but will also avoid that people use this kind of core code as an example of how to do things in contrib. Just a suggestion, not a requirement.

+++ modules/node/node.pages.inc
@@ -407,25 +416,45 @@ function node_form_submit($form, &$form_state) {
-  $node = node_submit((object) $form_state['values']);
 
+  // Update the node object by copying over the submitted form values. In order
+  // to adapt the values hook_node_submit() may be used.
+  $node = $form_state['node'];
+  foreach ($form_state['values'] as $key => $value) {
+    $node->$key = $value;
+  }

I'm still confused why we're doing this kind of merging rather than $node = (object) $form_state['values']; I can think of two possible reasons:
- we're intending to support partial node forms as per #367006-91: [meta] Field attach API integration for entity forms is ungrokable (item 6), but node_form_validate() seems to preclude that.
- we don't want to change the object pointer of $form_state['node'] (for example, so that this code also implicitly updates $form['#node'] as well).
If we're doing the second, we should document that. We should also document our position on the first.

81 critical left. Go review some!

+++ modules/node/node.pages.inc
@@ -89,14 +95,17 @@ function node_form_validate($form, &$form_state) {
+  // If the form is being rebuilt after a submit handler updated
+  // $form_state['node'], use that. Otherwise, initialize $form_state['node']
+  // with the node object passed as a build argument.
   if (isset($form_state['node'])) {
-    $node = (object) ($form_state['node'] + (array) $node);
+    $node = $form_state['node'];
   }
+  else {
+    $form_state['node'] = $node;
+  }
+

Re #50: I think this has something to do with it. This patch is shifting the merging from what's being done in HEAD in node_form() to with the patch being done in node_form_submit_build_node(). And since node_object_prepare() does add things to the node that aren't in $form_state['values'], we already do have some notion of a "partial node form", even if not a fully sane one.

79 critical left. Go review some!

subscribing. (noting issue summary in #39)

Based on above feedback, this patch:

• Changes all core usage of $form['#node'] to $form_state['node'] for node editing forms. If you grep core after applying the patch, you'll notice remaining usage of $form['#node'], but (hopefully) only for other forms. Changing those other forms would be an API change, and in any case, is a matter for a different issue. As with the prior patch, node_form() continues to make $form['#node'] available to not break contrib, and that line includes a // @todo Legacy support. Remove in Drupal 8. comment.
• Reworks node_form_validate(), node_validate(), node_form_submit_build_node(), and node_submit() to have some consistency and (hopefully) make a little more sense. Please review these changes carefully.

#54 failed because I changed node_form_submit_build_node() to not return the node any more, but forgot to update node_form_submit() and node_form_build_preview() to match. I decided to not have it return, because it's easier to understand the pipeline as being about updating $form_state['node']. Having the function also return the node masks that and makes the code less readable, IMO. Note, we're changing the API of node_form_submit_build_node() by taking away the $form_state['rebuild'] from it, so we might as well use the opportunity to make it as grokable in the process as we can. Following that logic, I decided to change its name to node_form_submit_update_node(). Again, the whole concept here is that in response to a form submission, we need to update $form_state['node'] with submitted data (after it's been validated, of course). I think "update" conveys the iterative nature of the process, especially in multi-step scenarios such as the "Preview" button, more clearly than "build". The down-side with "update" is if it conveys the wrong impression of saving something to the database (e.g., the "U" in CRUD, and hook_node_update()), so I'm open to other ideas, but let's pick a verb that we can use in our documentation as well as the function name: i.e., a verb we can use in the PHPDoc Summary Line of hook_node_submit().

Fixed typo.

Since I think we're mostly agreed on where we are so far with this, perhaps it's time to add in the fixes to all core entities. That way, this issue can be reviewed for its full impact. This patch will likely fail. For now, I just want to know where.

More abstraction, but same tests will likely fail.

This one might actually pass. If it does, I'll work on a summary.

One more bug fixed. All the failures from #63 now pass locally.

Ok, here's the summary of the issue as I understand it: it's long, because the issue is complex and has a lot of history. For those wanting even more background, #367006-91: [meta] Field attach API integration for entity forms is ungrokable contains an excellent explanation of the situation as it was a few months ago. Trying to fix the problem without breaking any APIs seemed hopeless. This patch does break some APIs, but attempts to contain the damage as much as possible.

When the Entity API went in to D7, two things were true:

• Multi-step forms were hard, because in all cases, the form builder callback (i.e., the function named the same as the form id or what hook_forms() returns for the form id) and all hook_form_alter() implementations that added additional elements to the form had to be multi-step aware, and set element #default_value properties correctly for where the user was in the form workflow. This requires having somewhere in $form_state to keep the information and from which to set the #default_value properties. In D6, that's what $form_state['storage'] is for. In D7, it can go anywhere in $form_state (other than reserved places like 'values' and 'input'), but it must go somewhere in $form_state. This is a general condition of all multi-step forms, not just entity form. But entity forms are a common kind of multi-step form because of the "preview" button and the "add another item" button if the entity is fieldable.
• We did not have #limit_validation_errors.

With the above being the case, the #builder_function property was introduced as a special property just for entity forms with the idea that each entity form could define a custom function for moving data from $form_state['values'] to some other part of $form_state. This function would then have to be called from one of the submit handlers of every button that required a rebuild. So, for example, field_add_more_submit(): the submit handler of the "add another item" button, but not just that button: every AJAX-enabled button you put on an entity form: yuk! But this function wasn't just about implementing multi-step persistence, it also attempted to generalize what we've learned over many Drupal versions as something we need to do for node forms: how to translate from $form_state['values'] to $node, so that after calling it, you can preview the node or save it to the database. The concept is simple: we need to have some kind of thing in $form_state that contains data about the entity across a multi-step workflow, that can be used for previewing the entity at any point in the workflow, and saving the entity once the form workflow is complete: hey, why not make that thing the entity itself!

But then we had to throw in #limit_validation_errors (and when I was working on that, I had no clue about this whole #builder_function business). And in that patch, we added warnings in the PHPDoc of form_set_error() to the effect of "don't do anything risky in a submit handler of a button that uses this". But calling #builder_function is exactly the kind of thing this warning tried to caution against, especially for node forms, but really, for any entity form. #builder_function and #limit_validation_errors are inherently incompatible (as the entity built by #builder_function must always be valid). This is no fault of anyone who worked on the Entity API, as #builder_function came first. It was my fault for not understanding #builder_function when I was working on #limit_validation_errors.

Fortunately, we have a solution. The first bullet point of this issue summary is no longer true: in D7, a form can be rebuilt without *always* moving *everything* from $form_state['values'] to $form_state[SOMETHING] to #default_value, because in D7, FAPI automatically retains user input of the submitted step during a rebuild. This means the buttons that use #limit_validation_errors don't have to (and shouldn't) call #builder_function.

Furthermore, by getting rid of the need to call #builder_function from every submit handler, we get rid of the need for the property at all. Entity forms have a specific known requirement, not an open-ended one. Their requirement is to map information from $form_state['values'] to an entity object (for previewing and saving). They also have the requirement of their form builder callback and hook_form_alter() implementations setting elements' #default_value from information in the entity, and therefore, having an entity from which to do that, whether the entity is one that is loaded from the database (i.e., the first step of the edit form of an already existent entity) or an in-progress one (i.e., within a multi-step workflow, such as after clicking "Preview"). The heavy lifting for both requirements can be centralized to a entity_form_initialize() and a entity_form_submit() function, which is what this patch does.

So, that's basically it. In summary, this patch:

• Introduces a $form_state['entity'] variable. This is always the variable that represents the entity being edited, regardless where in the form pipeline you are. It is a real entity object, not an array, and not simply an array of values cast to an object.
• Gets rid of #builder_function: no longer needed!
• Gets rid of the functions that used to be the values of #builder_function: all replaced with entity_form_submit(): leads to more consistency, less code duplication, and introduces no new API break, since the API of #builder_function needed to be broken anyway.
• Gets rid of $form_state['node'], $form_state['comment'], $form_state['term']: these were all semi-temporary arrays rather than real objects, and I doubt there's much contrib code using them (they were early attempts at solving the multi-step problems but from a mostly internal standpoint: they have several problems as public variables). They are replaced by $form_state['entity']. These were all new variables in D7, so no break for anyone coming from D6. For D7 contrib modules that are using these variables despite their deficiencies, it's a break. I suppose we can try to leave legacy support for them by making them array casts of $form_state['entity'], but I don't think it's worth it.
• Keeps $form['#node'] as an alias of / clone of $form_state['entity'] (the alias turns into a clone after form cache retrieval: see #52). Lots of contrib modules use it, and no need to break them. The patch, however, converts all of core to use the recommended $form_state['entity'].

While in some ways this is pretty significant API changing, it's actually not *that* bad. The most commonly used APIs by contrib modules that extend entites remain in tact. $form['#node'] remains, even though deprecated. The possibility to use a node form submit handler to do what is now possible with hook_entity_submit() remains, even though deprecated (the patch converts menu.module to the recommended implementation to serve as an example for how module authors *should* do it). The field attach API hooks remain unchanged. The validation hooks remain unchanged.

The breaks are essentially confined to submit handlers of buttons (so, if a contrib module implements its own "add another item" or "preview" button, it would need to be fixed) and for contrib modules that implement new entity types. For the latter, feedback from Damien and fago would be great, as they've done much work with contrib-defined entity types.

Once this issue is solved, we'll be able to solve #763376: Not validated form values appear in $form_state. Given that we were guilty of not following our own recommendation to not do anything risky in the submit handlers of #limit_validation_errors buttons, we shouldn't rely on it as a recommendation only: we should secure it at the FAPI level.

Wow. Thanks for the great summary (and the excellent work on the issue).

Thanks for the feedback, fago. This patch changes back to $form_state[$entity_type]. It re-introduces #builder_function to handle your point about us not knowing enough about the needs of contrib entities. I renamed entity_form_submit() to entity_form_submit_build_entity() and just made that the #builder_function value of comment and taxonomy_term forms and used by node_form_submit_build_node().

I renamed the new hook (what was hook_node_submit() in earlier patches and hook_entity_submit() in #65) to hook_entity_form_submit_build_entity().

I don't think we need to introduce hook_entity_form_alter() here. We already have hook_form_alter(). My main reason for wanting entity_form_initialize() and entity_form_submit_build_entity() is to avoid needless code duplication across the core entities.

This fixes a bug in #69 and has minor cosmetic code and comment improvements.

Re #71: I'm open to us changing node_submit() and comment_submit() from what they are in HEAD to something else, as you suggest. But is that an API change that's acceptable? We have a test that calls node_submit() directly. Are there contrib modules that call either of these functions directly, expecting them to do what they do now? This patch continues with the assumption that the API for these should not be changed. If that's the case, then entity_form_submit_build_entity() (or whatever we want to call it) must call these functions after it merges $form_state['values'] onto the entity, but before it calls field_attach_submit() and hook_entity_form_submit_build_entity() (or whatever we want to call that). That's the reason for 'submit callback': so we can re-use a single function for all the new stuff introduced by this issue, but still call the legacy node_submit() and comment_submit() functions without changing their API and at the correct time in the sequence.

entity_form_initialize() doesn't force entity forms to store their entity at a particular location within $form_state. If you don't want the default behavior, don't call entity_form_initialize(): do what you need to do yourself. And don't call entity_form_submit_build_entity(): use your own #builder_function. Presumably, if you don't have your entity maintained in the default part of $form_state, then it's probably because you're doing something fancy, and need to do your own logic anyway, both in the form builder and in #builder_function.

I dislike #builder_function - that just makes stuff complicated to grasp.

I agree. But it's what we got in HEAD. If we can get rid of it entirely as I tried to do in #65, then great! But you correctly pointed out the need for flexibility in contrib (and we even have a use-case for the flexibility with node forms). Do we really want to introduce a new name? Maybe, since the concept has evolved. But perhaps, it makes sense to leave it, at least for D7. I'm pretty flexible on this.

What about hook_$entity_type_submit() or hook_$entity_type_validate()?

This patch changes nothing about validation, and I think we should keep it that way, unless you're envisioning a critical issue needing it. webchick will want a justification for every API change in this patch, so let's keep it as minimal as possible. We already have a hook_node_validate() in HEAD, and are just expanding it here to receive $form_state: needed to not be disingenuous about saying $form_state['node'] is preferred to $form['#node'] (see the hunk in translation.module). We do not have across-the-board hook_ENTITY_TYPE_validate(). If we don't have across-the-board hook_ENTITY_TYPE_validate(), I'm skeptical about introducing an across-the-board hook_ENTITY_TYPE_submit(). That's what prompted me to come up with the hook_entity_form_submit_build_entity() as the name of the new hook: keeps it targeted as to what it's there for. But, I'm open to a better name for it.

Is any of this convincing? If not, want to post a patch with what you have in mind?

From #68:

The question remains whether we want to introduce hook_entity_submit() + hook_entity_validate() ... Basically this would allow a module to alter entity forms in general without making use of the field API. For sure that would be useful.

Despite what I said in #72, it's actually very easy to convince me to add new hooks, but webchick is increasingly strict about these things (fully appropriate, given her role on the project), which is why I've been resisting the urge, even up to #72. That said, if we can find sufficient justification, I'd be on board. I do think this is one of the areas where it's unfortunate we only went part way with the entity API and left as much as we did to be implemented only on an entity type by type basis. Example: does it really make sense for menu.module's ability to attach menu item control to the node editing form to be tightly coupled to nodes? Any reason for it to not add that same ability for all entities? I'm sure we can come up with lots of use-cases of things that should be able to extend entities that for one reason or another we don't want as a field. Alas, I fear it might be too late to solve this for D7 as completely as we would like to.

So far, I only skimmed the recent follow-ups, but I'd like to know: As long as custom/contrib entity module(s) can alter away the default behavior, I'd highly prefer a commonly expected default behavior for all (core) entities by default. Wouldn't that be possible with eff's patch?

i.e. I'd prefer to have a default way, mostly predefined by core (ideally nuking #builder_function), not only to won't fix support requests that ask for compatibility with module Blargh that will appear in my queues.

Will try hard to read + review more in-depth ASAP.

Ok, taking into account above feedback, this patch:

• Removes the addition of $form_state to hook_node_validate(). Trying to keep this patch focused, and this issue has nothing to do with making any changes to validation logic. It means translation_node_validate() needs to continue using $form['#node'] instead of $form_state['node'], but who cares. That can be dealt with in another issue (or not dealt with, since we're leaving $form['#node'] until D8 anyway).
• Removes #builder_function entirely. As fago points out in #76, since we no longer need it in field_add_more_submit() and similar AJAX-enabled buttons, its only hypothetical use would be for something like a "Next" button of a wizard designed to be entity-type agnostic. There's no need for us to add core support for such a thing. The module implementing that kind of wizard can introduce its own property for doing that: hopefully, it will come up with a better name than #builder_function.
• Introduces a new $form property: #entity_submit: This property is more or less what Damien suggested in #367006-102: [meta] Field attach API integration for entity forms is ungrokable as #builder_functions (i.e., an array replacement of #builder_function). This is in lieu of, and a unification of, what earlier patches in this issue attempted with hook_node_submit() and 'submit callback' (as an attempt to retain the legacy node_submit() and comment_submit() functions). The flaw with hook_node_submit() and 'submit callback' is that they implied that these were aspects of an entity type. But really, what we need is something that is an aspect of the form. A different form could require a different set of functions running to map the form values to the entity. Therefore, it makes a lot more sense as a form property than something tied to an entity type. I prefer #entity_submit over #builder_functions, because it serves a similar role to #submit. The main difference from #submit, is that #entity_submit runs regardless of which button triggers the submission, as long as the submission requires an updated entity (i.e., #entity_submit needs to run on "Save", "Preview", and when someone builds a wizard module, "Next"): see the code comment in this patch's node_form_submit_build_node().
• Comments entity_form_initialize() and entity_form_submit_build_entity() to make it clear that these are helper functions only, and do not have to be used if the entity form needs to implement something different than what these functions do.

From #76:

So either something with 'submit' or 'extract_form_values' ?

I'd be ok with changing #entity_submit to #entity_extract_form_values if that's preferred.

Fixed entity_form_submit_build_entity() to return the entity.

I have a thought I'd like feedback on before implementing. What about instead of introducing a new form property #entity_submit, we simply use hook_field_attach_submit()? It's a little odd, because the core uses of this are comment_submit(), node_submit(), and a replacement for menu_node_form_submit(), which are all examples that are specifically working with something about the entity that is not its fields. But in all cases, it is working with a fieldable entity, and I think it's reasonable to assume the same for contrib use-cases, as why would you ever create an entity type that you provide an editing form for and not make it fieldable? It allows us to not introduce a new hook (i.e., hook_entity_submit()) or form property (i.e., #entity_submit). But is it too conceptually weird? Are all the hook_field_attach_*() hooks really intended only for field-related activity? The problem is we do not have the same set of hooks to handle non-field form elements.

Cross-linking to #629252: field_attach_form() should make available all field translations on submit, as yched did in #21.

Re #80: No, using hook_field_attach_submit() for mapping non-field data would be wrong. To properly retain BC, we need #entity_submit, and we need it to run *before* field_attach_submit(). This is also consistent with other entity/field invocation order that we use (for example, hook_node_validate() runs before hook_field_attach_validate()). This patch fixes accordingly.

One may ask, if we're adding a #entity_submit, do we also need a #entity_validate? The answer is no. Because the regular #validate is just fine (at least until we revisit the whole validation workflow in D8). What's special about "submit" and only "submit" is the need to call it from multiple button clicks, where the buttons implement their own #submit, overriding the form-level ones.

So, now, I think this is good to go (well, I'm sure sun or fago will find something wrong with it, but hopefully, nothing major).

One more change: this patch replaces (node|entity)_form_submit_build_(node|entity)() with (node|entity)_form_submit_update_(node|entity)() (see #56). IMO, the verb change conveys the new concept better, and since we're breaking the API of node_form_submit_build_node (by not setting $form_state['rebuild']), it's actually better to change the function name to force contrib modules that are calling it to break in an obvious way rather than a subtle way.

Summary of API changes relative to HEAD:

• No more #builder_function. Any contrib module implementing a button-level submit handler that calls it needs to refactor their code to not use it. Such a contrib module was potentially insecure (if it was calling it from a button using #limit_validation_errors), so this is a necessary break to fix a critical issue. This patch includes hunks for field, poll, and book that provide examples for how to refactor. A contrib module implementing a custom entity type that sets #builder_function for its form ideally should remove the line of code that does that, but nothing breaks if it doesn't, as the property just has no meaning within core any more.
• No more node_form_submit_build_node(), comment_form_submit_build_comment(), and taxonomy_form_term_submit_builder() (i.e., the values of #builder_function for the corresponding core entity forms). Any contrib module that calls these functions directly will need to refactor to call node_form_submit_update_node() or entity_form_submit_update_entity(). In the process, they'll need to be aware that the new functions do not set $form_state['rebuild']. There shouldn't be many contrib modules calling these functions, though, as these were new functions in D7, and the only reason to call these functions is if you're adding a button to one of these entity forms that does something similar to "Save" or "Preview". This break is essentially the same break as getting rid of #builder_function.
• $form_state['node'] as an array is replaced with $form_state['node'] as an object. $form_state['comment'] as an array is replaced with $form_state['comment'] as an object. $form_state['term'] as an array is replaced with $form_state['taxonomy_term'] as an object (yes, the last one includes a name change as well as a type change). Contrib modules using these variables will need to adjust accordingly. As per #66, I don't expect too many such contrib modules, as these were new variables for D7, and were half-baked.

The above is it as far as breaks. Additionally, contrib modules are encouraged to, but not required to do the following:

• In form building and processing functions that extend the node form, access the node using $form_state['node'] rather than $form['#node'].
• If needing to extend the node form with a #submit handler that needs to run on all button clicks that require $form_state['node'] to be updated, switch to using #entity_submit instead of #submit (menu.module has an example).
• Contrib modules implementing custom entity types are encouraged to call entity_form_initialize() from their form builder function and entity_form_submit_update_entity() from the appropriate submit handlers, as is done for the core entity forms. Or, they can implement their own alternatives, using those helper functions as an example of what needs to be done. Contrib modules currently using the deprecated #builder_function technique will continue to work as they have been, which isn't very well.

Reviewers: as part of your review, please confirm that the above is an accurate and complete list of API changes.

I haven't had a chance yet to review the impact of #629252: field_attach_form() should make available all field translations on submit and how its introduction of $form_state['entity'] affects what we should do with #83.

I reviewed HEAD's entity and field attach APIs, and continue to think that it is better to introduce #entity_submit than hook_node_submit(), hook_entity_submit() and/or hook_ENTITY_TYPE_submit(), at least for D7. For D8, I think we can improve all our entity APIs for greater consistency and less code duplication across entity types, but I'm concerned about introducing any of the aforementioned hooks to D7. Here's why (sorry for the length, but this was my process for trying to grok the Entity API, and I haven't grokked it sufficiently yet to condense it well).

These are our hook_field_attach_*() hooks:

• presave, insert, update, delete, delete_revision, view_alter, preprocess_alter
• purge
• load
• form, validate
• submit

The first set are all invoked from an entity type specific function. So, for example, node_save() calls field_attach_presave('node', $node) and comment_save() calls field_attach_presave('comment', $comment). For most of these, hook_ENTITY_TYPE_*() is called. But not for all. For example, taxonomy_term_view() does not call drupal_alter('taxonomy_term_view', $build) the way that comment_view() calls drupal_alter('comment_view', $build). Was this an oversight? A performance optimization? For purposes of this discussion, it doesn't matter: what matters is that as long as we're delegating the decision to the entity type, we have to assume that these inconsistencies will exist. This is what I hope we can improve for D8, but I don't expect us to replace ENTITY_TYPE_save() and ENTITY_TYPE_view() functions with generic entity_save() and entity_view() functions in D7.

For 'purge', that's a field-specific concept, so there's no entity-level equivalent hook.

The 'load' hook is interesting. This is one that does go through a generic entity_load() function, though with the ability for a given entity type to override implementation using a custom controller class. If the default class's attachLoad() method isn't overridden and the 'load hook' within hook_entity_info() isn't overridden, then the generic implementation does invoke both hook_entity_load() and hook_ENTITY_TYPE_load(). This is the only case of an ENTITY_TYPE hook being invoked from a generic entity function rather than an entity-type-specific function, though as mentioned, either the controller class or the 'load hook' can be overridden to customize that.

All of the above have nothing to do with forms. They act on entities and fields the same way regardless of what form was used to edit the entity. A module that wants to implement an alternate editing form for a given entity type doesn't need to worry about any of those hooks.

field_attach_form() and field_attach_form_validate() (which calls field_attach_validate()) are called from forms. So, not just entity-type specific, but form-specific. So, comment_form() calls field_attach_form('comment', $comment, $form, $form_state);, but if a module wanted to implement ALTERNATE_COMMENT_form(), it would need to also call field_attach_form('comment', $comment, $form, $form_state);. In other words, for the 'form' operation, the Field Attach API is being used to attach a set of fields to a form, not to an entity. There is no corresponding hook_entity_form() or hook_ENTITY_TYPE_form() because we have hook_form_alter() and hook_form_FORM_ID_alter() instead (though I can see some benefit to adding hook_entity_form() and hook_ENTITY_TYPE_form() to D8). Similarly, field_attach_form_validate() is called from a FAPI #validate function. An alternate form for the same entity type may want an alternate #validate function, and that alternate #validate function would need to call field_attach_form_validate() where appropriate. We do not have hook_entity_validate() or hook_ENTITY_TYPE_validate(), because hook_form_alter() can customize #validate, so for whatever reason, we chose to let D7 code rely on standard FAPI rather than introducing Entity API hooks for this. As you said, it's rare for a button to specify a custom #validate, but if it does, then it's probably because it wants custom validation, and in this case, it would need to handle how it wants to integrate all of the form validation, including the field widgets. Note, we do have hook_node_validate(), but I think only for legacy reasons. We do not have hook_comment_validate() or hook_taxonomy_term_validate().

That brings us to the last one, 'submit', and the one this issue needs to address. The problem here is that we have something that FAPI alone can't handle properly (different buttons with different submit handlers, several (but not all) of which require $form_state['values'] to be mapped to an entity object). Ideally, we should solve this with a generic helper function, entity_form_submit_update_entity(), as many entity types will have this requirement. Advanced cases can implement an alternate function, but the generic one should be able to do the job most of the time, so we need the generic function to call either a well-named hook or a set of functions in a $form property. If we only needed this feature for nodes, then hook_node_submit() would make total sense, as we already have hook_node_validate(), but we need this feature for all entity types. Having a generic entity function call hook_ENTITY_TYPE_submit() would be introducing something only paralleled by entity_load(), but entity_load() does it via a controller class, and I don't think we want to introduce that level of complexity to solving this issue. If we do want to do it as a hook, then at the least we should introduce a 'submit hook' key to hook_entity_info() to fully parallel how 'load' is handled. On the other hand, introducing a $form property like #entity_submit seems fully consistent with how we're handling validation via #validate.

Thoughts? As I said, I'm still learning the Entity API, and am open to alternate ideas if I can fit them into some kind of conceptual understanding that makes sense.

Here's an approach I really like, and hope fago and sun do too. This adds the concept of #pre_submit to FAPI. It makes sense really: the concept of handlers needing to translate $form_state['values'] to something persistent in $form_state, and decoupling that from what a specific submit handler then does with the updated $form_state extends beyond entity forms, so I think this is a logical and valuable addition to FAPI. This then adds a entity_form() function instead of what in prior patches was entity_form_initialize(): using a similar pattern established by user_account_form() and system_settings_form() (the pattern of passing $form and $form_state to a helper function so that common needs across a class of forms can be centralized). It re-adds hook_node_submit() for nodes only. This creates consistency with hook_node_validate() existing for nodes only. Other entity types may choose whether or not to add hook_ENTITY_TYPE_(validate|submit)(), but ideally they should either add neither or both. These are a convenience only. A module can use #validate and #pre_submit to achieve the same thing for entity types that do not invoke the corresponding hooks. Finally, it changes what in earlier patches was $form_state[ENTITY_TYPE] to $form_state['entity'], since that's what #629252: field_attach_form() should make available all field translations on submit established. It provides $form['#' . ENTITY_TYPE] as an alias for it, since that helps code readability within entity-type-specific form processing functions and is what people are already used to with $form['#node']. It ensures object identity between $form['#TYPE'] and $form_state['entity'] after form cache retrieval by adding a #process function.

+++ includes/common.inc	25 May 2010 19:07:14 -0000
@@ -6629,6 +6629,113 @@ function entity_invoke($op, $entity_type
+  // Map all form values to entity properties. The entity may contain properties
+  // not being edited by the form, and these must not be wiped.
+  foreach ($form_state['values'] as $key => $value) {
+    $form_state['entity']->$key = $value;
+  }

Hm. This isn't so good at handling translatable fields: yay for committing #629252: field_attach_form() should make available all field translations on submit which has the necessary test for this! I added a workaround to make the test pass (no worse a workaround than what is in HEAD), but an @todo to the patch, because it really does expose a flaw that would be good to solve correctly. Any ideas on how to do that?

80 critical left. Go review some!

This patch includes an entity_form_pre_submit() implementation that solves #88 nicely, I think. And in the process reverts field_default_submit() to the nice simple function it was prior to #629252: field_attach_form() should make available all field translations on submit.

re #88 : well, this makes makes the actual extraction of form values into entity field values be performed by both entity_form_pre_submit() and field_default_extract_form_values() (invoked in field_attach_form_validate() and field_attach_submit()).
Also, doesn't this make this comment a bit off ?
// Allow fields to customize how their form values map to the entity.

Is it workable to have entity_form_pre_submit() do something like :

foreach ($form_state['values'] as $key => $value) {
  if ($key is NOT a Field instance) {
    merge into $entity;
  }
}

and leave the merging of field values to a field_default_X() op through f_a_submit() ?

minor : the common var name for an array of instances is $instances rather than $field_instances - but well, that's usually within Field API code, where the context is clear, so YMMV.

Implements #90. Thanks!

I fear reaching consensus on some of these points is proving tricky.

Here's a way scaled back patch. All it does is:

• removes calling #builder_function from buttons that use #limit_validation_errors
• fixes the form constructors to properly set/use $form_state[TYPE]
• fixes the *_submit_build_* functions to properly update $form_state[TYPE] (a single helper function, entity_extract_non_field_form_values() is added to assist with this)
• adds hook_node_submit() to give developers a path to transitioning away from using form-level #submit functions in a kludgy way
• fixes the handlers for the "save" and "preview" buttons to call #builder_function instead of a hard-coded function name. This allows module developers to swap in a different builder function without having to also override the submit handlers. For example, one might want to hook_form_alter() the comment form in a way that requires the builder function to do something different than what comment_form_submit_build_comment() does. It should be possible to do that without also duplicating all of comment_form_submit().

I think a lot of the other ideas that have been explored here are great seeds for D8, and maybe some of them can even be follow-up issues for D7. For example:

• I agree with fago that adding $form_state to hook_node_validate() should be on the table for D7.
• It might be worth opening a separate issue arguing for why it's ok to change the *_submit_build_* functions to not set $form_state['rebuild'] (I agree it makes much more sense that way, but is that sufficient at this point in D7?).
• I like the idea of a form_execute_default_handler() function, and also the idea of a #pre_submit phase.
• ...

But lets try to bring this issue to a close by targeting it to what is absolutely necessary.

+++ modules/comment/comment.module	28 May 2010 01:18:44 -0000
@@ -2067,48 +2078,59 @@ function comment_form_validate($form, &$
+  comment_submit($comment);
   field_attach_submit('comment', $comment, $form, $form_state);

Changed the comment, node, and taxonomy builder functions to call f_a_submit() before other submit functions/hooks. This is necessary because HEAD does a full $form_state['values'] to entity dump before starting the rest of the submit_build logic. But with this patch, we don't do that, as per #90. So, calling f_a_submit() sooner is more similar to how HEAD works. Fields are supposed to be encapsulated (not be tightly coupled to non-field entity data), so there should be no unwanted side effect from calling f_a_submit() sooner.

81 critical left. Go review some!

Thanks, fago, for your reviews and patience while I stumbled through properly understanding the problem-space. I agree with the changes described in #96 and a cursory look at the git history makes sense. I'll try to do a full review of the entire patch tomorrow, just to try to catch any remaining details, but I think you and I reached agreement on everything significant, so all that remains is just re-checking the details.

Upon closer examination, #98 continues to look great to me. Minor nits:

+++ modules/node/node.pages.inc
@@ -398,7 +403,6 @@ function node_form_submit($form, &$form_state) {
   if ($node->nid) {
-    unset($form_state['rebuild']);

Do we need to set 'rebuild' to TRUE in the corresponding "else" block?

+++ modules/node/node.pages.inc
@@ -413,19 +417,31 @@ function node_form_submit($form, &$form_state) {
-  // Unset any button-level handlers, execute all the form-level submit
-  // functions to process the form values into an updated node.
+  // @todo Improve this in Drupal 8. Module authors are encouraged to use
+  //   hook_node_submit().

I agree with you that we shouldn't discount the possibility that a button-level submit handler triggering form-level submit functions is potentially a useful pattern we may want to formalize and take advantage of. But for this specific use-case, we're encouraging people to use hook_node_submit(). So, how about something like this comment:

@todo Legacy support for modules that extend the node form with form-level submit handlers that adjust $form_state['values'] prior to those values being used to update the entity. Module authors are encouraged to instead adjust the node directly within a hook_node_submit() implementation. For Drupal 8, evaluate whether the pattern of triggering form-level submit handlers during button-level submit processing is worth supporting properly, and if so, add a Form API function for doing so.

+++ modules/taxonomy/taxonomy.admin.inc
@@ -833,21 +849,17 @@ function taxonomy_form_term_submit($form, &$form_state) {
-  $form_state['rebuild'] = TRUE;

In this case, should we remove the code that sets it to FALSE at the end of taxonomy_form_term_submit()?

+++ modules/user/user.pages.inc
@@ -278,22 +286,25 @@ function user_profile_form($form, &$form_state, $account, $category = 'account')
-  $edit = (object) $form_state['values'];
-  field_attach_submit('user', $edit, $form, $form_state);
-  $edit = (array) $edit;
+  entity_extract_non_field_form_values('user', $account, $form, $form_state);
+  field_attach_submit('user', $account, $form, $form_state);
+
+  $edit = (array) $account;
+  // Care for the password to be only updated, if the form contains it. Else
+  // 'pass' contains the hashed current password, so we must not save that.
+  $edit['pass'] = isset($form_state['values']['pass']) ? $form_state['values']['pass'] : NULL;
 
   user_save($account, $edit, $category);

So at the end of this process, we're setting $edit to an array containing all properties of $account, even those that weren't edited by the form. We're making a special exception for 'pass' only. Is this ok? Can we have a comment explaining this? It seems weird to me for the variable to be named $edit, but for it to contain things that weren't edited, but given that HEAD is a bit odd anyway in how it handles the entity / form values relationship, this is probably the right code.

79 critical left. Go review some!

However, with user_save() converting $edit back to a pseudo $entity for calling the field API attacher, I fear that this pseudo object might not contain all field values. E.g. consider a field doesn't show a form in a special case, then it's value wouldn't be in $edit and thus an empty value would be inserted for it?

I'm not so sure. field_attach_update() clearly says: // Leave the field untouched if $entity comes with no $field_name property,. But I also don't see harm in doing it the way the patch does it. Given that we're fixing the call to field_attach_submit() to use a real entity rather than pseudo entity, I don't see a clear answer for which way is more compatible with HEAD. So I think we just need to pick one, and I'm okay with #101's approach.

+++ modules/user/user.pages.inc
@@ -278,22 +286,28 @@ function user_profile_form($form, &$form_state, $account, $category = 'account')
+  // Put all account properties in $edit regardless they were actually edited,
+  // as $edit will be used for invoking field API attachers. This ensures that
+  // fields that did not appear in the form don't loose their values.
+  $edit = (array) $account;
+  // Care for the password to be only updated, if the form contains it. Else
+  // 'pass' contains the hashed current password, so we must not save that.
+  $edit['pass'] = isset($form_state['values']['pass']) ? $form_state['values']['pass'] : NULL;
 
   user_save($account, $edit, $category);

How about merging into one comment:

user_save() requires the $account entity to determine the account to update or whether to insert a new one, and a $edit array of keys and values to update/insert. Here, we invoke user_save() with a $edit array containing all of the data in $account, regardless of whether it was part of the form, except for the password, which we must strip out if it wasn't part of the form, since otherwise, user_save() would re-hash an already hashed value.

78 critical left. Go review some!

@sun, @Frando: this would be an excellent time for one or both of you to review the patch as well. Please include your thoughts on #101/#102 as part of your review. Thanks! I think we're very close to this being ready!

#104 looks commit-worthy to me. But it really should have at least one more reviewer before being RTBC. Earlier summaries are on #66 and #83. Here's an updated summary of what #104 does.

Removes calling #builder_function from buttons that use #limit_validation_errors or otherwise are "local" buttons that can simply update $form_state without invoking the full #builder_function processing:
- book.module
- field.form.inc
- form_test.module
- poll.module

Fixes entity form workflows such that:

• $form_state['node|comment|user|term|vocabulary'] always contains the up-to-date entity object being edited. This is a change from HEAD where these variables contain arrays instead, but as per #83, there probably aren't many contrib modules using these variables.
• $form_state['rebuild'] is set in the #submit handler, not in the #builder_function.
• The "Save" and "Preview" submit handlers call #builder_function instead of a hard-coded *_submit_build_*() function, decoupling the process of updating the entity in $form_state from what is then done with that entity, allowing a hook_form_alter() to swap in a different #builder_function without having to override the "save" and "preview" submit handlers.

- comment.module
- field_test.entity.inc
- node.pages.inc
- taxonomy.admin.inc
- user.pages.inc

Rolls back some of #629252: field_attach_form() should make available all field translations on submit, as the fixed workflow no longer requires those hunks to achieve what that issue needed. Note that the tests from that issue remain and pass.
- field.attach.inc
- field.default.inc

Adds helper functions entity_form_field_validate() and entity_extract_non_field_form_values() to assist entity forms in doing the fixed workflow correctly. This allows us to comment the stuff that is tricky in one place, helping with grokability as per #367006: [meta] Field attach API integration for entity forms is ungrokable. Entity forms may choose whether to use these helper functions or do something different, so advanced contrib use-cases that we may not be thinking of yet aren't stuck with any pre-set behavior, as per #68.
- common.inc

Adds hook_node_submit(), because HEAD already has a hook_node_validate(), and having one without the other makes no sense. Also, as per #4, the pattern of a button-level submit handler calling form-level submit handlers doesn't appear anywhere else in core, so this hook gives modules a way to transition away from that pattern and into a hook system, which is more consistent with the rest of the node system. BC is retained for modules that choose not to transition.
- menu.module
- node.api.php

Awesome analysis and collaboration here, guys! Has been a pleasure to read.

This patch looks mostly RTBC to me. Only relatively minor things remain (compared to the complex and critical heart of this patch).

+++ includes/common.inc
@@ -6626,6 +6626,50 @@ function entity_invoke($op, $entity_type, $entity) {
+function entity_extract_non_field_form_values($entity_type, $entity, $form, &$form_state) {

After reading the docblock of this function, the actual function name feels irritating. The entity is not extracted. Instead, form values are selectively extracted into entity properties. At the very least, a function name like the following would be more appropriate:

entity_form_extract_non_field_values()

However, I'm quite sure we can find a better name than that... thinking out loudly...

entity_update_from_form()
entity_form_properties()
entity_form_values_to_properties()

I feel that a proper function name is important here, as this function plays a key role in the process, and anyone who reads this function name will have to read, parse, associate, and understand its meaning, in order to understand what's going on (elsewhere) -- hopefully without having to look up the actual function body to understand what it does.

Ideally, short and concise; but properly expressing the purpose and meaning of this function is a challenge.

+++ modules/book/book.module
@@ -416,15 +416,13 @@ function book_form_alter(&$form, &$form_state, $form_id) {
+      // Since the "Book" dropdown can't trigger a form submission when
+      // JavaScript is disabled, add a submit button to do that. book.css hides
+      // this button when JavaScript is enabled.

Bonus points for opening a new issue that removes the custom styles in book.css in favor of our new, generic CSS classes in D7.

+++ modules/comment/comment.module
@@ -1738,23 +1738,8 @@ function comment_edit_page($comment) {
+  // Add default properties to the comment object.
+  $defaults = array(
     'name' => '',
     'mail' => '',
     'homepage' => '',

@@ -1765,7 +1750,28 @@ function comment_form($form, &$form_state, $comment) {
     'language' => LANGUAGE_NONE,
     'uid' => 0,
   );
...
+  foreach ($defaults as $key => $value) {
+    if (!isset($comment->$key)) {
+      $comment->$key = $value;
+    }
+  }

It looks like these default properties only need to be set for the initial form build; whereas a comment explaining initial form build stuff is only following after these lines. Can anyone think of a condition we could wrap this code into to clarify that? ++consistency

+++ modules/comment/comment.module
@@ -1942,8 +1948,9 @@ function comment_form($form, &$form_state, $comment) {
 function comment_form_build_preview($form, &$form_state) {
-  $comment = comment_form_submit_build_comment($form, $form_state);
+  $comment = $form['#builder_function']($form, $form_state);

(and elsewhere) Shouldn't we at least test for !empty($form['#builder_function']) before trying to invoke it? In case fago goes nuts during the D7 cycle and reinvents everything, he most probably needs a way to set #builder_function to FALSE, NULL, or empty string...?

+++ modules/comment/comment.module
@@ -2061,49 +2068,57 @@ function comment_form_validate($form, &$form_state) {
 function comment_submit($comment) {
...
+  // @todo Legacy support. Remove in Drupal 8.
+  if (is_array($comment)) {
+    $comment += array('subject' => '');
+    $comment = (object) $comment;
+  }

If we are able to remove this, we should do so and break BC here. We already know from the past that one thing should always be one type of thing. Mixed data types may save us 10 minutes now, but will bury down countless of hours/days/weeks of developer time later on.

+++ modules/node/node.pages.inc
@@ -140,7 +142,9 @@ function node_form($form, &$form_state, $node) {
+  // @todo Legacy support. Modules adding form building and processing functions
+  //   to the node form are encouraged to access the node using
+  //   $form_state['node']. Remove in Drupal 8.
   $form['#node'] = $node;

+++ modules/taxonomy/taxonomy.admin.inc
@@ -100,17 +100,28 @@ function theme_taxonomy_overview_vocabularies($variables) {
+  // @todo Legacy support. Modules are encouraged to access the entity using
+  //   $form_state. Remove in Drupal 8.
+  $form['#vocabulary'] = $form_state['vocabulary'];

+++ modules/user/user.pages.inc
@@ -243,6 +243,14 @@ function template_preprocess_user_profile_category(&$variables) {
+  // @todo Legacy support. Modules are encouraged to access the entity using
+  //   $form_state. Remove in Drupal 8.
   $form['#user'] = $account;

We definitely want to remove this for D7 already, since this entire entity form structure and handling is complex enough, so we absolutely need to decrease confusion where possible. Duplicating this data into $form makes no sense at all and is plain confusion.

However, as both of you mentioned before, let's do that in a separate issue.

+++ modules/taxonomy/taxonomy.admin.inc
@@ -100,17 +100,28 @@ function theme_taxonomy_overview_vocabularies($variables) {
 function taxonomy_form_vocabulary($form, &$form_state, $edit = array()) {
...
+  elseif (empty($form_state['vocabulary'])) {
+    $edit += array(
...
+    $form_state['vocabulary'] = (object) $edit;

oh hey, Taxonomy's vocab form constructor is sound! This clear condition and code flow pattern, we can also use in comment_form() at least, as mentioned above. Possibly elsewhere, too?

+++ modules/user/user.pages.inc
@@ -243,6 +243,14 @@ function template_preprocess_user_profile_category(&$variables) {
 function user_profile_form($form, &$form_state, $account, $category = 'account') {

I'm pretty sure that at least fago would also like user_register_form*() functions to be adjusted accordingly. :)

+++ modules/user/user.pages.inc
@@ -278,22 +286,24 @@ function user_profile_form($form, &$form_state, $account, $category = 'account')
 function user_profile_form_submit($form, &$form_state) {
...
+  // Populate $edit with the properties of $account, which have been edited on
+  // this form by taking over all values, which appear in the form values too.
+  $edit = array_intersect_key((array) $account, $form_state['values']);

Is it possible that this is targeting {users}.data hickups, but perhaps not aware of latest changes via #721436: Remove magical fairy saving of cruft from user_save() ?

After addressing these, this patch would look ready to fly for me, so please proceed.

Powered by Dreditor.

This patch renames entity_extract_non_field_form_values() to entity_form_submit_build_entity() and moves the call to field_attach_submit() into it. I think this makes sense given that we're providing the entity_form_field_validate() function: the more parallel our organization structure between validate and submit, the better.

This patch also cleans up and makes consistent the way the *_form() functions get/set the entity in $form_state.

Shouldn't we at least test for !empty($form['#builder_function']) before trying to invoke it? In case fago goes nuts during the D7 cycle and reinvents everything, he most probably needs a way to set #builder_function to FALSE, NULL, or empty string...?

I'm open to discussion on this, but I don't think so. I think a form either has a #builder_function (which it should if it has more than one submit button like "Save" and "Preview") or it doesn't (like the user profile form, which doesn't have "preview" and inlines the entity building into user_profile_form_submit()). I think it would help consistency to always use a #builder_function (even for the user form, where there's no "preview" button), but that's a code organization decision that's ultimately up to each entity form. In any case, any particular submit handler needs to call SOMETHING to get the entity: whether it's a literal function, the way user_profile_form_submit() does it, or whether it's a variable function like $form['#builder_function']. But I don't think it makes sense to not have anything to call.

Regarding breaking BC in comment_submit() to no longer support an array. My preference would be to take that up in a separate issue, perhaps the same one that argues for breaking BC of $form['#node']. I fear webchick will be stressed out enough about this patch as it is, I don't want anything unnecessary getting in the way.

I'm pretty sure that at least fago would also like user_register_form*() functions to be adjusted accordingly.

@fago: want to roll that in to this patch, or leave it for a follow-up issue?

Is it possible that this is targeting {users}.data hickups...

I'm not sure. Perhaps fago can speak to that. But my inclination is to agree with #104. It doesn't really make sense that user_save() takes an $edit parameter. node_save() and other entity saving functions don't do that. But it does, and therefore, we should call the function the way the API dictates: only with values edited by the form.

FYI: unrelated to forms, but for more fun with entity complexity and trying to come up with a semi-sane solution without breaking BC: #823428: Impossible to alter node URLs efficiently.

First, while I know we keep personal issues out of the queue, allow me a rare comment of extreme happiness that this patch got this far without involving me at all. That effulgentsia is awesome we have seen in the #limit_validation_errors but this goes far beyond. Lots of thanks to fago for help, too.

Issue summary is in #66 and in #93 .

The API breakage is sort of minimal but it really needs good documentation because it's also very subtle: the node form now has it's $form_state['input'] = array() removed. Which means that if you have button level submit handler on, say, preview which does something to $node->foo then that change get's overridden by the user input when the form is rebuilt. I think at least this is what happens. And that's fine but it needs to be documented.

I also think the patch is ready but it's really fago who should RTBC this.

Wow: thanks for all the kind words :) I enjoy working on issues like this one: I learned a ton from fago and Frando in the process.

#109:

Which means that if you have button level submit handler on, say, preview which does something to $node->foo then that change get's overridden by the user input when the form is rebuilt. I think at least this is what happens. And that's fine but it needs to be documented.

#110:

I agree that this needs to be documented, but note that this is now default behaviour for any drupal form. So it should be documented globally, if not done yet?

rfay and jhodgdon are working on an issue to add high-level $form and $form_state documentation prominently at the top of form.inc: #802746: Document $form_state and form builder function in form_api group. Here's what's there currently about $form_state['input']:

+ * - 'input': The array of values as they were submitted by the user. These are
+ *   raw and unvalidated, so should not be used without a thorough understanding
+ *   of security implications. In almost all cases, code should use the data in
+ *   the 'values' array exclusively. The most common use of this key is for
+ *   multi-step forms that need to clear some of the user input when setting
+ *   'rebuild'.

If that's the correct issue to address the documentation requested above, but you have feedback on it, please comment on that issue. If you think a new documentation issue is needed, please open one. Thanks!

Lets not let this sit too long. chx effulgentsia and fago have approved.

While I unfortunately do not have the time to do a proper, line-by-line review at the moment, I approve the direction of the patch as well. I properly reviewed the patch in #36, and while it has changed quite a bit since then, I very much like the direction it went. As has been stated several times, a completely clean and generalized solutioln won't be feasable for D7, but this patch goes much further than I thought would be possible. Amazing.

While I still do not like #builder_function a lot, I agree that keeping it might be the best for contrib and for allowing button-level submit handlers to update the entity.

I also like that we added entity_form_* helper functions. This makes it much easier for standard contrib entity implementations.

Anyway: This patch is VERY important and brings much much needed sanity to entity form handling. It has improved incredibly from my first patch in #5, awesome work fago and effulgentsia.

Let's get this in!

Woop, woop. Big patch, buy in from many different people, and a nice improvement. Committed to CVS HEAD. Thanks.

There are issue summaries in #66 and #93, but I'd appreciate it if one of you could update the issue summary here, to summarize the final impact of this patch. It took many forms in its long life.

Also, could you please summarize the impact of the API change and what it means to existing code?

Are there any documentation impacts?

Thanks,
-Randy

Edit: chx in #109 says

The API breakage is sort of minimal but it really needs good documentation because it's also very subtle: the node form now has it's $form_state['input'] = array() removed. Which means that if you have button level submit handler on, say, preview which does something to $node->foo then that change get's overridden by the user input when the form is rebuilt. I think at least this is what happens. And that's fine but it needs to be documented.

chx in IRC also says

also, for custom entity forms, we are no longer autofiring #builder_function on the add more button

The first half on #105 lists the API changes. In summary:

• #builder_function is not called by buttons that use #limit_validation_errors like "add another item", and should not be called by contrib modules that add similar buttons.
• #builder_function is called by "Save" and "Preview" buttons for the node, comment, and taxonomy term forms. A contrib module implementing an entity type form may want to use these forms and their #builder_function implementations as examples.
• The core entity #builder_function functions no longer set $form_state['rebuild']. Contrib module entity builder functions are encouraged to parallel this decision, but each entity builder function is free to decide for itself what it wants to do.
• $form_state['node', 'comment', 'user', 'term', 'vocabulary'] contain the corresponding entity being edited by the corresponding form. Since contrib modules that implement new buttons similar to "add another item" or dependent drop-down to one of these forms must no longer call #builder_function, they can instead modify these variables directly, and the modification will persist throughout the form workflow. See book_pick_book_nojs_submit() for an example. This is a standard multi-step form pattern. Note that in some cases, a submit handler is not required to modify the entity in $form_state, but can use some alternate variable in $form_state to transmit information needed during form rebuild: for example, field_add_more_submit(). In any case, we can't document all possible scenarios here. Perhaps in a more in-depth multi-step form tutorial.

What chx and fago are referring to in #109 and #110 is that we never documented the impact of #622922: User input keeping during multistep forms in http://drupal.org/node/224333 and probably should. D6 multi-step forms were hard partly because a submit handler always had to transfer ALL submitted form values to somewhere in $form_state['storage'] (or within a $form property like $form['#node'], but that was even more confusing), and the form builder function and all form alter functions had to set ALL #default_value properties based on what was in $form_state['storage'], or else what the user had just entered would be lost in the rebuilt form. In D7, it's no longer necessary to go through this complex process for elements that you're simply going to re-display with whatever the user had just submitted, because $form_state['input'] is retained during the rebuild (this is why "add another item" buttons no longer need to call #builder_function). But that means in situations where you do want to clear some of the user input in the rebuilt form, you have to do so explicitly. See poll_more_choices_submit() for an example.

Finally, this patch added a hook_node_submit().

Good job. :)

The only concern that just crossed my mind: In some of the comments above, we identified that the (old) logic/flow could lead to security issues or otherwise invalid data. Would it be possible to add a functional test to ensure that we don't break this relatively complex construct and underlying considerations in the future?

I believe #763376: Not validated form values appear in $form_state will take care of that.

An interesting side-effect caused by the better entity form workflow: #844388: Taxonomy terms disappear from node preview if previewed more than once.