Re: cvs commit: httpd-2.0/modules/http mod_mime.c
[EMAIL PROTECTED] wrote: >jerenkrantz2002/09/05 00:31:14 > > Modified:.CHANGES > modules/http mod_mime.c > Log: > Add ModMimeUsePathInfo directive. > > +int use_path_info;/* If set to 0, only use filename. > + * If set to 1, append PATH_INFO to filename for > + * lookups. > + * If set to 2, this value is unset and is > + * effectively 0. > + */ > -1 don't use magic numbers please use a #define > > +new->use_path_info = 2; > + > > >
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
On Thu, Sep 05, 2002 at 06:43:52AM -0700, Ian Holsman wrote: > -1 > don't use magic numbers > please use a #define Sorry, but the rest of the server uses this strategy for flags. Look specifically at IdentityCheck and ContentDigest. The problem is that the AP_INIT_FLAG and ap_set_flag_slot passes in 0, 1 and therefore that must correspond to Off and On. Using a local enum or a #define will break compatibility with how flags are handled (because some genius may think they can change those values). Therefore, I believe we have to stick with magic numbers. And, due to directive inheritance, there is also needed a special value for 'unset' - and the strategy we have used in the past is to use 2. If you think it'd be better to rewrite all of the flag handling code to use global enums, I'd agree, but I'm not going to do that. That will break all modules using flags. -- justin
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
Ian Holsman wrote: > > [EMAIL PROTECTED] wrote: > > >jerenkrantz2002/09/05 00:31:14 > > > > Modified:.CHANGES > > modules/http mod_mime.c > > Log: > > Add ModMimeUsePathInfo directive. > > > > +int use_path_info;/* If set to 0, only use filename. > > + * If set to 1, append PATH_INFO to filename for > > + * lookups. > > + * If set to 2, this value is unset and is > > + * effectively 0. > > + */ > > > -1 > don't use magic numbers > please use a #define better yet, an enum. Greg
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
I'm in complete agreement with Justin on this one.
"Add" says "add" to me. And filters *are* additive.
I wouldn't agree with Joshua's comments about tossing filter directives and
rely on each module to provide their own (how would you order them?), but I
think his meta-comment about "this stuff is confoozled" applies. A step back
and a rethink is probably necessary, rather than poking and prodding at the
various config directives.
Cheers,
-g
On Sun, Sep 02, 2001 at 09:05:50PM -0700, Justin Erenkrantz wrote:
> On Sun, Sep 02, 2001 at 10:49:52PM -0500, William A. Rowe, Jr. wrote:
> > Not this way. No other mod_mime variable behaves the way you you are trying.
> > I'm not kidding about adding a Set{Input|Output}FilterByType/SetHandlerByType
> > so when we ask folks to rely upon mime types, they can actually do so.
>
> And, that's additive?
>
> So, I could do:
>
> SetOutputFilterByType BAR text/*
> SetOutputFilterByType FOO text/plain
>
> As a user, I *expect* that both filters are activated.
>
> I think you make it sound like we have to do:
>
> SetOutputFilterByType BAR text/*
> SetOutputFilterByType BAR;FOO text/plain
>
> Yuck, yuck, yuck, yuck, yuck. (Did I mention I think this is yucky?)
>
> > Yes... please read mod_mime.html. AddSomething is not additive, and can't be.
> > The server config is often a nightmare to grok as things stand today. Don't make
> > things harder by absusing fairly consistant definitions such as AddSomething or
> > SetSomething. The inner container always overrides the outer.
>
> As a user, I expect that to be additive *in the case of a filter*. I
> expect it to override in the other cases - just not this case. You
> can't have multiple handlers, but you can certainly have multiple
> filters.
>
> > So the inner needs AddOutputFilter FOO;BAR html - as of today. I suggested an
> > entire +|- syntax as well, it was somewhat booed since existing +|- syntaxes are
> > perceived as confusing. Here, well I think it's necessary.
>
> That's confusing. I think the cleanest way is for it to be additive
> (with a RemoveOutputFilter to remove one from a higher level -
> ignoring this directive if the filter doesn't exist from a prior
> level).
>
> > None of this is addressing filter ordering across commands yet. I said 8 months
> > ago we've done an inadequte job of defining the use-filters syntax. I'm saying
> > the very same thing today.
>
> Yeah, I expect that the ordering of filter execution isn't going to be
> right given the code we have now. -- justin
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
On Sun, 2 Sep 2001, Justin Erenkrantz wrote: > Yuck, yuck, yuck, yuck, yuck. (Did I mention I think this is yucky?) Let me propose something way out in left field. Feel free to shoot it down. Problem: filters configuration is damn confusing. I only have time to skim new-httpd (err, dev@httpd), but I've completely lost track of how to configure filters. How do they inherit from one context to another? How are they ordered? Where can they be used? What is the syntax? I can only imagine how confused the users are going to be. Solution: make filters a back-end concept that is not directly accessible to users. Remove all user-level filter controls. Any module that provides a filter must also provide configuration directives to activate it (ie, IncludesFilter On). Perhaps this is just a straw-man, but unless someone is planning to write one helluva good set of docs for this stuff, I pity the people trying to use it. [By the way, I retract any objections to OtherBills proposed changes. I don't understand this stuff well enough to have an informed opinion.] Joshua.
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
On Sun, Sep 02, 2001 at 10:49:52PM -0500, William A. Rowe, Jr. wrote:
> Not this way. No other mod_mime variable behaves the way you you are trying.
> I'm not kidding about adding a Set{Input|Output}FilterByType/SetHandlerByType
> so when we ask folks to rely upon mime types, they can actually do so.
And, that's additive?
So, I could do:
SetOutputFilterByType BAR text/*
SetOutputFilterByType FOO text/plain
As a user, I *expect* that both filters are activated.
I think you make it sound like we have to do:
SetOutputFilterByType BAR text/*
SetOutputFilterByType BAR;FOO text/plain
Yuck, yuck, yuck, yuck, yuck. (Did I mention I think this is yucky?)
> Yes... please read mod_mime.html. AddSomething is not additive, and can't be.
> The server config is often a nightmare to grok as things stand today. Don't make
> things harder by absusing fairly consistant definitions such as AddSomething or
> SetSomething. The inner container always overrides the outer.
As a user, I expect that to be additive *in the case of a filter*. I
expect it to override in the other cases - just not this case. You
can't have multiple handlers, but you can certainly have multiple
filters.
> So the inner needs AddOutputFilter FOO;BAR html - as of today. I suggested an
> entire +|- syntax as well, it was somewhat booed since existing +|- syntaxes are
> perceived as confusing. Here, well I think it's necessary.
That's confusing. I think the cleanest way is for it to be additive
(with a RemoveOutputFilter to remove one from a higher level -
ignoring this directive if the filter doesn't exist from a prior
level).
> None of this is addressing filter ordering across commands yet. I said 8 months
> ago we've done an inadequte job of defining the use-filters syntax. I'm saying
> the very same thing today.
Yeah, I expect that the ordering of filter execution isn't going to be
right given the code we have now. -- justin
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
From: "Justin Erenkrantz" <[EMAIL PROTECTED]>
Sent: Sunday, September 02, 2001 10:40 PM
> On Sun, Sep 02, 2001 at 10:17:46PM -0500, William A. Rowe, Jr. wrote:
> > Thank you, first off, for catching my foobar.
> >
> > But -1 on the solution. This is contrary to the syntax of AddSomething.
> > AddSomething in mod_mime (and SetSomething in core) always _replaces_ the
> > prior declaration!!!
>
> If I setup mod_bar's filter on a per-server level for a mime-type and
> then I also want the same mime-type in a directory to be handled by
> mod_foo's filter as well, I want *both* filters to be executed. How
> do you propose to handle that?
Not this way. No other mod_mime variable behaves the way you you are trying.
I'm not kidding about adding a Set{Input|Output}FilterByType/SetHandlerByType
so when we ask folks to rely upon mime types, they can actually do so.
> The easiest way I see to configure this is:
>
> AddOutputFilter BAR html
>
> AddOutputFilter FOO html
>
>
> Maybe I'm missing something here. -- justin
Yes... please read mod_mime.html. AddSomething is not additive, and can't be.
The server config is often a nightmare to grok as things stand today. Don't make
things harder by absusing fairly consistant definitions such as AddSomething or
SetSomething. The inner container always overrides the outer.
So the inner needs AddOutputFilter FOO;BAR html - as of today. I suggested an
entire +|- syntax as well, it was somewhat booed since existing +|- syntaxes are
perceived as confusing. Here, well I think it's necessary.
None of this is addressing filter ordering across commands yet. I said 8 months
ago we've done an inadequte job of defining the use-filters syntax. I'm saying
the very same thing today.
Bill
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
On Sun, Sep 02, 2001 at 10:17:46PM -0500, William A. Rowe, Jr. wrote: > Thank you, first off, for catching my foobar. > > But -1 on the solution. This is contrary to the syntax of AddSomething. > AddSomething in mod_mime (and SetSomething in core) always _replaces_ the > prior declaration!!! If I setup mod_bar's filter on a per-server level for a mime-type and then I also want the same mime-type in a directory to be handled by mod_foo's filter as well, I want *both* filters to be executed. How do you propose to handle that? The easiest way I see to configure this is: AddOutputFilter BAR html AddOutputFilter FOO html Maybe I'm missing something here. -- justin
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
> > AddOutputFilter GZ html (server-level) > > AddOutputFilter Includes html (directory-level) I think what you want is AddOutputFilter Includes html. SetOutputFilterByType gz text/* That syntax doesn't exist... yet :) Bill
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
Thank you, first off, for catching my foobar.
But -1 on the solution. This is contrary to the syntax of AddSomething.
AddSomething in mod_mime (and SetSomething in core) always _replaces_ the
prior declaration!!!
We are going to have 10,000,000 webserer users three years from now asking
why don't they successfully replace their filters!
The effects from multiple extensions may be cumulative (.shtml.gz, for example.)
The replacement of a given extension may not be.
We have to remain consistent. I'll patch this Tuesday if you don't beat me to it.
Bill
From: <[EMAIL PROTECTED]>
Sent: Sunday, September 02, 2001 3:34 AM
> jerenkrantz01/09/02 01:34:45
>
> Modified:modules/http mod_mime.c
> Log:
> We should be copying the filters as well when we perform the extension merge
> (which seems to use a ; syntax).
>
> Try:
> AddOutputFilter GZ html (server-level)
> AddOutputFilter Includes html (directory-level)
>
> Oops.
>
> Without this, when you use mod_gz and go to /foobarnotthere/, the error
> page isn't handled by mod_include. Oooops.
>
> Revision ChangesPath
> 1.62 +18 -0 httpd-2.0/modules/http/mod_mime.c
>
> Index: mod_mime.c
> ===
> RCS file: /home/cvs/httpd-2.0/modules/http/mod_mime.c,v
> retrieving revision 1.61
> retrieving revision 1.62
> diff -u -r1.61 -r1.62
> --- mod_mime.c 2001/08/30 04:11:57 1.61
> +++ mod_mime.c 2001/09/02 08:34:45 1.62
> @@ -191,6 +191,24 @@
>if (overlay_info->charset_type) {
>base_info->charset_type = overlay_info->charset_type;
>}
> +if (overlay_info->input_filters) {
> +/* We need to concat the filters */
> +if (base_info->input_filters)
> +base_info->input_filters = apr_pstrcat(p,
> +base_info->input_filters, ";",
> +overlay_info->input_filters, NULL);
> +else
> +base_info->input_filters = overlay_info->input_filters;
> +}
> +if (overlay_info->output_filters) {
> +/* We need to concat the filters */
> +if (base_info->output_filters)
> +base_info->output_filters = apr_pstrcat(p,
> + base_info->output_filters, ";",
> + overlay_info->output_filters, NULL);
> +else
> +base_info->output_filters = overlay_info->output_filters;
> +}
>}
>else {
>apr_hash_set(base, key, klen, overlay_info);
>
>
>
>
