perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philippe M. Chiasson" <go...@cpan.org>
Subject Re: [mp1 Patch] Looking for magic in Apache->request
Date Thu, 29 May 2003 05:44:56 GMT
On Thu, 2003-05-29 at 12:09, Stas Bekman wrote:
> Philippe M. Chiasson wrote:
> > On Thu, 2003-05-29 at 07:13, Stas Bekman wrote:
> > 
> >>Geoffrey Young wrote:
> >>
> >>>>The problem pointed out by Ken is that it would be nice to be able to
> >>>>call
> >>>>
> >>>>Apache->request($hr);
> >>>>
> >>>>So that later call to Apache->request (by other modules) would
> >>>>return the subclassed object.
> >>>
> >>>
> >>>it would not only be nice, request() is documented to behave this way :)
> >>>
> >>>
> >>>>Well, the following patch does just that. Seems fine to me but I'd like
> >>>>to get a few more eyeballs on this one.
> >>>
> >>>
> >>>nice work philippe :)
> >>
> >>gozer++
> >>
> >>
> >>>the only thing that concerns me here is the stability this may take away

> >>>from the 1.0 tree for very little value - although stuff like 
> >>>Apache::Filter might benefit from this feature, I'm not sure that adding

> >>>it now is worth it, since any interested modules are becoming deprecated

> >>>in 2.0.
> >>
> >>/me has the same concern. May be keep it in the STATUS file as an available 
> >>patch. After all there was only one request for this feature. If the patch is

> >>available those who want it can try it and if tested enough, can enter 1.29.
> > 
> > 
> > Sounds like a good idea to me!
> 
> let's go with it then.
> 
> >>regarding the patch. why do you keep both IV and SV? Also looks like you mix

> >>usages of NULL and Nullsv for SV* values. Otherwise looks good.
> > 
> > 
> > Well, on the first item of having both IV and SV, it's quite simple in
> > an evil way.
> > 
> > My first attempt, after Doug's suggestion in his original comments, was
> > to simply convert mp_request_rec from an IV to a SV.
> > 
> > But there are problems with that approach. Whenever a user gets a $r,
> > either thru the first argument of a handle or thru Apache->request, a
> > new RV is created and blessed on the fly for it. Plus, in more than one
> > place, the value of mp_request_rec is reset by the mp core in case of
> > subrequests and so on.
> > 
> > After some major fiddling, I've found that in 99% of the cases,
> > Apache->request(something) won't be used, so converting a simple IV to a
> > SV was both overkill and a reference count nightmare.
> > 
> > So the current patch simply keeps using the good old IV most of the
> > time, but if and when someone starts assigning it thru
> > Apache->request(), that user-supplied SV gets saved in mp_request_rec_sv
> > and successive calls to Apache->request will return it.
> > 
> > Also worth noting as a side effect, DESTROY will be called on that
> > object after the CleanupHandlers have been run.
> 
> +1
> 
> > As a side note, there are a few places in lib that still call
> > Apache->request($r) for what seems to me like bogus reasons.
> > 
> > Objections to at least cleaning that stuff up prior to 1.28 ?
> 
> I'm not sure why that code was there. So I'm not so confident on removing it. 
> mp1's test suite is too poor for regression testing.

Yeah, you can say that again. I dunno if it would be eventually worth it
to port mp1's tests to Apache::Test and try and achieve better
covereage. But once again, how long are we planning mp1 will stick
around ? Would it be worth it ? (not for 1.28 for sure)

> > Index: lib/Apache/Registry.pm
> > ===================================================================
> > RCS file: /home/cvs/modperl/lib/Apache/Registry.pm,v
> > retrieving revision 1.34
> > diff -u -I$Id -r1.34 Registry.pm
> > --- lib/Apache/Registry.pm	23 May 2002 04:21:07 -0000	1.34
> > +++ lib/Apache/Registry.pm	28 May 2003 12:53:54 -0000
> > @@ -33,13 +33,6 @@
> >  
> >  sub handler {
> >      my $r = shift;
> > -    if(ref $r) {
> > -	$r->request($r);
> > -    }
> > -    else {
> > -	#warn "Registry args are: ($r, @_)\n";
> > -	$r = Apache->request;
> > -    }
> 
> This has to do with subclasses, I suppose. Which seem to allow calling 
> handler() with no real $r object. I'd keep it in.

Hrm, not super sure about it, but I just didn't like the fact that with
my patch applied, the modperl_request_rec_sv would be set for every
single Apache::Registry requests, which is quite a waste, since it does
not set it to a subclass of 'Apache'. Just would prefer avoiding
creation of useless SV unless it's really neded. How about:

<untested patch>
 -    if(ref $r) {
 +    if(ref($r) ne 'Apache') {
</untested patch>

> > Index: lib/Apache/Status.pm
> > ===================================================================
> > RCS file: /home/cvs/modperl/lib/Apache/Status.pm,v
> > retrieving revision 1.28
> > diff -u -I$Id -r1.28 Status.pm
> > --- lib/Apache/Status.pm	28 Nov 2002 09:42:45 -0000	1.28
> > +++ lib/Apache/Status.pm	28 May 2003 12:53:54 -0000
> > @@ -55,7 +55,6 @@
> >  
> >  sub handler {
> >      my($r) = @_;
> > -    Apache->request($r); #for Apache::CGI
> 
> What's Apache::CGI? If you remove it, will it break that module?
> 

For the love of me, I couldn't find it anywhere, search.cpan.org or
google, so ...

> __________________________________________________________________
> Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
> http://stason.org/     mod_perl Guide ---> http://perl.apache.org
> mailto:stas@stason.org http://use.perl.org http://apacheweek.com
> http://modperlbook.org http://apache.org   http://ticketmaster.com
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
> For additional commands, e-mail: dev-help@perl.apache.org
-- 
-- -----------------------------------------------------------------------------
Philippe M. Chiasson /gozer\@(cpan|ectoplasm)\.org/ 88C3A5A5 (122FF51B/C634E37B)
http://gozer.ectoplasm.org/    F9BF E0C2 480E 7680 1AE5 3631 CB32 A107 88C3 A5A5
Q: It is impossible to make anything foolproof because fools are so ingenious.
perl -e'$$=\${gozer};{$_=unpack(P7,pack(L,$$));/^JAm_pH\n$/&&print||$$++&&redo}'

Mime
View raw message