Re: [PATCH] url_rewrite_extras for redirector helpers

On 03/13/2014 06:06 AM, Amos Jeffries wrote:
> On 25/02/2014 5:53 a.m., Tsantilas Christos wrote:
>> On 02/24/2014 01:40 AM, Amos Jeffries wrote:
>>> On 2014-02-24 10:12, Tsantilas Christos wrote:
>>>> Hi all,
>>>>
>>>> This patch add the url_rewrite_extras for redirector helpers.
>>>> The url_rewrite_extras is a "quoted string" with logformat %macro
>>>> support. It is appended to request line for redirector helpers.
>>>>
>>>> Example usage:
>>>> url_rewrite_extras "Note1=%{Note1}note Note2=%{Note2}note"
>>>>
>>>> The url_rewrtite_extras it is similar to the "key_extras" authenticator
>>>> helpers options. Originally developed to allow notes exchange between
>>>> authenicator helpers and redirector helpers.
>>>>
>>>> In this patch we add a new type for cf.data the TokenOrQuotedString to
>>>> allow new configurations use quoted strings for the new option without
>>>> explicitly turning configuration_includes_quoted_values on.
>>>>
>>>> This is a Measurement Factory project.
>>>
>>>
>>> in src/cf.data.pre:
>>> * why are you calling this new directive an alias of a non-existent
>>> "format" directive?
>>> "
>>> +NAME: url_rewrite_extras format
>>> "
>>>

I have already commit the patch.
Sorry.

>
> You still have the above issue. Looking at the way you added it to
> storeid_extras as well it looks like you are completely misunderstanding
> the syntax of the NAME: entry.
>
> NAME: a b c
>
> ... means that directive name 'a' is the current official squid.conf
> name but this directive has previously been known by the names 'b' and
> 'c'. The code is *also* able to auto-convert between syntax of a, b, and c.
> NP: if the auto-convert is not possible (such as splitting an existing
> direcive in two like your patch is implying about directive "format")
> then the "TYPE: obsolete" mechanism is used instead of aliases to give
> instructinos on replacing the old directive.
>
> There has never been a "format" directive and it certainly is not
> deprecated by two separate new directives. So please remove the extra
> "format" words in cf.data.pre.

True...
It should be fixed....

>
>
> Also I see some more:
>
> in src/redirect.cc
> * new ::Format::Format("redirecor_extras");
> - s/redirecor_extras/url_rewrite_extras/
>
> * new ::Format::Format("storeId_extras");
> - s/storeId_extras/store_id_extras/

Looks that "Format::Format" name is used to inform users about
configuration problems, so it should be fixed too.

>
>
> Those can be done on commit. +1 conditional on the above change.

I have apply the patch to trunk. Should I uncommit the patch or apply a
second one with fixes?

>
> Amos
>
Received on Thu Mar 13 2014 - 09:07:55 MDT