Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
On Mon, Oct 25, 2004 at 06:47:12PM -0700, David Wheeler wrote: No, just hacking. Let's see...oh, I get it. I changed it so that it ignored $RedirectOK if LWP was installed. Thanks David. No comments here on what's right, only what works ;)
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
On Oct 26, 2004, at 10:00 AM, Geoffrey Young wrote: that's not so bad, but it will affect users somewhat - I know that I have used it in at least one of my tests... Bleh. Bad Geoff! maybe keeping $RedirectOK but moving the perl-framework (and mod_perl) over to the new API would be a nice compromise (along with a deprecated warning in Changes). I decided not to do it this way. I realized that I could just use a lexical for when the module sets up the redirection and let users continue to use the (undocumented) package variable. This is handy for folks who want to use Clocal $RedirectOK, which is a cute hack. I'm still tempted to replace it with a class method, just to enforce good practice. If I do so, I'll probably tie $RedirectOK so that it will indirectly use the class method, and thus remain backward compatible (and can issue a warning for a while, too). Thoughts? But not today. Regards, David
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
On Sun, Oct 24, 2004 at 11:37:11AM +0100, Joe Orton wrote: On Fri, Oct 22, 2004 at 10:09:54PM -, [EMAIL PROTECTED] wrote: theory 2004/10/22 15:09:54 Modified:perl-framework/Apache-Test Changes perl-framework/Apache-Test/lib/Apache TestRequest.pm Log: Redirect from POST fixes (or prevention, depending on how you lok at it). It looks like this change broke the t/modules/alias.t test in httpd-test? Also mod_perl's t/apache/scanhdrs2.t started failing and I can't see anything else that changed, sorry, no time to look any further into it at the mo... Any chance this could be fixed or reverted? It's hindering regression testing... unexpected failures in httpd-test are currently: t/modules/alias.t 62 15 24.19% 14-18 49-58 t/ssl/proxy.t1723 1.74% 8 62 121 joe
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
On Oct 25, 2004, at 4:33 PM, Geoffrey Young wrote: let's give david a chance to investigate - either to fix or, if a quick fix isn't obvious, revert the behavior. if david doesn't respond by, say, wednesday, we (you or I) should feel free to just revert the change. maybe david is on vacation or something, or just temporarily behind in his emails. No, just hacking. Let's see...oh, I get it. I changed it so that it ignored $RedirectOK if LWP was installed. That's not necessarily a good idea, given the goofy way in which this module is written. I've applied a fix to only let LWP handle the call to redirect_ok() if a an array reference was passed to user_agent( requests_redirectable = []). Ugh, that is so ugly! But this doesn't seem to help t/ssl/basicauth.t. But even if I roll back the changes that test still fails, so I'm inclined to think that it's failing for some other reason. You know, I'm inclined to remove that stupid $RedirectOK global variable, because you can't tell whether it was set by the user of Apache::TestRequest. This makes it difficult to decide whether or not to let LWP handle the call to redirect_ok(). What say you all to my changing this to a class method, say RedirectOK()? Then it can be smarter about who is doing what to whom. It would require I change the tests that rely on it, but they don't appear to be too many: % grep -r RedirectOK t t/modules/alias.t:local $Apache::TestRequest::RedirectOK = 0; t/modules/alias.t:local $Apache::TestRequest::RedirectOK = 0; t/ssl/proxy.t:local $Apache::TestRequest::RedirectOK = 0; Thoughts? Regards, David
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
On Fri, Oct 22, 2004 at 10:09:54PM -, [EMAIL PROTECTED] wrote: theory 2004/10/22 15:09:54 Modified:perl-framework/Apache-Test Changes perl-framework/Apache-Test/lib/Apache TestRequest.pm Log: Redirect from POST fixes (or prevention, depending on how you lok at it). It looks like this change broke the t/modules/alias.t test in httpd-test? Also mod_perl's t/apache/scanhdrs2.t started failing and I can't see anything else that changed, sorry, no time to look any further into it at the mo... joe
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
David Wheeler wrote: On Jul 31, 2004, at 5:04 PM, Stas Bekman wrote: I guess losing the skip message by making need_ functions that replace the existing have_ functions is okay. It's most important that tests continue to pass... They will. Then I say we go with need. I kind of favor this as well - it's really no big deal that have functions will all of a sudden stop printing a skip message, and in doing so it will encourage users that care to upgrade to the new function. --Geoff
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
[EMAIL PROTECTED] wrote: theory 2004/07/30 19:43:33 + # Always allow redirection. + my $redir = have_lwp ? [qw(GET HEAD POST)] : 1; + Apache::TestRequest::user_agent(reset = 1, + requests_redirectable = $redir); Using have_ macros for non-plan() usage should be avoided, since it populates the SKIP messages array and if later the test is skipped, for a different reason it'll misleadingly tell the user that LWP was also a requirement for that test (which quite possibly could be what we want). We need to fix that in general (since this issue is recurrent) and have a better way to handle requirements check+skip and only requirements check. Ideas? -- __ Stas BekmanJAm_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
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
Using have_ macros for non-plan() usage should be avoided, since it
populates the SKIP messages array and if later the test is skipped, for
a different reason it'll misleadingly tell the user that LWP was also a
requirement for that test (which quite possibly could be what we want).
We need to fix that in general (since this issue is recurrent) and have
a better way to handle requirements check+skip and only requirements
check. Ideas?
have_foo('bar', 1); # don't populate @SkipReason
or
check_foo('bar');# same as have_foo but don't populate @SkipReason
or a combination of both (where check_foo() is a wrapper around
have_foo($a,1) and we keep the interface undocumented).
?
--Geoff
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
Geoffrey Young wrote:
Using have_ macros for non-plan() usage should be avoided, since it
populates the SKIP messages array and if later the test is skipped, for
a different reason it'll misleadingly tell the user that LWP was also a
requirement for that test (which quite possibly could be what we want).
We need to fix that in general (since this issue is recurrent) and have
a better way to handle requirements check+skip and only requirements
check. Ideas?
have_foo('bar', 1); # don't populate @SkipReason
or
check_foo('bar');# same as have_foo but don't populate @SkipReason
or a combination of both (where check_foo() is a wrapper around
have_foo($a,1) and we keep the interface undocumented).
I'm in favor of having two distinct base names in order to keep things
intuitive -- it's hard to remember what the extra argument in
have_foo('bar', 1) means: should 1 add skip messages, or should it not...
My suggestion for the names selection:
have_foo # don't populate @SkipReason
need_foo # populate @SkipReason
so, have_foo is intuitive to be used anywhere in the code, and plan now
will look like:
plan tests = 5, need_lwp, need_cgi, need_php;
or call it require_foo, or want_foo, or desire_foo, etc...
--
__
Stas BekmanJAm_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
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
On Jul 31, 2004, at 1:14 AM, Stas Bekman wrote: so, have_foo is intuitive to be used anywhere in the code, and plan now will look like: plan tests = 5, need_lwp, need_cgi, need_php; I like this, but isn't it putting the onus of change on module owners and introducing the likelihood of unexpected test failures when module owners don't realize that they need to change their Cuse lines from have_* to need_*? Perhaps we leave have_* with its current semantics, but then add got_* for the new semantics to be used anywhere in the code: # Always allow redirection. my $redir = got_lwp ? [qw(GET HEAD POST)] : 1; Apache::TestRequest::user_agent(reset = 1, requests_redirectable = $redir); Regards, David smime.p7s Description: S/MIME cryptographic signature
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
David Wheeler wrote: On Jul 31, 2004, at 1:14 AM, Stas Bekman wrote: so, have_foo is intuitive to be used anywhere in the code, and plan now will look like: plan tests = 5, need_lwp, need_cgi, need_php; I like this, but isn't it putting the onus of change on module owners and introducing the likelihood of unexpected test failures when module owners don't realize that they need to change their Cuse lines from have_* to need_*? Perhaps we leave have_* with its current semantics, but then add got_* for the new semantics to be used anywhere in the code: # Always allow redirection. my $redir = got_lwp ? [qw(GET HEAD POST)] : 1; Apache::TestRequest::user_agent(reset = 1, requests_redirectable = $redir); to me, got and have are exactly the same thing. How are you going to remember which one to use when? Authors of the existing tests don't have to change anything, have_foo will work just the same, but won't add the skip reason anymore. This won't make affect the existing tests in any way, rather than not printing the reason for a tests being skipped. But, yes, the transition could be made 100% perfect, by keeping have_ as it is, and adding a new interface which doesn't add the skip reason. But we need to find an unambiguous name for it. skip_foo will be good, but we have a general function have(), which can't be replaced with skip(). So may be want_foo() is a better choice. Or may be you have a better name... -- __ Stas BekmanJAm_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
Re: [PATCH] Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
Sander Temme wrote:
on 7/10/03 12:56, Sander Temme at [EMAIL PROTECTED] wrote:
parameter to the request invocations in t/apache/acceptpathinfo.t. Neither
produces any result. Am I looking in the right place?
Breadcrumbing my way through Apache-Test/lib/Apache/TestRequest.pm by
liberally sprinkling print statements, I get to the following around line
97:
+if (my $redir = $args-{requests_redirectable}) {
+if (ref $redir and (@$redir 1 or $redir-[0] ne 'POST')) {
+$RedirectOK = 1;
+} else {
+$RedirectOK = 0;
+}
+} else {
+$RedirectOK = $redir;
+}
+
We arrive at this if statement in all failing acceptpathinfo and
modules/alias tests, and end up in the else clause because $redir is
undefined. The result is that we reset RedirectOK to something undefined,
hence don't follow the redirection. Getting rid of the else clause gets me
my passing tests back, since RedirectOK stays at its default value of '1'.
Like so:
Index: Apache-Test/lib/Apache/TestRequest.pm
===
RCS file:
/home/cvspublic/httpd-test/perl-framework/Apache-Test/lib/Apache/TestRequest
.pm,v
retrieving revision 1.81
diff -u -r1.81 TestRequest.pm
--- Apache-Test/lib/Apache/TestRequest.pm 8 Jul 2003 07:56:24 -
1.81
+++ Apache-Test/lib/Apache/TestRequest.pm 10 Jul 2003 21:05:34 -
@@ -94,8 +94,6 @@
} else {
$RedirectOK = 0;
}
-} else {
-$RedirectOK = $redir;
}
$args-{keep_alive} ||= $ENV{APACHE_TEST_HTTP11};
(also attached to avoid breakage by line wrappage). Tested on Darwin 6.6,
Perl 5.6.0, against Apache 2.0.47.
Does that sound sound?
Does this work?
else if ($redir) {
$RedirectOK = $redir;
}
__
Stas BekmanJAm_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
Re: [PATCH] Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
on 7/10/03 18:00, Stas Bekman at [EMAIL PROTECTED] wrote:
Does this work?
else if ($redir) {
$RedirectOK = $redir;
}
It does. However, isn't this the same condition as in the top if clause?
Wouldn't you want to:
Index: Apache-Test/lib/Apache/TestRequest.pm
===
RCS file:
/home/cvspublic/httpd-test/perl-framework/Apache-Test/lib/Apache/TestRequest
.pm,v
retrieving revision 1.81
diff -u -r1.81 TestRequest.pm
--- Apache-Test/lib/Apache/TestRequest.pm 8 Jul 2003 07:56:24 -
1.81
+++ Apache-Test/lib/Apache/TestRequest.pm 11 Jul 2003 04:28:45 -
@@ -88,14 +88,12 @@
$UA = undef;
}
-if (my $redir = $args-{requests_redirectable}) {
+if (defined (my $redir = $args-{requests_redirectable})) {
if (ref $redir and (@$redir 1 or $redir-[0] ne 'POST')) {
$RedirectOK = 1;
} else {
$RedirectOK = 0;
}
-} else {
-$RedirectOK = $redir;
}
$args-{keep_alive} ||= $ENV{APACHE_TEST_HTTP11};
In other words, if $redir is defined, do something with it to the effect of
$RedirectOK. If it isn't defined, go with the default value which is 1. What
exactly is this code trying to do?
S.
--
Covalent Technologies [EMAIL PROTECTED]
Engineering groupVoice: (415) 856 4214
303 Second Street #375 South Fax: (415) 856 4210
San Francisco CA 94107
PGP Fingerprint: 7A8D B189 E871 80CB 9521 9320 C11E 7B47 964F 31D9
===
This email message is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. Any unauthorized review,
use, disclosure or distribution is prohibited. If you are not the intended
recipient, please contact the sender by reply email and destroy all copies
of the original message
===
TestRequest.pm.patch
Description: Binary data
Re: [PATCH] Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
David Wheeler wrote:
On Friday, July 11, 2003, at 09:27 AM, Sander Temme wrote:
The above patch doesn't work. But this does:
Ehm... works for me. I think you're working in the mod_perl space and I'm
just concentrating on the Apache core. Maybe there are side effects
that I'm
not seeing?
Yes, the redirect_ok subroutine, which is relevant to using lwp to send
requests to the server.. It might work to have it return 0 instead of
undef, though.
I don't think it will... I need redirect to be 1 under the circumstances
that I described: during those apache/acceptpathinfo and modules/alias
tests
that need it. So, unless there is a compelling reason (like
$args-{requests_redirectable} existing and containing pertinent
information), we should leave $RedirectOK alone here.
Oops, of course. The problem was actually my stupid use of local in a
block where it actually wouldn't do anything. So try this, instead:
--- TestRequest.pm.~1.81.~Fri Jul 11 09:02:32 2003
+++ TestRequest.pmFri Jul 11 10:43:36 2003
@@ -88,14 +88,13 @@
$UA = undef;
}
-if (my $redir = $args-{requests_redirectable}) {
+if (exists $args-{requests_redirectable}) {
+my $redir = $args-{requests_redirectable};
if (ref $redir and (@$redir 1 or $redir-[0] ne 'POST')) {
$RedirectOK = 1;
} else {
$RedirectOK = 0;
}
-} else {
-$RedirectOK = $redir;
}
$args-{keep_alive} ||= $ENV{APACHE_TEST_HTTP11};
@@ -298,9 +297,9 @@
sub UPLOAD {
my($url, $pass, $keep) = prepare(@_);
-if (exists $keep-{redirect_ok}) {
-local $RedirectOK = $keep-{redirect_ok};
-}
+local $RedirectOK = exists $keep-{redirect_ok} ?
+ $keep-{redirect_ok} : $RedirectOK;
+
if ($keep-{filename}) {
return upload_file($url, $keep-{filename}, $pass);
}
@@ -461,9 +460,8 @@
*$name = sub {
my($url, $pass, $keep) = prepare(@_);
-if (exists $keep-{redirect_ok}) {
-local $RedirectOK = $keep-{redirect_ok};
-}
+local $RedirectOK = exists $keep-{redirect_ok} ?
+ $keep-{redirect_ok} : $RedirectOK;
return lwp_call($method, undef, $url, @$pass);
};
That looks good. I've committed it. Thanks David!
--
__
Stas BekmanJAm_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
Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
on 7/8/03 0:28, [EMAIL PROTECTED] at [EMAIL PROTECTED] wrote:
stas2003/07/08 00:28:28
Modified:perl-framework/Apache-Test/lib/Apache TestRequest.pm
Log:
Change the way the redirect_ok parameter works so that it affects only
_that call_ to the function. Afterward it should revert to the old
value of $RedirectOK.
Change user_agent() so that the LWP::UserAgent requests_redirectable
parameter actually does something useful vis-à-vis $RedirectOK.
Submitted by:David Wheeler [EMAIL PROTECTED]
Reviewed by:stas
This patch causes the following failures, across multiple platforms:
apache/acceptpathinfo.t 366 16.67% 1-2 13-14 25-26
modules/alias.t 62 20 32.26% 19-38
In analysing, I concentrated on the acceptpathinfo tests and noticed that we
are no longer following redirects, getting 301 responses where we expect
200. Backing out the patch resolves all these failures.
In the documentation, I read that the default behaviours is to follow the
redirects. In the acceptpathinfo cases, this does not seem to happen. I
tried both setting
$Apache::TestRequest::RedirectOK = 1;
and passing a
redirect_ok = 1
parameter to the request invocations in t/apache/acceptpathinfo.t. Neither
produces any result. Am I looking in the right place?
Revision ChangesPath
1.79 +16 -3
httpd-test/perl-framework/Apache-Test/lib/Apache/TestRequest.pm
Index: TestRequest.pm
===
RCS file:
/home/cvs/httpd-test/perl-framework/Apache-Test/lib/Apache/TestRequest.pm,v
retrieving revision 1.78
retrieving revision 1.79
diff -u -r1.78 -r1.79
--- TestRequest.pm24 Apr 2003 05:16:57 -1.78
+++ TestRequest.pm8 Jul 2003 07:28:27 -1.79
@@ -88,6 +88,16 @@
$UA = undef;
}
+if (my $redir = $args-{requests_redirectable}) {
+if (ref $redir and (@$redir 1 or $redir-[0] ne 'POST')) {
+$RedirectOK = 1;
+} else {
+$RedirectOK = 0;
+}
+} else {
+$RedirectOK = $redir;
+}
+
$args-{keep_alive} ||= $ENV{APACHE_TEST_HTTP11};
if ($args-{keep_alive}) {
@@ -278,9 +288,6 @@
}
push @$pass, content = $content;
}
-if (exists $keep-{redirect_ok}) {
-$RedirectOK = $keep-{redirect_ok};
-}
if ($keep-{cert}) {
set_client_cert($keep-{cert});
}
@@ -291,6 +298,9 @@
sub UPLOAD {
my($url, $pass, $keep) = prepare(@_);
+if (exists $keep-{redirect_ok}) {
+local $RedirectOK = $keep-{redirect_ok};
+}
if ($keep-{filename}) {
return upload_file($url, $keep-{filename}, $pass);
}
@@ -451,6 +461,9 @@
*$name = sub {
my($url, $pass, $keep) = prepare(@_);
+if (exists $keep-{redirect_ok}) {
+local $RedirectOK = $keep-{redirect_ok};
+}
return lwp_call($method, undef, $url, @$pass);
};
--
Covalent Technologies [EMAIL PROTECTED]
Engineering groupVoice: (415) 856 4214
303 Second Street #375 South Fax: (415) 856 4210
San Francisco CA 94107
PGP Fingerprint: 7A8D B189 E871 80CB 9521 9320 C11E 7B47 964F 31D9
===
This email message is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. Any unauthorized review,
use, disclosure or distribution is prohibited. If you are not the intended
recipient, please contact the sender by reply email and destroy all copies
of the original message
===
[PATCH] Re: cvs commit: httpd-test/perl-framework/Apache-Test/lib/Apache TestRequest.pm
on 7/10/03 12:56, Sander Temme at [EMAIL PROTECTED] wrote:
parameter to the request invocations in t/apache/acceptpathinfo.t. Neither
produces any result. Am I looking in the right place?
Breadcrumbing my way through Apache-Test/lib/Apache/TestRequest.pm by
liberally sprinkling print statements, I get to the following around line
97:
+if (my $redir = $args-{requests_redirectable}) {
+if (ref $redir and (@$redir 1 or $redir-[0] ne 'POST')) {
+$RedirectOK = 1;
+} else {
+$RedirectOK = 0;
+}
+} else {
+$RedirectOK = $redir;
+}
+
We arrive at this if statement in all failing acceptpathinfo and
modules/alias tests, and end up in the else clause because $redir is
undefined. The result is that we reset RedirectOK to something undefined,
hence don't follow the redirection. Getting rid of the else clause gets me
my passing tests back, since RedirectOK stays at its default value of '1'.
Like so:
Index: Apache-Test/lib/Apache/TestRequest.pm
===
RCS file:
/home/cvspublic/httpd-test/perl-framework/Apache-Test/lib/Apache/TestRequest
.pm,v
retrieving revision 1.81
diff -u -r1.81 TestRequest.pm
--- Apache-Test/lib/Apache/TestRequest.pm 8 Jul 2003 07:56:24 -
1.81
+++ Apache-Test/lib/Apache/TestRequest.pm 10 Jul 2003 21:05:34 -
@@ -94,8 +94,6 @@
} else {
$RedirectOK = 0;
}
-} else {
-$RedirectOK = $redir;
}
$args-{keep_alive} ||= $ENV{APACHE_TEST_HTTP11};
(also attached to avoid breakage by line wrappage). Tested on Darwin 6.6,
Perl 5.6.0, against Apache 2.0.47.
Does that sound sound?
S.
--
Covalent Technologies [EMAIL PROTECTED]
Engineering groupVoice: (415) 856 4214
303 Second Street #375 South Fax: (415) 856 4210
San Francisco CA 94107
PGP Fingerprint: 7A8D B189 E871 80CB 9521 9320 C11E 7B47 964F 31D9
===
This email message is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. Any unauthorized review,
use, disclosure or distribution is prohibited. If you are not the intended
recipient, please contact the sender by reply email and destroy all copies
of the original message
===
TestRequest.pm.patch
Description: Binary data
