The error class is not only applied to fields of the actually submitted form, but to all forms on the current page that contain fields with the same name. An example:

1/ Go to a /contact page as an anonymous user
2/ On this page you will have both the log-in block and the contact form, both of which have a field named "name"
3/ Submit an empty contact form
4/ Both "name" fields now have the error class while only the contact form's "name" field should have it since the log-in block's "name" field is not #required for the contact form.

CommentFileSizeAuthor
#5 contact.patch2.5 KBwmostrey

Comments

wmostrey’s picture

wmostrey commented
Version: 5.1 » 6.x-dev

If work is going to be done on the Forms API, this should be taken into account, or is this a "won't fix"?

douggreen’s picture

douggreen commented

Does this problem still exist? And if so, can you make these field names unique on this page? If so, please submit a patch.

wmostrey’s picture

wmostrey commented

This problem still exists in the exact same state, just tested it on the latest dev version. Don't you think that changing field names is just a quick fix for that specific case? There are probably a lot more cases where for instance the field name "name" or "pass" is used. Ideally FormAPI itself would be fixed to only mark the submitted form's errorous fields with the "error" class.

As a quick core fix, this could work of course. As far as I can tell the example in the original post is the only instance in core where 2 forms appearing on the same page have a shared field name.

douggreen’s picture

douggreen commented

If you were to fix the more generic problem, how would you do that? I think that the problem is that we call form_set_error with the field name, and this matches more than one field. Would you just match the first field name?

It seems like a reasonable restriction that each page can only have one field of each name. This is already a css restriction (each id value should be unique). So it seems like a bug to me if two fields in two different forms, on the same page, have the same name.

wmostrey’s picture

wmostrey commented
Status: Active » Needs review
StatusFileSize
new contact.patch2.5 KB

A nice and clean patch to fix this issue.

I'm not sure how to fix the generic problem here. I wouldn't match the first field name, but only set the error to the $form_id of the submitted form. Any fields from other $form_ids should be ignored, there's no need to process them for validation since "their" form isn't submitted.

douggreen’s picture

douggreen commented

Did you test this? Don't you also need to change 'name' to 'cname' in contact_mail_page_submit? I think that you'll have to be careful when sending $values to drupal_mail. Would it be easier to change the other form with 'name' on it?

That sounds like a reasonable long-term solution. I think that you'll need to add $form_id as an argument to form_set_error() and form_get_error, and having just done some quick checks, adding it to form_get_error is going to involve a lot of refactoring, and in the case of functions like theme_select, maybe impossible.

wmostrey’s picture

wmostrey commented

contact_mail_page_submit is changed in the patch. I'll also give the long-term-solution a shot but I guess that isn't going to make it into Drupal 6?

douggreen’s picture

douggreen commented

Sorry, I see now that you did carry the name change all the way through to contact_mail (contact's hook_mail). A side-effect of this patch is that $params sent to hook_mail will now include cname instead of name. I checked the other hook_mail's in core (system and update) and they are unaffected.

As far as the long-term solution, if you've found a simpler solution than I'm thinking of, then that would be great, and a simpler patch does have a good chance of making it in. If you have a simple solution, I'd post that here too.

wmostrey’s picture

wmostrey commented

So the current patch is RTBC? I'd like to see it in before the first RC so any bugs coming from this is identified in time.

I currently don't have a simpler solution, I'm testing some things but this will probably take some time.

douggreen’s picture

douggreen commented

It's not RTBC until someone marks it as such, and since I have only looked at the code and not actually tested it yet, I'm not ready to mark it RTBC. Please write up a test scenerio, not so much for me, but for anyone else that wants to test it. Thanks!

wmostrey’s picture

wmostrey commented

Test scenario is still the same as described in the original post:

1/ Enable the contact module and create a contact page.
2/ Logout and go to the /contact page as an anonymous user
3/ On this page you will have both the log-in block and the contact form, both of which have a field named "name"
4/ Submit an empty contact form
5/ Both "name" fields now have the error class while only the contact form's "name" field should have it since the log-in block's "name" field is not #required for the contact form.

With this patch, only the name field of the contact form will be highlighted as "error", not the one from the user login block.

douggreen’s picture

douggreen commented

Also test that if you submit a valid contact form that the email is sent and identical to the way it was before.

wmostrey’s picture

wmostrey commented
Status: Needs review » Fixed

This error is fixed in Drupal 6 and 7 in that the user block doesn't appear on the /user page anymore.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.