Forum OpenACS Development: ad_page_contract to be used on includes

Collapse
Posted by Jun Yamog on
Hi,

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"
        }
    }

Collapse
Posted by Tom Jackson on

Why don't you just rewrite the tcl page which accepts form vars so that it includes the page that doesn't. Fix the way you write pages.

Please don't change ad_page_contract if the behavior changes in any way.

Bottom line: if a tcl/adp pair is an include, write it to be an include.

Collapse
Posted by Dave Bauer on
Tom,

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.

Collapse
Posted by Robert Locke on
Hi guys,

I have written several 'includes' that use ad_page_contract and expect to find the variables in the form data.  I find this behavior to be useful, logical, and consistent.

I appreciate Dave and Jun's intentions, but I agree with Tom. I think it would be better to either add an explicit switch to ad_page_contract or implement a different command that handles variables passed to an include.

My 2 centavos... =)

Collapse
Posted by Tom Jackson on

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!

Collapse
Posted by Jun Yamog on
Hi,

Actually I wrote the code similar to what you guys are saying.  Then Dave B pointed to me what he was trying to do.  Initially I said wow that will be nice.  I might need that instead of putting on the comments of ad_page_contract how to properly use the included .tcl.

I went into my merry way hoping that someday ad_page_contract will be like that.  Anyway after a while due to extensive use of includes and the forms and lists that I was making, I keep on forgeting what parameters to pass.  So I have to read the comments I put in the child file.  Then I remember what DaveB told me, I asked DaveB about that patch.  I then applied it to my instance.  I then found out that it did not work when the request came from a rp_internal_redirect.  I fixed it a bit, then I ended up with the implementation above, after discussion with DaveB.

What this new behaviour means is that it enforces ad_page_contract on includes too.  Actually the coding style of what Rob said is what I have done initially.  I then told DaveB I don't get it.  It works for me.  Then he explained further, then I realize that DaveB idea of using includes and ad_page_contract is proper.

Should the includes pages blindly process the form vars.  Or should the parent page do it?  What also happens if the parent page process the form var?  I am not sure if its ok to use another construct that functions the same as ad_page_contract.

Just look at this:

parent.adp

<include src="child" foo="bar">

child.tcl

ad_page_contract {} {foo:notnull}

In the current form of ad_page_contract.  It will put an error of two values of foo.  So the work around is either remove foo="bar" or remove foo:notnull.  Which I think both workarounds seems to remove how the include is to be used.  Or the include can't be used as stand alone, no ad_page_contract.

What this patch does is that.  If a child.tcl is included.  You have to follow what is stated in ad_page_contract which is foo:notnull and pass it in the include tag.  Also with the patch its now possible to have the parent code to compute for foo.  Which normally may be just a form var of child.  This way pages that normally process form vars can be included on parent pages and the parent pages can alter and/or produce the vars in ad_page_contract.

ad_page_contract can now enforce passing through form vars or what vars much be present on include tags.  I hope things are more clear.  Hope to get more opinion about this.  Thanks.

Come to think of it I maybe should call set_page_form_data_gathered 1 when processing includes.

Collapse
Posted by Tom Jackson on

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.

Collapse
Posted by Jun Yamog on
Hi Tom,

Thanks for lesson in using a wrapper.  But I think it would be nice to just have just parent and child only.  Don't you think so?

Also it would be nice to have wrapped-child to say.  Hey caller I am expecting "var".  Or maybe when wrapped-child is called directly.  Hey request I am expecting "var".

Collapse
Posted by Robert Locke on
<blockquote> Also it would be nice to have wrapped-child to say.  Hey
caller I am expecting "var".
</blockquote>

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?)).

Collapse
Posted by Jun Yamog on
Hi Rob,

Yes I think a switch should be done.  Maybe just one.  So ad_page_contract will run in includes and form.  Since that is new changed behaviour.

Keeping and upgrading api in tcl is a bit harder I guess.

Collapse
Posted by Peter Marklund on
I think having a new proc ad_include_contract would be the clearest. That proc should of course reuse code in ad_page_contract where appropriate.
Collapse
Posted by Dave Bauer on
Wait!

Robert,

If I understand what you are saying, there are query variables that are not referred to in the parent call to ad_page_contract, so if the query vars are not parsed in the include, the query params are never set, so we need to parse the query for the include also.

The intention is that if you happen to refer to the same variable in the parent page and the include that ad_page_contract does not cause an error. This is the opposite of what you are doing, I think.

Right now if this happens ad_page_contract tries to create a variable that already exists and causes an error.

So maybe the key is to check is the variable already exists before trying to create another one?

Jun,  would this work for what you are trying to do?

I'd rather not create another proc unless there is no other solution that does not break existing code.

Collapse
Posted by Robert Locke on
Hi Dave,

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...

Collapse
Posted by Dave Bauer on
Woah, I missed all the discussion while I was writing my post.

I definitely don't want to break any existing code, so I'll look into other solutions including changing the coding style of the pages, and an additional tcl proc.

Peter,

Reusuing ad_page_contract is probably not possible unless some parts of it are broken into seperate tcl procs. I'll have to look into this after 5.0 release.

Collapse
Posted by Jun Yamog on
Hi Dave,

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?

Collapse
Posted by Tom Jackson on

I hope this discussion can somehow be condensed into a TIP, because, in my opinion, mucking _at all_ with ad_page_contract is a big deal. ad_page_contract is _not_ broken, and I believe I have shown a much better method of creating an include that results in a re-useable page.

Even if the result of this experiment is that ad_page_contract appears on the exterior to work the same, it seems odd to have it work one way or another depending on the context it appears to be in. And it will need to do this extra work of figuring out where it is at, for every page, and every variable. Debugging may also become an issue, and it already is difficult to trace problems.

Collapse
Posted by Don Baccus on
Tom's right - this needs to be written as a TIP.  My first reaction to the discussion is that I like Peter's ad_include_contract approach.
Collapse
Posted by Jun Yamog on
Ok will provide a tip when I make the patch.  Although the general approach I will do is to make ad_include_contract a wrapper to ad_page_contract.  Since that is what its currently doing.

Will make a tip and patch, this week or next week.

Collapse
Posted by Jun Yamog on
Hi,

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.