also why modperl and perl-script have a duplication of the env handling, shouldn't it use the same code (probably moving into the _run handler).
well, it's not an exact duplicate - perl-script defaults to +SetupEnv, while modperl does not, which means that the if-logic is slightly different. so, the code duplication is really this in perl-script
if (MpDirSETUP_ENV(dcfg) || !MpDirSeenSETUP_ENV(dcfg)) { modperl_env_request_populate(aTHX_ r); }
versus this in modperl
if (MpDirSETUP_ENV(dcfg)) {
modperl_env_request_populate(aTHX_ r); }
yeah, but that can be arranged so that you don't duplicate it
in perl-script
if (!MpDirSeenSETUP_ENV(dcfg)) {
MpDirSETUP_ENV_On(dcfg);
}and inside the common function:
if (MpDirSETUP_ENV(dcfg)) {
modperl_env_request_populate(aTHX_ r);
}or does it make things less clear to the reader?
plus the new interpreter selection in the modperl handler.
which is (should be) exactly the same as in 'perl-script', no?
But why did you introduce the interp select code? it's not bare bones handler any longer. can we get away w/o it?
I needed the interpreter to get a THX so I can populate the environment +SetupEnv. and it is still bare bones unless +SetupEnv is called - see the
/* default is -SetupEnv, add if PerlOption +SetupEnv */ if (MpDirSETUP_ENV(dcfg)) {
that hold this (and the below) logic. see also the very end of this email
http://marc.theaimsgroup.com/?l=apache-modperl-dev&m=107540306610034&w=2
I can't find any references to select/unselect interp code in that mail. But sure, you need aTHX, no questions. I thought it was adding an overhead to the 'modperl' handler, but it was calling the select inside modperl_callback anyway. So there is no problem.
I'd just like to see that refactored so there is no duplication between perl-script and modperl handlers. Perhaps use some macros. But this can (and probably should) be done after you commit your current work.
+ modperl_env_request_populate(aTHX_ r);
Are you sure? 'modperl' doesn't do anything to %ENV unless +SetupEnv is on.
that's exactly it. see the above note.
Ah, sorry, I've missed the closing brace because of the #ifdef/#endif. That's perfect.
-# PerlOptions +SetupEnv is needed here, because we want the mod_cgi -# env to be set at the access phase. without it, perl-script sets it -# only for the response phase -PerlOptions +SetupEnv +PerlOptions -SetupEnv
why not using 'SetHandler modperl', instead of 'perl-script/-SetupEnv'?
I just toggled the bit to minimize the changes.
sure, let's keep it this way, so it's sort of a test on its own.
In any case the comments in t/modperl/cookie.t are wrong with your .pm patch, because you have added -SetupEnv. I'm talking about:
# in this test we should be able get the cookie via %ENV, # since 'SetHandler perl-script' sets up mod_cgi env var...
the rest of the comment is
...Moreover # adding 'PerlOptions +SetupEnv' adds them at the very first stage used # by mod_perl handlers,
which is no longer true - what we had agreed on was making %ENV only relevant during the response phase. that's why the +SetupEnv logic is in the modperl handler and not elsewhere.
anyway, that's the problem I was trying to fix, and I was trying to do it in the simplest way possible. once the other changes are in place, if you have better suggestions for fixing cookie.t I'm open to them.
I wasn't talking about the code, but the comment. Again with your change it says:
# in this test we should be able get the cookie via %ENV, # since 'SetHandler perl-script' sets up mod_cgi env var. Moreover # calling $r->subprocess_env adds them on demand. the last sub-test makes # sure, that mod_cgi env vars don't persist and are properly re-set at # the end of each request
but the test (after your changes) doesn't do that, because you have set -SetupEnv. So 'SetHandler perl-script' is irrelevant (that's why I've suggested to replace perl-script/-SetupEnv with 'modperl'. So if the setup remains as is, I think this is a more clear description:
# 'SetHandler perl-script' combined with 'PerlOptions +SetupEnv', or # 'SetHandler modperl' don't populate %ENV with CGI vars. So in this test # we call $r->subprocess_env, which does that on demand, and we are able # to get the cookie via %ENV. # # The last sub-test makes sure, that mod_cgi env vars don't persist and # are properly re-set at the end of each request
Also in modperl_env.c you had:
+ MP_dRCFG; + MP_dSCFG(r->server); ... + /* only once per-request */ + if (MpReqPERL_SET_ENV_SRV(rcfg)) { + return; + }
sounds wasteful to me. It was outside the call originally. For some reason you've moved it in:
- /* only happens once per-request */ - if (MpDirSETUP_ENV(dcfg)) { - modperl_env_request_populate(aTHX_ r); - } + /* per-server PerlSetEnv and PerlPassEnv - only once per-request */ + modperl_env_configure_request_srv(aTHX_ r); +
same for modperl_env_configure_request_rec... oh may be I've got a bit lost...
my thought was that I preferred the cleaner approach in modperl_callback, allowing all of the ENV logic to be self-contained, rather than having modperl_callback make decisions on its own.
Since you call these only from one place, it's probably not as bad and will give a significant speedup (as your current code goes, it'll dive into these functions on almost every callback. It's much faster to make a binary compare than calling a function, setup a few variables, and leave it.
anyway, thanks for reviewing it :)
;)
__________________________________________________________________ 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]
