#473 Don't pass ActiveRecordHelper label tags through field_error_proc - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#473 ✓wontfix

Don't pass ActiveRecordHelper label tags through field_error_proc

Reported by Cheah Chu Yeow | June 23rd, 2008 @ 03:55 PM

One of the 1st things I do with a new Rails project is to set a field_error_proc like so:

ActionView::Base.field_error_proc = Proc.new do |html_tag, instance|
  if html_tag =~ /<label/
    html_tag
  else
    # Handle every other tag.
  end
end

This trivial patch checks for a content_tag name argument of :label (because that is what Rails uses internally to generate a label tag) and skips error wrapping.

I don't really see a case for <label>s being wrapped around an error div.

Also, unrelated to this ticket, but I've also changed the #respond_to? calls to receive symbols rather than strings (it's cheaper).

Comments and changes to this ticket

josh September 30th, 2008 @ 05:55 PM
- State changed from “new” to “open”
- Milestone cleared.
- Tag set to “actionpack, patch, tiny”
Seems legit.
You flagged this item as spam.
Rob Anderton September 30th, 2008 @ 06:11 PM
-1

Not everybody uses field_error_proc to wrap fields in

tags...I use it to append an 'error' class to elements, including labels.
Sorry :)
You flagged this item as spam.
Rob Anderton September 30th, 2008 @ 06:12 PM
Oops: that should read "in div tags"...
You flagged this item as spam.
Cheah Chu Yeow October 1st, 2008 @ 02:56 AM
I know what you mean because my preferred field_error_proc does the same using Hpricot (i.e. inserting a CSS class instead of wrapping in needless divs).

Your use of a CSS class on labels is definitely legit, so will gladly withdraw my patch unless someone else thinks it's valuable :) I still don't appreciate the default Rails' behavior though!
You flagged this item as spam.
Rob Anderton October 1st, 2008 @ 10:25 AM
That's exactly what I do.

I wonder if an alternative approach would be to allow something like this:
```
ActionView::Base.on_field_error(:except => :label) do |html_tag, instance|
  # blah
end
```
So the on_field_error method takes an options hash that allows :only and :except options and a block which is called by the error wrapping code.

Might be overkill for the sake of one 'if' though :D
Pratik October 9th, 2008 @ 08:28 PM
- Milestone cleared.
Too late for 2.2
Pratik January 11th, 2009 @ 03:17 PM
- Milestone cleared.
- Assigned user set to “josh”
Not convinced this is a good idea, but Josh can decide that.
josh January 13th, 2009 @ 08:33 PM
- Assigned user changed from “josh” to “Pratik”
Handoff!
Pratik January 18th, 2009 @ 05:39 AM
- State changed from “open” to “wontfix”
Don't really think this is a good change. And now you can just use rails templates when generating a new app :)