Posted by ryan_courtnage on April 1, 2008 at 3:59pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | bug report |
| Priority: | major |
| Assigned: | sun |
| Status: | closed (fixed) |
| Issue tags: | API change |
Issue Summary
hook_validate() expects 2 args: ($node, &$form)
form_set_value() expects 3: ($form, $value, &$form_state)
As a result, there is no way to pass $form_state to form_set_value() when inside of hook_validate().
Bentley_: ouch. true.
Bentley_: that's a critical bug, file a report.
eaton: Bentley_ just discovered we do not pass in $form_state to hook validate / nodeapi op validate
Comments
#1
Yup, this is going to be a big problem for trying to port project* to D6.
#2
Indeed, it will be. This is extra confusing because on api.drupal.org (http://api.drupal.org/api/function/hook_validate/6), the notes for hook_validate() state that "The preferred method to change a node's content is to use or hook_nodeapi($op='submit') instead. If it is really necessary to change the node at the validate stage, you can use function form_set_value()." Actually, neither of these will work, at the present. D6 no longer uses the 'submit' op in hook_nodeapi(), and of course you can no longer call form_set_value() unless you can pass in $form_state as a parameter, and that's not possible to do from hook_validate().
#3
Yes, this is hurting me today, trying to update image_attach to a usable state.
I need to update a node with the newly-created image info, and the only place that seems to be able to do now it is nodeapi(validate).
form_set_value() cannot work within hook_nodeapi(validate)
Perhaps we can pass the $form_status in that overloaded 4th argument if appropriate?
... as this is pretty broken and pretty critical, it looks like I'll have to find some other, even hackier way to resurrect image_attach on 6. :(
#4
FTR, my work-around for my problem was to shift the action out of nodeapi into a custom #validate function, where the $form_state IS available.
Either in your form declaration or via hook_form_alter should work.
Just FYI in case someone else is looking for clues.
#5
Subscribing, i spent the day trying to figure out how to get $form_state from hook_nodeapi with $form, I still don't get it ...
#6
Subscribing
#7
Subscribing....
Adding a file upload field to a node form is very difficult to do without this functionality.
#8
I'm using $_POST['form_element_id'] to get the values that used to be in global $form_values.
#9
what I do is :
in hook_form :
$form_state=(array)$form_state;
in hook_validate :
hook_validate($node, &$form_state) {
$form_state['values']['myvalue']=value;
}
This allows to get some special cck fields to work in hook_form as well.
#10
Although this is present in D7 as well and therefore should be fixed there first, it is best to fix it for D6 first. This is because in this case the best way to test whether the patches solve the issue, is with the modules the people in this issue have written when they came across the problems.
Attached is a first attempt at writing a patch that solves this. It's a bit of an ugly hack, but I believe it is a step in the right direction of how this should be fixed.
I wanted to use $a3 or $a4 (in node_invoke*) for this, but $form_state has to be send as an reference to function properly, which can't be applied to $a3/$a4 because of the way they are used by $op = 'view'.
#11
I used hook_nodeapi($op='presave') to change node values.
#12
subscribe
#13
yay! Thanks jakeo, simple idea but it saved my day (or rather, my night). Thanks again!
#14
Subscribing, having same sort of issues..but do not want to hack core..will this patch go in?
Greetings, Martijn
#15
D6's APIs cannot be changed anymore.
However, properly passing $form and $form_state is required for D7, as much more functionality requires those two data stores.
The same applies to hook_form(), as identified in #757154-43: Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter() -- additionally, hook_form() implementations are not able to access, prepare, or alter the main form actions/buttons on the node_form. Since node_form uses button-level submit handlers for the primary form actions, hook_form() implementations are not able to properly participate in those actions.
All of this is a major issue, and a required API change.
#16
The last submitted patch, 241364_hook_validate_formstate.patch, failed testing.
#17
Re-rolled against HEAD, and slightly revamped this.
hook_form() adjustments are still missing.
#18
I don't think it's wise to change node_invoke() to receive parameters by reference this late in D7. It sucks that module_invoke(), module_invoke_all(), node_invoke(), and other *_invoke() functions from ancient Drupal days are unable to route with reference preservation, but let's take that challenge up in D8.
The problem that hook_form() doesn't receive a $form parameter, and runs before node_form() adds the action buttons is annoying (#15), but let's deal with that in a separate issue: it seems pretty separate to the hook_validate() issue this thread is trying to address.
So, how about this patch to tackle the issue of $form_state getting passed to where it needs to?
Leaving the "API Change" tag, but the only BC breakage that I see here is making $form and $form_state non-optional to node_validate(). This can break modules that call node_validate() directly, but modules shouldn't do that: that's like Drupal 4.6 stuff from before we had a Form API. I'm not even all that sure why we still have a node_validate() function: we should move its contents into node_form_validate(), since that's the only function that ever calls it, and it's a pretty thin wrapper function. Note that comment_form_validate() does not call a comment_validate() function. But this probably isn't the appropriate issue in which to remove node_validate() so I restrained myself.
#19
yay! Totally waited for you :)
+++ modules/node/node.api.php 11 Aug 2010 21:54:22 -0000
@@ -1116,10 +1119,12 @@ function hook_update($node) {
-function hook_validate($node, &$form) {
+function hook_validate($node, $form, &$form_state) {
+++ modules/node/node.module 11 Aug 2010 21:54:23 -0000
@@ -926,7 +926,7 @@ function node_object_prepare($node) {
-function node_validate($node, $form = array()) {
+function node_validate($node, $form, &$form_state) {
For full Form API compliance in D7, $form is also passed by reference to all form validation handlers. While the equation of Node API != Form API holds true, I'd be OK with $form not being passed by reference.
+++ modules/node/node.pages.inc 11 Aug 2010 21:54:23 -0000@@ -80,10 +80,7 @@ function node_form_validate($form, &$for
- // Field validation. Requires access to $form_state, so this cannot be
- // done in node_validate() as it currently exists.
wowowow? ;-) Looks like we could have used this simple change a bit earler? :P
Powered by Dreditor.
#21
Acquia lets developers occasionally have a "community" day, and I got to have one today, which I'm so happy with. I'm having another one on Tuesday, but that will be focused on criticals. If you can join that one remotely, that would be awesome!
Um, not in node_form_validate() or even comment_form_validate(). Note that the patch's change to hook_validate() is a documentation change only, because the current documentation is wrong as long as node_form_validate() doesn't have that "&". But I'd be on-board with a separate issue to s/$form/&$form/g;
#22
This patch seems RTBC to me.
#23
Not yet, the following still needs to be resolved:
#24
+1
#25
Did I mention that Node API's entire hook_form() becomes technically obsolete with #757154: Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter() ?
#26
Especially given #25, does #23 need to be done in this issue? (see #18 p2)
#27
I strongly suggest we mark those two callbacks as deprecated and move on with our lives. No API change and everybody is happy.
#28
ok, let's just do this, and let's remove hook_form() entirely in D8.
#29
Mmmm. Is this change ok with chx? I thought it was fully by design that you were not allowed to monkey with $form_state in validate.
#30
form_set_value is the proof that sometimes you really should change form_state from validate. This is OK.
#31
Commtted ? http://drupal.org/cvs?commit=415998
#32
Committed to CVS HEAD. Thanks.
#33
Issue summary and BC API breakage summary please. I see effulgentsia's notes in #18. Perhaps there's no significant API breakage that needs to be announced?
#34
hook_node_validate and hook_validate now has an extra parameter: previously it was $node, $form now it's $node, $form, $form_state. These are API additions that should've been done years ago. Finally, if form_state was changed in hook_form the change did not 'stick' because form_state was not passed in a way that it could be taken by reference. Now it does. It' more an API fix than BC change.
#35
I don't think there's anything that needs to be announced here. Though as per #18.4 and #27, I wonder if we should add documentation that makes explicit the hooks/functions we consider to be deprecated.
#36
Automatically closed -- issue fixed for 2 weeks with no activity.