HTML::FIllInForm patch to mark invalid fields

samtregar on 2007-12-03T19:57:21

I've been working on producing yet another form validation system, this time for MasonX::WebApp. I modeled my work on the very nice CGI::Application::Plugin::ValidateRM, with a bit of the Krang::Message system thrown in to help get errors out to the user.

One thing I wanted to do this time that I'd never done before was automate marking invalid form fields. I'd done this in the past using manual template changes, but that's a lot of grunt work.

Instead, my coworker Perrin Harkins suggested we use HTML::FillInForm to do the job. We're already using it to re-fill forms with errors, so why not use it to setup a CSS class on invalid fields too. It turns out HTML::FillInForm has a very similar facility already to disable selected fields, so adding an invalid fields feature was pretty simple.

I've already sent along the patch to the maintainer, but if you want to get it now here it is:

fillinform_invalid2.diff

-sam


how generic is it?

mpeters on 2007-12-03T20:48:08

Awesome! I've always thought it a kludge that we have to use templates to mark invalid fields when we have the niceness of HTML::FillInForm doing the filling for us. Is this generic enough to do any kind of class setting for fields?

Re:how generic is it?

mpeters on 2007-12-03T20:54:20

Thinking about my comment a little more, perhaps I should clarify. Instead of having the ability to specifically mark fields as invalid, why not be able to mark any fields a certain way. Something like:


HTML::FillInForm->fill(\$html, $q,
    classify => {
        invalid => [qw(name password)],
        required => [qw(name password birth_date)],
    },
);


These are just the first 2 obvious uses I see for this, but I'm sure there are others.

Re:how generic is it?

samtregar on 2007-12-03T21:16:13

Sounds potentially useful, although I can't say I have an immediate need for it. I think it would be a pretty easy change to make.

-sam

thanks and comment

markjugg on 2007-12-04T16:18:52

I think I'll like this patch as well.

However, I think the form should be clearly marking which fields are required even before it is reloaded.

So, a simple "invalid" class perhaps should be enough.

I know ValidateRM usually puts "missing" or "invalid" on the page, but I had one customer who wanted to just use "Oops!" in all cases. I think that works in most cases, since the page design should already provide the context to explain /why/.

In a few cases it is imported to provide more detail, like explaining that the photo invalid because the dimensions are too large or something.

Something like that seems to go beyond the scope of what HTML::FillInForm should be doing, but I still like the idea of having an "invalid" class added to some form fields.

Re:thanks and comment

mpeters on 2007-12-04T16:38:58

However, I think the form should be clearly marking which fields are required even before it is reloaded.
You're assuming that I don't already run the form through HTML::FillInForm the first time. It's not just good for showing a form to a user for correcting, but for prefilling a form with either default values or something else (in the case of an edit screen).

You're also assuming that the fields on a form are predefined. We have a CMS that let's users add extra custom fields to different form types and to decide which of those custom fields is required. Having the ability to do this when the form passes through HTML::FillInForm would be the ideal place.

Something like that seems to go beyond the scope of what HTML::FillInForm should be doing, but I still like the idea of having an "invalid" class added to some form fields.
I agree that adding extra HTML elements (like the actual failure messages) is beyond what HTML::FillInForm should be doing, but changing the attributes of the form fields is exactly what it is doing. I always hated having to add tmpl stuff to deal with filling in form values (which really sucked for selects and radios). I don't see this as any different from that pain which HTML::FillInForm already fixes.