Forum OpenACS Q&A: Validating form elements with the form builder

I am working on fixing up forums a little bit. I need to validate that
the content of a post specified as HTML does not contain any
unauthorized tags.

I want to use the value of the html_p form element to determine how to
validate the body element.

I am having trouble with this because the html_p element is not
created until after the body element.

First I was just going to move the html_p widget, but people seemed to
this it more logically should be after the message body.

Is there any other way to check the value of the html_p element?
I could possibly check this in ad_page_contract, but then I don't get
nice inline error messages.

I could write the entire form template by hand, but that seems
extreme. Is there any way to access the form contents before form
element create is called for the html_p element?

Collapse
Posted by Jeff Davis on
you could always do it the old fashioned way with
[ns_queryget html_p f] in the body validation bit.
Collapse
Posted by Tom Jackson on

Is it just me or is anyone else concerned with the chunking over the side of ad_page_contract in favor of the form builder procs? My opinion is the .tcl page should be setting up vars for use in an .adp template. If you want to use a different template than the default, just specify the location in the call to ad_return_template. Some parts of the form builder procs are very poorly written. Specifically, template::form get_values calls ns_getform for each value it is asked to get. This requires parsing the entire query set for each value. For a novice like me it takes forever to figure out where the form template is located and to figure out the spagetti code created by this bunch of procs. Sorry for venting, but this approach to coding has so far failed to save me any time on debugging other people's code, an unfortunately necessary past time. Then you get suggestions like Jeff just made to call ns_queryget half way down a page. This is nice and convenient for the developer, but inconvenient for future maintainers. Wasn't there a standard way of doing stuff like this at some point? Why is ad_page_contract even being used in these cases?

The kinds of questions that have arisen lately over the use of this set of procs indicates to me that they are far from convenient to use in anything but the most simple situation.

Collapse
Posted by Jeff Davis on
The problem is that if you use ad_page_contract as it stands it is convenient for developers and maybe easier to maintain but then you get error pages like:
We had a problem processing your entry:

* Value for subject contains HTML tags
* bug Id must be an integer

Please back up using your browser, correct it, and resubmit your entry.

Thank you.
which is infinitely worse from a user standpoint that inline error messages like you get with the form builder code.

You can say the form builder code is bad but I am pretty sure it (or something roughly as complex) is necessary for building a usable web interface. Form builder has failings and we should address them (the one Dave ran into and the duplication of filtering/validation procs between ad_page_contract and the form stuff being a couple I am aware of). Any replacement would like have roughly equivalent shortcomings over the medium term.

It is also a misconception that form builder replaces ad_page_contract. ad_page_contract also serves to document pages which have no associated forms and ensure that inputs adhere to strict rules. It has it's place even on pages with forms.

Collapse
Posted by Jon Griffin on
I have also updated my doc on using form builder, adding the suggestions I recieved and also showing some tricky date stuff (It actually has a problem with PG 7.2)

I will upload it tomorrow as I am not at home now. I will also build a mega form with every option I can think of to show some of the more (shall I say) obscure uses.

I started off hating the forms API, I then progressed with it a little, did a client delivery, and now I can see the merit of it.

So given how sceptical I was I think I'd have to say it *is* a good thing.

But, I think it so correct to point out it does become a little more troublesome when applied to more complex situations.

As you've probably seen I had some problems/difficulties when using it in a site with a number of inline frames etc..

I would certainly like to see a little more in the way of API control of its behaviour.

Collapse
Posted by Tom Jackson on

Of course I'm just complaining about something I don't know everything about, which isn't exactly fair. I understand Jeff's point about the ugly complaints you get from ad_page_contract. However relegating this proc to a documentation role seems really to chunk good code overboard for no good reason. Maybe ad_page_contract could be given a new switch 'nocomplain' for each var. New status vars could be set for each var/filter that is run. These status vars could be made available to the tcl/adp to control output. ad_form can still be used in this case, it is just that the request processing is handled efficiently at the top of the page.

I'm not going to deal with the seeming mess that is created by having one page perform multiple functions, I guess the developer can choose to do this. Again the main problem is not in developing with this system, but with maintaining it. I get the feeling this code was written for tcl'ers so they can quickly bang out a UI for a simple data model. We (I guess I'm one of 'em) don't really like writing the adp files: they don't do anything!

Why am I complaining? Because maintainers are having trouble maintaining code using this stuff.

Collapse
Posted by Don Baccus on
There is some really ugly code in the form builder as both Tom and Jeff point out.  I'm hoping to find some time to do some serious clean-up.  I personally dislike the style in which it is written but I see no reason to do a large-scale rewrite for that reason alone.  I'm only interested in cleaning up functionality, complete uncompleted stuff (as I've partially done with the date and currency types for 4.6), etc.

My goal will be to make the data types straightforward and easy to use and better integrated with ad_form.

I agree with Jeff's statements regarding the fact that inline errors are extremely useful, the fact that ad_page_contract and the form builder don't really fulfill the same need, etc etc.  Also writing new presentation templates for the form builder is actually quite easy once you understand how the "standard" template works.  I've written several and it's very convenient to be able to write a small set of templates that can be shared by all form pages on a client sites.

Strangely enough, some clients seem to really care about the look of forms on their site and the templated form builder supports this surprisingly well.  It is rather opaque, I admit (easy for me to admit since I didn't write it!)

As far as usability, I wrote ad_form because I decided hiding the original API would be a lot easier than rewriting all of the templating form builder code.

Now, as I mentioned elsewhere I think it won't be that hard to allow use of ad_page_contract-style filters with ad_form, for trimming and that kind of thing.

Dave ... as far as your problem goes I think you may've uncovered a boneheaded implementation strategy on my part, i.e. the validation block shouldn't be executed until all the form elements are created.  I thought that was true, frankly ... I'll look later today.

In the interim, remember that the "on_submit" block is executed before the "new_data" or "edit_data" blocks and after all form elements are created.  You can use the form builder's set_error procedure to put the error into the form after calling the text-html proc, which does the required html security check for you as well as do the right conversion.

Oh, damn, that proc returns an error string if there's a problem ... that's not much help.  Can't really expect your code to check for the error string then set the error!  OK, I've got a few things to look into today, thanks, dude!

Collapse
Posted by Dave Bauer on
Don,

Thanks alot.

I actually reverted to using the form builder native procs in forums because it made more sense with the 2 step confirm/add options page flow that forums uses.

Maybe someday it will be rewritten with ad_form, but for now the code works and is checked in using the recommendation from Jeff.

Collapse
Posted by Jeff Davis on
Don, the validation is actually done in the template::element::create
routine and the problem is that since it is done there it gets
evaluated in the order that the elements are created.  Hence the
forward reference fails.  I saw two possible solutions, one is to only
validate when you call is_valid (at which point the form is created
and there are no forward references), the other would be to add
validation at the form level for composite checks (which is more or
less what ad_page_contract does).  Either way I think it might break
some older code...

Also, One issue for composite checks is how to attribute the check
back to a particular element for presentation purposes.  I like the
way it's done in ad_page_contract and I think we should only be using
one set of filters in any case so maybe we can figure out a way to do
this that makes ad_page_contract and ad_form be as similiar as
possible in the frontend and share common transform and validation
code.

Tom, I think you have it backwards on why form builder is there -- it
is to make extremely complicated forms managable.  If the form is
small you can do it all the old way and get it right. When you start
talking about something like a CMS system with lots of dynamic content
types you can't manage the complexity without resorting to a form
builder style system to manage the state.  I am a big fan of ad_page_contract and want to preserve it but I know why form builder
is necessary and I think it should be the standard way to handle
user input except in certain special circumstances.

Collapse
Posted by Don Baccus on
Dave ... ad_form handles two-part form scripts with a confirm page without any problem (check out "confirm_template"). Of course exactly duplicating the existing forums behavior might be a bit tricky ...

My examples are always self-submit scripts because that means you only have to define or change form element definitions in one place. This is also why I tried to make it easy to write single scripts that handle editing of existing data and the insertion of new data.

Shared code is good ... if it's not overwhelmingly complex, and my hope is that the ad_form wrapper simplifies stuff to the point where this style is simpler than the alternative.

Jeff ... yes, single-value validation is done as you describe. I was assuming that Dave tried writing an ad_form validation block, checking the html_p widget and if set true then transforming the data string to HTML (which doees a html security check) otherwise transforming it to text.

ad_form's validation block is executed from within ad_form, not the form builder itself ...

In ad_form the way you pin an error to a particular form element when you're doing a composite check is to give that form element's name as the tag to the validation snippet. Simple datatype validation errors are handled by the form builder and of course pinned to the element that triggers it.

    {end_date
        { [template::util::date::compare $end_date $start_date] > 0 }
        "End date must be after the start date"
    }
This example checks that an end date follows a start date, and pins the error to the end date field. If I wanted to pin it to the start date element I'd change the "end_date" tag to "start_date" - and presumably rewrite the error message to reflect this.
Collapse
Posted by Jeff Davis on
Don, thats great.  I confess that I did not look closely enough at the
ad_form code to see that it did this.  One thing I wanted to do (and which would fix more things than it broke) was to make -required text fields say ![string is space $value] rather then ![empty_string_p $value]
since I think if a text field is required all blanks should not
count as provided.  You see this all over for things like folder names and group names and what happens when a space is entered is the links
to edit the things don't show up either (since of course they say things
like <a href="edit?id=@id@">@folder_name@</a>) and unless you show source and enter the url by hand you can't get to the edit page to fix it.
Collapse
Posted by Don Baccus on
The handling of text fields is done in the form builder, of course.  I'd have to look to see whether knocking out spaces would work because there are a whole slew of widgets that use the basic text data type.  For the datatype/widget pair text/text I think it would make sense to disallow blank entries, no doubt about it.  So perhaps it should be a widget check not datatype check but the form builder's built around the notion that the datatype implementation routines do the checking not the widget implementation code ...

Dave ... I looked at the ad_form code that executes the validation block and all form element values are set before that block is executed.

Can you e-mail me or post here the code that failed?

Collapse
Posted by Dave Bauer on
Don,

It wasn't ad_form, but the stanadrd form builder procs. The validation worked fine.

The problem was, forums adds additional form fields on the confirm screen for attachements and notifications.

To get the validation checks into 4.6 without too much trouble I kept the form builder stuff that was already in there and added validation.

If there is a way to add validation to a form element after it is created, maybe I can try that.

Collapse
Posted by alex vorobiev on
i found this thread and figured that my question would be somewhat relevant.

i am trying to figure out various ways to validate form input data in openacs.

1) via ad_page_contract: this one displays a brief error message and instructs user to use the back button;

2) ad_form validate flag, does this one actually invoke form::element validate, or does it's own validation?

3) form::element validate flag;

finally, i am looking at
https://openacs.org/doc/acs-templating/demo/sandwich,
where the form has inline validation, ie the form is redisplayed with specific error messages.  yet, looking at the code, none of the above options are being used.  what i have noticed that the only field that doesn't generate a validation error when not populated, vitamins, has a -optional flag to from::element.

hence my questions:
1) in the acs-template docs the form::element actually is not documented as having such a flag available; the form::request, however, does...  out-of-date documentation, or does form::element pass the flag onto request (just guessing here, without detailed knowledge of the code).

2) if ad_form is just a wrapper around form templ. functions, is the -optional flag getting set for all elements by default?  in other words, it seems like no validation takes place in ad_form unless i explicitly create validation blocks.  so, i am trying to figure out how the -optional flag factors into this (as in the sandwich example above).

thanks for all your help and patience,

--sasha

Collapse
Posted by alex vorobiev on
re my question #2): it looks like if i take out the variable from the ad_page_contract, then i get nice inline validation error message through ad_form.

as a related question: i am assuming then that all elements added via ad_form are considered required unless specified otherwise; if so, why don't i see red stars next to required elements, since it should be true by default as per roberto mello's ad_form reference sheet...

--sasha

Collapse
Posted by Jade Rubick on
Alex, I don't know the answer to that. Since nobody is responding here, perhaps you can ask on IRC, and follow up here if you find it out?