Re: [PATCH] stage 2 of store url as fake store_url helper

On 2/12/2012 7:22 p.m., Eliezer Croitoru wrote:
> On 12/1/2012 9:27 PM, Alex Rousskov wrote:
>> On 12/01/2012 06:34 AM, Eliezer Croitoru wrote:
>>
>> BTW, to compute the first parameter, you may be able to just call
>> HttpRequest::storeUrl() method discussed above (if it always returns a
>> URL for Store).
> Well the question here is how the http->uri is computed:
> I am not sure that always the uri and canonical uri that I am using on
> the request are the same.
> I will check it but if someone knows I will wait for the word.

I've done some separate work on cleaning up the URL handling. As far as
I could tell from that http->url is/was supposed to be the canonical URL
for the request from before any adaptation happened. For logging the
client request-line. As such it seems far safer to use the HttpReqeust
URL via urlCanonical() whenever possible, that way you pick up any
changes the URL-rewrite and adaptation did.

>>
>> Finally, I suggest using "store ID" instead of "store URL" in all
>> internal names and code comments. It is a good idea to remind us that
>> there is nothing "URL" about this opaque string.
>>
>> I would even argue that squid.conf directive should be called
>> differently to emphasize that this is a URL:ID mapping feature rather
>> than URL rewriting feature (I do not think we need to maintain backward
>> compatibility with Squid2 unfortunate naming here).
> It's fine by me but I am almost sure that backwards compatibility is
> wanted.
> There was a change about the redirector and url_rewrite in the past if
> I'm right.
> This kind of a change will remind the admins that the feature works
> differently from the old store_url_rewrite.

Don't worry about that. Just place the old directive name as a second
word on the NAME: line in cf.data.pre. The config generator will take
care of the upgrade notices etc and accept both names. The code in
redirect.cc is already taking care of helper protocol backward
compatibility.

Amos
Received on Sun Dec 02 2012 - 06:41:52 MST