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;
}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. But if it can handle redirects on POSTs, then we ought to just let it, in which case the patch would look something more like this:
--- TestRequest.pm.~1.100.~ Tue Oct 19 18:08:08 2004 +++ TestRequest.pm Tue Oct 19 18:16:48 2004 @@ -115,14 +115,14 @@
if (exists $args->{requests_redirectable}) {
my $redir = $args->{requests_redirectable};
- if (ref $redir and (@$redir > 1 or $redir->[0] ne 'POST')) {
- $RedirectOK = 1;
+ if (ref $redir) {
+ $RedirectOK = { map { $_ => 1 } @$redir };
} elsif ($redir) {
$args->{requests_redirectable} = [ qw/GET HEAD POST/ ]
if $have_lwp;
- $RedirectOK = 1;
+ $RedirectOK = { map { $_ => 1 } qw/GET HEAD POST/ };
} else {
- $RedirectOK = 0;
+ $RedirectOK = {};
}
}@@ -196,12 +196,12 @@
\%wanted_args;
}-$RedirectOK = 1;
+$RedirectOK = { map { $_ => 1 } qw/GET HEAD/ }; sub redirect_ok {
- my($self, $request) = @_;
- return 0 if $request->method eq 'POST';
- $RedirectOK;
+ my $self = shift;
+ return $self->SUPER::redirect_ok(@_) if $have_lwp;
+ return $RedirectOK->{shift->method};
} my %credentials;
@@ -326,8 +326,8 @@
sub UPLOAD {
my($url, $pass, $keep) = prepare(@_);- local $RedirectOK = exists $keep->{redirect_ok}
- ? $keep->{redirect_ok}
+ local $RedirectOK = exists $keep->{redirect_ok}
+ ? { UPLOAD => $keep->{redirect_ok} }
: $RedirectOK; if ($keep->{filename}) {
@@ -494,7 +494,7 @@
*$name = sub {
my($url, $pass, $keep) = prepare(@_);
local $RedirectOK = exists $keep->{redirect_ok}
- ? $keep->{redirect_ok}
+ ? { $name => $keep->{redirect_ok} }
: $RedirectOK;
return lwp_call($method, undef, $url, @$pass);
};Boris, can you write some tests to demonstrate whether Apache::TestRequest can handle redirects for HEAD requests when LWP is *not* installed? You can add them to t/redir.t.
Thanks,
David
