Patch for implementing ap_document_root as a hook

[Jakob Goldbach <ja...@gmail.com>]

Hi,

Attached is a patch which implements ap_document_root(r) as a hook.
This way modules can set document_root on the fly. (think vhost_alias)
AND get the right DOCUMENT_ROOT env. variable (as set by
ap_add_common_vars(r)).

The patch also changes ap_core_translate to use ap_document_root(r)
instead of conf->ap_document_root.  This way, modules that just need
to point to a different docroot won't have to implement a translate
hook by appending r->uri til r->filename, but just rely on
ap_document_root.

Comments ?

/Jakob

Re: Patch for implementing ap_document_root as a hook

[Jakob Goldbach <ja...@gmail.com>]

It is now in bugzilla as  #42192

/Jakob

Re: RFC: replace r->subprocess_env was Re: Patch for implementing ap_document_root as a hook

[Jakob Goldbach <ja...@gmail.com>]

> Thoughts?

I like it.

I prefer this generel env. API rather than making
ap_add_{common,cgi}_vars hook'able.

/Jakob

RFC: replace r->subprocess_env was Re: Patch for implementing ap_document_root as a hook

["Akins, Brian" <Br...@turner.com>]

This idea has been rattling around in my head off and on for a while.  What
is we replaced all the r->subprocess_env with something a little more
interesting...

General "environment" API:

/*
"directly" set an env variable. Will always show up in env list
*/
apr_status_t ap_set_env(request_rec *r, const char *key, const char *val)

/*
Get the value of an env var.
*/
const char *ap_get_env(request_rec *r, const char *key

And the interesting ones:

/*
Set a handler for a given key for env variables.  Can choose whether or not
the key shows up in the list.
*/
apr_status_t ap_set_env_handler(constchar *key, ap_env_func *func, int
show_in_list)

/*
Return a list of available (exposed) env variables suitable for iteration
*/
apr_array_header_t *ap_env_list( request_rec *r, const char *key)

ap_env_func would be :
const char* my_env_handler(request_rec*r, const char* key)


This would allow most env variables to be overridden easily.  Also, many env
variables could be set "lazily," ie, only calculate it when someone actually
needs it.  A good example of this is when you occasionally use UNIQUE_ID and
only want the calculation to be done when you actually need it, not on every
single request.

The handler could cache it's results if it wanted.  We may want a flag to
say cache is okay or not. Or do caching in env itself...

The actually handler, could actually be a hook.  For example, the handler
for "DOCUMENT_ROOT" could actually be a wrapper around a hook.

Thoughts?


-- 
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies

Re: Patch for implementing ap_document_root as a hook

["Akins, Brian" <Br...@turner.com>]

On 4/23/07 11:33 AM, "Paul Querna" <ch...@force-elite.com> wrote:
> +1, I've been down this road before too.

+1 on the concept. Still looking at patch.

-- 
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies

Re: Patch for implementing ap_document_root as a hook

[Paul Querna <ch...@force-elite.com>]

Brian J. France wrote:
> 
> On Apr 23, 2007, at 10:32 AM, Jakob Goldbach wrote:
> 
>>> -1 on the face of things.  The map_to_storage hook was added to 
>>> accomplish
>>> what you desire.
>>
>> I thought map_to_storage was made to do per-dir configuration. Not
>> path-translation.
>>
>> The problem is not really doing the translation. I can always provide
>> my own translate handler in my module.
>>
>> But in the current API I cannot to set my env. variables at will. They
>> will be overwritten by ap_add_common_vars which returns
>> carved-in-stone docroot from ap_document_root.
> 
> We need this same functionality (would like to back port to 2.2 if 
> possible).
> 
> We currently hack the doc root in the post read hook in 1.3, would like 
> to be able to do it with out hacking the core and screwing around with 
> internal structs at runtime.

+1, I've been down this road before too.

The problem is that many applications use DOCUMENT_ROOT.  I kinda think 
that they shouldn't, because the entire concept of an document root 
isn't very valid with Alias / Rewrite rules changing the path, but it 
doesn't change that people are using it.

Either we need to change ap_add_common_vars to allow overrides easier 
(and hopefully in a more generic way, for all environment variables), or 
we need to truly make it pluggable, like the path that this patch is 
going down.

I do have issues with how this patch does it, but its general direction 
isn't wrong.

-Paul

Re: Patch for implementing ap_document_root as a hook

["Brian J. France" <li...@firehawksystems.com>]

On Apr 26, 2007, at 8:35 AM, Jim Jagielski wrote:
>> We currently hack the doc root in the post read hook in 1.3, would  
>> like to be able to do it with out hacking the core and screwing  
>> around with internal structs at runtime.
>
> VERY doubtful that 1.3 will be updated to do this.

I don't need it in 1.3, but I would like to have a clean way to do it  
in 2.x that doesn't include replacing data in the internal structs at  
runtime and putting it back at the end of the request.

Brian

Re: Patch for implementing ap_document_root as a hook

[Jim Jagielski <ji...@jaguNET.com>]

On Apr 23, 2007, at 10:46 AM, Brian J. France wrote:

>
> On Apr 23, 2007, at 10:32 AM, Jakob Goldbach wrote:
>
>>> -1 on the face of things.  The map_to_storage hook was added to  
>>> accomplish
>>> what you desire.
>>
>> I thought map_to_storage was made to do per-dir configuration. Not
>> path-translation.
>>
>> The problem is not really doing the translation. I can always provide
>> my own translate handler in my module.
>>
>> But in the current API I cannot to set my env. variables at will.  
>> They
>> will be overwritten by ap_add_common_vars which returns
>> carved-in-stone docroot from ap_document_root.
>
> We need this same functionality (would like to back port to 2.2 if  
> possible).
>

I'm +1 on the concept but -0 on the provided method of
doing it...

> We currently hack the doc root in the post read hook in 1.3, would  
> like to be able to do it with out hacking the core and screwing  
> around with internal structs at runtime.
>

VERY doubtful that 1.3 will be updated to do this.

Re: Patch for implementing ap_document_root as a hook

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

William A. Rowe, Jr. wrote:
> Brian J. France wrote:
>> On Apr 23, 2007, at 10:32 AM, Jakob Goldbach wrote:
>>
>>>> -1 on the face of things.  The map_to_storage hook was added to
>>>> accomplish what you desire.
>>> I thought map_to_storage was made to do per-dir configuration. Not
>>> path-translation.
> 
> Actually it does both.  An amusing use case is

...not the one I was thinking of, this one is more fun... jump to the
ftp_cmd_rnto(...) implementation in

http://svn.apache.org/viewvc/httpd/mod_ftp/trunk/modules/ftp/ftp_commands.c?revision=522706&view=markup

Re: Patch for implementing ap_document_root as a hook

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Brian J. France wrote:
> 
> On Apr 23, 2007, at 10:32 AM, Jakob Goldbach wrote:
> 
>>> -1 on the face of things.  The map_to_storage hook was added to
>>> accomplish what you desire.
>>
>> I thought map_to_storage was made to do per-dir configuration. Not
>> path-translation.

Actually it does both.  An amusing use case is



>> The problem is not really doing the translation. I can always provide
>> my own translate handler in my module.
>> But in the current API I cannot to set my env. variables at will. They
>> will be overwritten by ap_add_common_vars which returns
>> carved-in-stone docroot from ap_document_root.
> 
> We need this same functionality (would like to back port to 2.2 if
> possible).
> 
> We currently hack the doc root in the post read hook in 1.3, would like
> to be able to do it with out hacking the core and screwing around with
> internal structs at runtime.

I don't quite see this happening, but maybe.  If the docroot could be
cached from the translate_name/map_to_storage phase, you could obviously
abuse it as necessary.

>> I thought a document_root hook was more elegant. Or maybe a
>> add_common_vars hook? [I would be happy to supply it]

***ding ding ding*** - hooking add_common_vars is something I could get
behind.

>> What's so bad about ap_document_root?  I know the source says 'dont
>> use' because it won't be right with mod_userdir etc. But with a hook
>> it would be possible to get right.

What is the docroot of http://example.com/user/webapp/login - is it the
path to /user/webapp/ or somewhere in the admin's htdoc?  I find the
concept fundamentally flawed :)

Re: Patch for implementing ap_document_root as a hook

["Brian J. France" <li...@firehawksystems.com>]

On Apr 23, 2007, at 10:32 AM, Jakob Goldbach wrote:

>> -1 on the face of things.  The map_to_storage hook was added to  
>> accomplish
>> what you desire.
>
> I thought map_to_storage was made to do per-dir configuration. Not
> path-translation.
>
> The problem is not really doing the translation. I can always provide
> my own translate handler in my module.
>
> But in the current API I cannot to set my env. variables at will. They
> will be overwritten by ap_add_common_vars which returns
> carved-in-stone docroot from ap_document_root.

We need this same functionality (would like to back port to 2.2 if  
possible).

We currently hack the doc root in the post read hook in 1.3, would  
like to be able to do it with out hacking the core and screwing  
around with internal structs at runtime.

Brian




> My only other option is to patch every single module which calls
> add_common_vars, that is,  cgi,cgid, fastcgi, includes,...
>
> I thought a document_root hook was more elegant. Or maybe a
> add_common_vars hook? [I would be happy to supply it]
>
>> Unfortunately it is not coupled to the DOCUMENT_ROOT
>> variable, but I'd look at remedying this over building on  
>> ap_document_root,
>> which should simply go away, IMHO.
>
> What's so bad about ap_document_root?  I know the source says 'dont
> use' because it won't be right with mod_userdir etc. But with a hook
> it would be possible to get right.
>
> /Jakob

Re: Patch for implementing ap_document_root as a hook

[Jakob Goldbach <ja...@gmail.com>]

> -1 on the face of things.  The map_to_storage hook was added to accomplish
> what you desire.

I thought map_to_storage was made to do per-dir configuration. Not
path-translation.

The problem is not really doing the translation. I can always provide
my own translate handler in my module.

But in the current API I cannot to set my env. variables at will. They
will be overwritten by ap_add_common_vars which returns
carved-in-stone docroot from ap_document_root.

My only other option is to patch every single module which calls
add_common_vars, that is,  cgi,cgid, fastcgi, includes,...

I thought a document_root hook was more elegant. Or maybe a
add_common_vars hook? [I would be happy to supply it]

> Unfortunately it is not coupled to the DOCUMENT_ROOT
> variable, but I'd look at remedying this over building on ap_document_root,
> which should simply go away, IMHO.

What's so bad about ap_document_root?  I know the source says 'dont
use' because it won't be right with mod_userdir etc. But with a hook
it would be possible to get right.

/Jakob

Re: Patch for implementing ap_document_root as a hook

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Jakob Goldbach wrote:
> Hi,
> 
> Attached is a patch which implements ap_document_root(r) as a hook.
> This way modules can set document_root on the fly. (think vhost_alias)
> AND get the right DOCUMENT_ROOT env. variable (as set by
> ap_add_common_vars(r)).
> 
> The patch also changes ap_core_translate to use ap_document_root(r)
> instead of conf->ap_document_root.  This way, modules that just need
> to point to a different docroot won't have to implement a translate
> hook by appending r->uri til r->filename, but just rely on
> ap_document_root.

-1 on the face of things.  The map_to_storage hook was added to accomplish
what you desire.  Unfortunately it is not coupled to the DOCUMENT_ROOT
variable, but I'd look at remedying this over building on ap_document_root,
which should simply go away, IMHO.