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:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com
> http://modperlbook.org http://apache.org http://ticketmaster.com
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
--
-- -----------------------------------------------------------------------------
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}'
signature.asc
Description: This is a digitally signed message part
