On Oct 19, 2004, at 2:48 PM, Stas Bekman wrote:
In which case, can you please review Boris' patch and commit it if you think it's good? As I haven't coded and haven't used much this feature, I'd rather let somebody who is more familiar with it do the decision. If it breaks something, we can always fix that later. And of course when changing something it's a good idea to add a new test which exercises that fix. Thanks.
I hate this code.
I think that the original idea was that, if LWP was installed, then redirects from POST were supported. Otherwise, they were not, as Apache::TestRequest didn't have the code to handle it. (Why? I have no idea.)
If that's the case, then Boris' code makes decent sense, beucause the call to can('SUPER::redirect_ok') will return a code reference if LWP is installed and TestRequest inherits from it. Otherwise, it decides for itself. So to keep things consistent, I would actually do this:
--- TestRequest.pm.~1.100.~ Tue Oct 19 18:08:08 2004 +++ TestRequest.pm Tue Oct 19 18:09:24 2004 @@ -199,8 +199,9 @@ $RedirectOK = 1;
sub redirect_ok { - my($self, $request) = @_; - return 0 if $request->method eq 'POST'; + my $self = shift; + return $self->SUPER::redirect_ok(@_) if $have_lwp; + return 0 if shift->method eq 'POST'; $RedirectOK; }
that looks better than goto :)
But this means that POST redirects will work if LWP is installed and won't if it is not. If it's true that, without LWP, TestRequest *cannot* handle redirect on POST requests, then this is how it should be.
I believe that it can't. I think TestRequest doesn't try to provide a replacement for lwp. It's a very basic client which should be used only for very basic tests. One should add need_lwp in the plan() call for more complicated cases.
-- __________________________________________________________________ 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
