Stas Bekman wrote:
> Great work, Geoff. But too big to swallow at once, may be will absorb it
> with time, but your descriptions sounds reasonable to me.
yes, it's a lot to take in. it took me a while to do it, too :)
>> +#ifdef USE_ITHREADS
>> + interp = modperl_interp_select(r, r->connection, r->server);
>> + aTHX = interp->perl;
>> + if (MpInterpPUTBACK(interp)) {
>> + rcfg->interp = interp;
>> + }
>> +#endif
>
>
> What about modperl_interp_unselect? See modperl_response_handler_cgi.
whoops :)
>
> 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);
}
plus the new interpreter selection in the modperl handler.
>
> 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
>
>> + 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.
>> -# 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.
>
> 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.
>
> 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.
anyway, thanks for reviewing it :)
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
