httpd-test-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Wheeler <da...@kineticode.com>
Subject Re: Fix: Apache::TestRequest::redirect_ok
Date Wed, 20 Oct 2004 01:19:21 GMT
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


Mime
View raw message