Forum OpenACS Development: ad_page_contract to be used on includes

Me and DaveB did some discussion about this:

https://openacs.org/bugtracker/openacs/com/acs-tcl/bug?bug%5fnumber=630

I guess you might have noticed both patches was refused because it did not work 100%.  After my failure on patching it too.  Me and DaveB finally decided to use ad_conn as the global var to know if ad_page_contract was already used or not.

I have gone away with the implementation.  So far it does seem to work.  The following cases work:

- basic includes
- includes coming from rp_internal_redirect
- includes coming from explicit ad_template_return

The following behaviour of ad_page_contract was changed.  It only gets form vars in first pass. Succeeding passes, which  should be likely in an include you will need to pass the var on the include.  Of course the include will complain that notnull vars are not passed, just as if its a standalone .tcl file.  The good side of this is that we can use ad_page_contract to enforce rules on includes.  I can't find the thread where DaveB started to discuss about this.  Maybe someone can point it out.  The merits of what DaveB wants to achieve is something that is positive.

If my current implementation is to be applied, it might break existing code that depends on this situation:

- includes that makes use of ad_page_contract that does not involve the parent doc.  That means reusing ad_page_contract instead of properly passing the vars on the includes.  Can be corrected by properly passing the vars.
- 2nd calls of ad_page_contract in rp_internal_redirect.
ex.
index.vuh
ad_page_contract {} {foo:notnull} {}
mypage
ad_page_contract {} {bar:notnull} {}

on mypage "bar" must be passed not via form vars.

I think this is a more proper behaviour of ad_page_contract, but what are your thoughts?  It may break some code.  If it does break a lot of code.  Maybe we can create a switch on ad_page_contract to function in an include only, bad side is that .tcl file can not be used if its not used as an include.

Here is a the patch:  The later part is needed because ad_page_contract by nature will clobber local vars.  Since it does assume its the first thing that is called.  So I added some checking on those.

diff -u -r1.1.1.1 tcl-documentation-procs.tcl
--- packages/acs-tcl/tcl/tcl-documentation-procs.tcl    15 Jul 2003 12:05:50 -0000    1.1.1.1
+++ packages/acs-tcl/tcl/tcl-documentation-procs.tcl    21 Sep 2003 17:03:08 -0000
@@ -46,6 +46,32 @@
    set ad_conn(api_page_documentation_mode_p) $page_documentation_mode_p
}

+ad_proc -private page_form_data_gathered_p {} {
+    Checks if the ad_page_contract has already been processed once, this is used
+    to check if a page that is in includes will not form process the vars
+    but rather get the vars from passed values on the the include tag
+
+    @return true if the thread has already processed ad_page_contract
+} {
+    global ad_conn
+    if { [info exists ad_conn(page_form_data_gathered_p)] } {
+        return $ad_conn(page_form_data_gathered_p)
+    }
+    return 0
+}
+
+ad_proc -private set_page_form_data_gathered { page_status } {
+    set a flag to see if the page through ad_page_contract has finished gathering
+    data
+
+    @param page_status set to true if page has gathered data
+
+    @see page_form_data_gathered_p
+} {
+    global ad_conn
+    set ad_conn(page_form_data_gathered_p) $page_status
+}
+
####################
#
# Complaints procs
@@ -820,6 +846,11 @@
    # Step 2: Go through all the actual arguments supplied in the form
    #
    ####################
+
+    # check to see if we have processed the ad_page_contract once,
+    # if yes skip through this block of code
+    # big if - start
+    if {![page_form_data_gathered_p]} {

    set form [ns_getform]

@@ -943,6 +974,12 @@
        }
    }
    }
+
+    # we have already process the form vars once, set its status
+    set_page_form_data_gathered 1
+    # big if - end
+    }
+

    ####################
    #
@@ -998,7 +1035,7 @@

        # no value supplied for this arg spec

-        if { [info exists apc_default_value($formal_name)] } {
+        if { [info exists apc_default_value($formal_name)] && ![info exists var] } {

        # Only use the default value if there has been no complaints so far
        # Why? Because if there are complaints, the page isn't going to serve anyway,
@@ -1012,7 +1049,7 @@
            }
        }

-        } elseif { ![info exists apc_internal_filter($formal_name:optional)] } {
+    } elseif { ![info exists apc_internal_filter($formal_name:optional)] && ![info exists var] } {
        ad_complain -key $formal_name "You must supply a value for $formal_name"
        }
    }

The goal, I think is to allow reusable forms. Allowing ad_page_contract to be used on includable forms is useful.

I think fixing a shortcoming in ad_page_contract is not a bad thing. I definitely think that it can be accomplished without breaking any existing code.

We have to remember that ACS 4 that OpenACS is based on was never finished, and there will definitely be places where we can improve on what we started with. As more and more developers use OpenACS to built real web sites, they will find  things that can be improved.

ad_page_contract also serves to provide consistent documentation of tcl/adp pages. I think just that feature is worth using on includable pages, besides the other benefits.

I never said don't use ad_page_contract for documentation purposes. But it sounds to me like you have a page which is designed to be accessed normally, and then you decide to include it in some other page. _If_ it is an include, write it like one, in other words, write a wrapper around the include. Now you have provided documentation on how to include the tcl/adp pair in other templates.

It might also be nice in this case to pass in the .adp template that should be used like:

<include src="myfile" var="val" template="mytemplate" >
 
# then in the include:

ad_return_template $template

Now everyone knows they can include 'myfile', and they can use their own template on top of that!

Here is an example of a wrapper:

parent.adp:

<include src="wrapped-child" var="val" >

child.tcl

ad_page_contract { I handle query vars } {var:notnull} 

ad_return_template

child.adp:

<include src="wrapped-child" var="@var@" >

otherpage.adp:

<include src="wrapped-child" var="@var@" >


wrapped-child.tcl:

ad_page_contract { This is my include file, no query vars.} 

ad_return_template

wrapped-child.adp:

my var is @var@

You can use ad_page_contract on the include file, and you can possibly copy and paste the include tag to other templates when needed.

I agree, Jun.  But I don't think we should overload ad_page_contract for that purpose.  Having ad_page_contract change its behavior based on context sounds like trouble to me.

I vote for a switch.  Maybe -form (default), -vars (just check in namespace), and -both (check in both (but in what order?)).

Sorry Dave, I should have read your bug report more carefully.  I think there are 2 separate issues here.

One has to do with your bug (ie, ad_page_contract throws an error when the variable already exists from a previous invocation of ad_page_contract).  Checking for its existence seems like a reasonable solution, though it might mask a deeper problem relating to parse levels and such.  I dunno.

The other has to do with allowing 'includes' to provide a contract for invoking them.  If this is a desired feature, I like Peter's idea for an ad_include_contract.

In any case, bottom line, I don't think ad_page_contract's fundamental behavior should be changed in any way (other than fixing that weird bug ofcourse!)

Thanks...

I have done the checking on the clobber on the latter part of the patch.  So its in there already.  Since after making the patch.  The default value override the local var value.  Hehehe.

Hi Peter,

We actually use pretty much all except adding an if.  And 2 procs to set and check the status of ad_conn was added.  Also I think it will be redundant if the case is like this:

ad_page_contract {} {foo:notnull}

ad_include_contract {} {foo:notnull}

All,

I will create a new patch that has a switch.  I think that would be best.

How about this:

ad_page_contract {} {} {} -some_nice_name_that_says_apply_contract_to_includes_too

Hmmm maybe

ad_page_contract {} {} {} -apply_to_page_n_includes

How about that?

I took another stab at this.  Anyway I made a ad_include_contract.  As of now basically it runs ad_page_contract.

So my .tcl file has something like this:

----------------------------

ad_page_contract {
    display search results

} {
    name_search:optional
    title_search:optional
    content_search:optional
    content_type_search:optional
    {display_search_results:optional 0}
    {bulk_actions:optional ""}
    {bulk_action_export_vars:optional ""}
    {orderby:optional}
    {page:optional}
}

ad_include_contract {
    display search results

} {
    name_search:optional
    title_search:optional
    content_search:optional
    content_type_search:optional
    {display_search_results:optional 0}
    {bulk_actions:optional ""}
    {bulk_action_export_vars:optional ""}
    {orderby:optional}
    {page:optional}
}

-------------------------------

Looks a bit odd.  But both ad_include_contract and ad_page_contract on my example has the same validation functions.  Anyway its a bit hard since ad_page_contract will complain that it has 2 values of foo.  Since I pass foo on the include tag.  If I remove ad_page_contract things are fine.  But I really would like my page to be either called alone or within a page.  I use my search.tcl when searching or included on relating items page.

Anyway I have given up.  I will not post a tip anymore.  I need to get my project underway.  Thanks.