[cgiapp] Re: concerns with new header_props

[Mark Stosberg <suppressed>]

On 2003-11-16, Cees Hek <suppressed> wrote:
> It looks like the attachment didn't make it through...  (I guess there are some
> bugs in IMP with attachments!).
>
> Anyway, I have put the patch at the following URL:
>
> http://cees.crtconsulting.ca/perl/patches/CGI-Application-3.1.headers.patch

I was going to work on this today as well. :) 

I appreciate that the patch includes documentation and test updates as
well as code.

I like header_add(), although I would update the docs to be even clearer it's
creating or appending to an array of possible values:

#####
The header_add() method is used to create or add an additional value to a HTTP
response header. Existing values will not be overwritten.  This method is
really only useful for the Set-Cookie header. That's the only common
header that can appear multiple times in an HTTP response.

For all other headers, using C<header_props()> is a better choice, to
prevent accidentally setting two values.

The parameters will eventually be passed on to the CGI.pm header() method, so
refer to the L<CGI> docs for exact usage details.  
######

header_props() and header_set() are too similar and will cause
confusion as well as a kind of interface bloat. It seems the primary benefit of
header_set() is to maintain perfect backwards-compatibility with how
header_props() worked in the past. That's worthy goal. I propose an
alternate solution to address this issue:

header_props() could keep its clobbering behavior by default. However,
a mechanism would be added to allow it merge in new headers and
not clobber the old. One idea for this is to support a private 
hash key perhaps named "keep" or "merge".

This flag would be stored in the in the CGI::App object and would remain
in effect until toggled back.

So, if you want set a cookie in setup(), you could add "keep" once
there, and in the rest of the application, header_props() would
appropriate add to the hash you started:

$self->header_props(
	merge=>1,
	cookie=>$cookie,
);

Considering the "keeping" or "merging" behavior of header_props() is
probably more desirable anyway, new CGI::App projects could just start
putting this in setup() or a super class:

$self->header_props( merge=>1 );
#################

In other news, I want to comment on a couple of warnings in the original
code:

carp ("Replacing previous run mode '$rm' with prerun_mode '$prerun_mode'") if ($^W);
carp ("No such run mode '$rm'.  Using run mode 'AUTOLOAD'") if ($^W);

For these warnings, I disagree that they should be warnings at all. In
both cases, they are warning you about performing an action that is
valid, documented in the interface, and serves some useful function.

Instead of having the logic of "...if warnings are on", I think it
should be "...if DEBUG mode is on".

The would be useful for debugging something unintentional. However, I'd
prefer not to have the warnings when I know what I'm doing. :) 

By constrast, I think the warnings in Cees patch /do/ make sense. It
doesn't make sense to set header properties when header_type == none, 
so a warning seems appropriate.

	Mark


---------------------------------------------------------------------
Web Archive:  http://www.mail-archive.com/suppressed/
              http://marc.theaimsgroup.com/?l=cgiapp&r=1&w=2
To unsubscribe, e-mail: suppressed
For additional commands, e-mail: suppressed