Re: [patch] ($r|$s)->is_hook_enabled($hook_name)

Geoffrey Young wrote:
> > > > So first of all I've called the method: is_hook_enabled
> > > > and it checks srv config flags when called as 
> > > > $s->is_hook_enabled($hook_name) or dir config flags if called as 
> > > > $r->is_hook_enabled($hook_name).
> > > >
> > > > Is that a healthy API? Or should we use:
> > > >
> > > >     my $dir_cfg = $self->get_config($s, $r->per_dir_config);
> > > >     my $srv_cfg = $self->get_config($s);
> > > >
> > > > add call:
> > > >
> > > >     $dir_cfg->is_hook_enabled($hook_name);
> > > >     $srv_cfg->is_hook_enabled($hook_name);
> > > >
> > > > anyways, here is the patch of the current implementation and tests:
> > >
> > >
> > >
> > >
> > > Thinking more about it, and since we already change the API, it 
> > > should probably be called is_option_enabled, or even 
> > > is_perl_option_enabled so not to confuse with native Options, which 
> > > we may provide an api for later on, now that we have a parsed config 
> > > tree. So we will have:
> > >
> > > is_perl_option_enabled('ParseHeaders'); # for PerlOptions +ParseHeaders
> > > is_option_enabled('FollowSymlinks');    # for Options FollowSymlinks
> >
> >
> >
> > Any comments on this API change/extending?
>
>
> :)
>
> personally, I've become a fan of making mod_perl users think in Apache 
> terms, so I'd prefer the more verbose route (via per_dir_config) here 
> rather than the $r->is_hook_enabled() way.  in fact, I'd be more partial 
> to something like
>
> my $follow_symlinks =
>   $self->get_config($s, $r->per_dir_config)
>        ->get('Options')
>        ->get('FollowSymlinks');
>
> and do away with the is_option_enabled/is_perl_option_enabled stuff
>
> but this is all just handwaving on my part - no patches here :)

I think we should go with get_config first of all, so we have the Apache like 
functionality and later on we can provide a shortcut with syntax sugar.

So we start with:
my $dir_cfg = $self->get_config($s, $r->per_dir_config);
my $srv_cfg = $self->get_config($s);

And then we can have something like headers_out-like (APR::Table) interface, 
though read-only.

$parse_headers_is_on = $dir_cfg->perl_options->get('ParseHeaders');
$parse_headers_is_on = $dir_cfg->perl_options->{'ParseHeaders'};
%srv_perl_options = %{ $srv_cfg->perl_options() };

What do you think?

__________________________________________________________________
Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/     mod_perl Guide ---> http://perl.apache.org
mailto:stas@xxxxxxxxxx http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com