Clean up node submit hooks

This seems like a bad idea to me and will break alot of modules that extend a node. Instead of having a simple consistent way for adding/updating/delete node based content this seems to fracture what I would argue is a nice clean api. While I like Drupal and am comitted to using it this is one of those changes that I think adds no real benifit while breaking existing code. I understand there are core developers that feel otherwise, but there comes a point in a project where stability is needed otherwise people find themselves frustrated when things move forward. Change is OK, but it needs to be easier to maintain existing modules and keep them up to date.

The proposed changes make sense to me. Should make things easier for developers, and potentially, more secure as well. Haven't tested the patch.

No. This is a bad idea. The hooks being removed are at the data level, and are being replaced at the form submission level. What this means is that if you want to work with nodes programmatically you'll have to fake a form submission, which is a big pain. Remember, through the web browser is only one way to create content. Content could also be created by XML-RPC calls, by wrapping a legacy database, by periodic file import, etc.

When you call node_save, data-related things like nodeapi insert happen. These are not form-related. They're data-related.

This patch makes Drupal less flexible. I also think it's premature to be thinking about this. After fapi 2.0 hits, we should think about it.

I agree with jvdyk on this. to a certain extent.

We need FAPI2.0 to fix this properly, but in the mean time, we have a broken node api.

Previously all validation happened in node_validate, but seeing as a whole bunch of the validation
was moved to the form, the only way to create a properly validated node in code now, is to fake a _POST.

(brought up here : http://lists.drupal.org/archives/development/2006-06/msg00151.html)

Node form is one of the few forms that you can pull the field definitions of.
Even at that point, the field definition is not yet complete, as the form_alter mechanism has not been run on it.

Luckily at that point, the form can validate itself, but it's still far from perfect, as the only way to have the form know
about any code , is by faking the $_POST.

In some of the early code for FAPI, we had an extra parameter in drupal_get_form for the variables, which was meant to
replace $_POST. The problem with that variable was, it was almost impossible to use it properly as you would need
to build the form array (properly) to be able to execute it.

So now we are faced with two choices.

Either we make the necessary changes to FAPI to accept an array other than $_POST for input, and provide a helper function for executing forms (along with the building process and validation) via code.

Or we somehow fix node_save and friends to pull the node form and drive validation through that.

Agreed with Adrian: this patch is ultimately the way to go, but it is premature right now. Once there is a lower-level place to put a _submit callback (rather than as part of a form), this patch can go in at that level.

Let's postpone this for now. John, maybe you can work on FAPI 2.0 instead?

Patched to fresh HEAD no obvious hang ups.
Code style looks good and in depth comments and documentation done well.
This can move ahead. +1

Moshe, Merlinofchaos (Earl), and myself discussed this patch on IRC today. Earl and I are concerned that this patch is taking an approach that is fundamentally a hack. It is treating a form as a data definition system; and it is using the FAPI _validate() and _submit() hooks as the mechanisms for programatically submitting data, whereas FAPI (and its callbacks) is really only designed for submitting data from an actual user-displayed form.

However, with the recent changes to FAPI in http://drupal.org/node/77919 (switch to the 'pull' method, programmatic form submission now possible, etc), it seems that the "Drupal 4.8 way" is going to be the way of treating forms as data definitions. Drupal 4.8/5.0 (i.e. FAPI 2.0) is not going to have a data definition system/layer - perhaps the version after it (FAPI 3.0?) won't have one either. Getting such a system/layer into core will be an enormous job, and it will be extremely tricky to design.

We DO need to get rid of the crufty node hooks that this patch is aiming to nuke. And moving the node validation and submission into FAPI _validate() and _submit() hooks is the only way that this is realistically going to happen. So, I have (somewhat reluctantly!) decided to vote for being realistic: let's try and get this patch into HEAD now!

OK, now that I've gotten through all the philosophical / design issues, here are some issues with the patch itself:

1. This code can probably be moved from node_save() to node_form_submit():

  // Set some required fields:
  if (empty($node->created)) {
    $node->created = time();
  }
  // The changed timestamp is always updated for bookkeeping purposes (revisions, searching, ...)
  $node->changed = time();

2. Can't we encourage module authors to set $form['field_foo']['#value'] instead of $form['#post']['edit']['field_foo']? The former seems like a much cleaner approach to me - we're not pretending that the user submitted the form, we're actually assigning the values directly. However, $form['#post']['edit']['field_foo'] seems to be the recommended approach that http://drupal.org/node/77919 introduced, so maybe FAPI needs to be patched before this can be changed. Chx / Eaton, any thoughts?

Will test out the patch when I get a chance. Otherwise, looks very nice!

The form pull patch is IMO the way to go.

I said the channel just this morning that actually this is the way to go. We test the Drupal forms until they turn blue during the freeze, and it would be a waste to not use just tried stuff to our advantage.

Setting #value directly? Hard. Forms are not flat. You would need to do a recursive walk and set #value sporadically. On your way doing that you are sidestepping the safety pegs we planted into the builder itself. I do not like this. In other words, $form['#post']['edit']['field_foo'] can be the #value of $form['a']['b']['c']['field_foo']. Actually, could add temporarly a watchdog('debug', var_export((_$POST, TRUE); into your form, and adjust accordingly.

About this one. I wholeheartedly agree. It was a very sorry mess before, esp. validate was tainted where in the place of $teaser you got $form...

Forms are not flat. You would need to do a recursive walk and set #value sporadically ... In other words, $form['#post']['edit']['field_foo'] can be the #value of $form['a']['b']['c']['field_foo'].

You need to set $form['#post']['edit']['a']['b']['c']['field_foo'] as well, if #tree is TRUE, don't you? Also, in most cases, the code that sets these values will know exactly what fields it wants to set - it won't just be going through all available fields. So recursive walking will rarely be necessary, since walking of any kind will rarely be necessary.

On your way doing that you are sidestepping the safety pegs we planted into the builder itself.

OK, didn't realise that this would be a side effect. I wasn't aware that values inside '#value' are assumed to be processed / validated. Fair enough. But if this is how we're going to be using the '#post' attribute, then I think that it needs to be renamed to something more generic, like '#submit_value' (this should be filed as a separate issue, of course).

I agree that #post should be named something more appropriate, perhaps #submit_data would work well. In addition, adrian's suggestion from the dev mailing list is a good one: we should also provide a wrapper like drupal_do_stuff($form_id, $form_values) for this sort of action.

For a long time, validation and submission of nodes has been a pain. We've already put the work into validation and processing infrastructure for the forms side of things, this just lets us leverage it in a way that makes sense.

Last patch does not apply, due to changes introduced by http://drupal.org/node/4074 . New patch: exactly the same as previous, but accounts for minor offsets, and for book.module changes.

One thing we should keep in mind is that hooks are also callbacks, just automatically registered ones.

It is concievable to just have nodeform add all the _save and _validate hooks to the stack. This leaves an extra function
definition for all modules out of it, and they can also still have a form_alter to enable them to change the order if needed.

I tested this patch quite thoroughly: in particular, I tried out creating, updating, and deleting nodes of all types that are included in core (book, forum, poll, etc). Updated patch fixes the problems that I found (all the problems were in forum.module):

1. Using array_unshift() to prepend forum_node_submit_early() to $form['#submit'] wasn't working. I implemented a new approach which isn't as pretty, but which works.
2. forum_node_submit_early() is getting passed $node as an array, not an object (when did we start passing nodes as arrays? seems like a stupid idea to me). Therefore, it needs to convert $node to an object at the start, and convert it back to an array at the end.
3. forum_node_submit() had the wrong parameters (was missing $form_id).
4. forum_node_submit() needs to check $node->is_new to determine whether to insert or update.

One more suggestion: I think that we should rename node_save() to something else (e.g. _node_save()), and that we should create a new node_save() function, that executes the code that is current listed in the comments docs as a 'create / edit node example'. Why should module developers have to copy this code, every time they want to save a node? If we want to encourage everyone to save their nodes completely within the FAPI framework, then we should have a shortcut function to make this easier. Same goes for node_delete(), and the 'delete single node example' in the comment docs.

Thoughts on this, people? If we can agree on doing it this way, then I'd like to see this applied to other functions in core modules, e.g. user_save().

I tested book, forum, poll, page. But, I also adhered to Adrian's advice. We have the following handlers automatically registered for node types: $type_node_validate, $type_node_submit_early, $type_node_submit.

Jaza -- reforming node_save is a separate patch IMO.

I too can confirm that I think this is good to go:

1. Tested adding/editing/previewing etc. all built-in node types.
2. Also created some node types of my own and did the same.
3. Tested forum validation.
4. Tested adding nodes to book outlines.
5. Tested the poll "add more choices" option, which tends to break often in these types of patches.

The only thing I found was a stray theme('placeholder') in the forum container validation error, and I'll submit a separate patch for that. :)

Oops. That actually was part of this patch.

Here's a re-roll with this tiny thing fixed.

Tested webchick's latest patch, works great. I say this is ready to go in.

Patch is off by a few lines but still applies cleanly.

This patch makes sense to me. I always wondered why we had duplicates of those hooks when you could almost do that as well with a form_alter(). WIth the recent "pull" changes in, this patch is certainly a nice bridge to cleaning up the API and making things more succinct.

I think the forum validate function could be made uncessary if the forum list used <optgroups> but that is a different patch all together (one that would be nice!).

Overall this patch looks good to me--I like the fact that I can use form_alter() to even alter these submit, valid, and insert hooks ... more flexibility is a good thing, not to mention we are more conistent too.

OT, but optgroups on forums won't work because they cant be nested as containers can. See http://drupal.org/node/27663

Committed to CVS HEAD. This needs to be documented and communicated. :)

NOT fixed, Dries applied the wrong patch ;)

Please revert rev. 1.354 and submit the right patch...

This patch doesn't make any sense to me!?

Is the (automatic?) submit function modulename_node_submit called for all node submissions or only for nodes of type modulename??

If the former, then the patch code for the book module (at least) looks wrong. If the latter, then how does one replace hook_nodeapi($op='submit')?

ok, I think I got this patch for the book module: http://drupal.org/node/81226 to confrom to the changes to the node insert/update/delete flow. Please review

I'm confused. Someone please post a patch and explain what is going on.

After trying to use the API after this patch, I think it was not a good idea. I can't see how it enables programmatic submission of nodes- after trying it last night it seems to make it harder, and I'd like to see it reverted.

As far as the arguments for efficiency, I think an approach that would be similer, but much more consistent with the current API, simpler in terms of updating contrib modules, and essentially the same in terms of reducing the # of fucntion calls would be breaking of hook_nodeapi into a series of hooks like hook_nodeapi_view(), hook_nodeapi_load(), hook_nodeapi_update(). This would probably add much more in terms of efficiency than this patch since there are typically many more load, view, etc. operations than update/insert/delete.

The situation looks somewhat inconsistent. We are removing _nodeapi insert/update/delete/..., but there is still _nodeapi load/view/search result/rss item/... I dont think thats a good idea. At least hook_nodeapi should be renamed to reflect the fact, that there are only operations left dealing with actually displaying the node content.

This patch has been rollback'ed.

setting this to a real version- to see if that permits it to show up in my search results

Shouldn't have been closed.

bumping to D7