[Seaside-dev] session tracking refactoring: all tests green

[Philippe Marschall]

2011/7/19 Julian Fitzell <jfitzell at gmail.com>:
> On Mon, Jul 18, 2011 at 1:40 PM, Philippe Marschall
> <philippe.marschall at gmail.com> wrote:
>> 2011/7/18 Julian Fitzell <jfitzell at gmail.com>:
>>> On Sun, Jul 17, 2011 at 1:47 PM, Philippe Marschall
>>> <philippe.marschall at gmail.com> wrote:
>>>> Hi
>>>>
>>>> I'm happy to report that the session tracking refactoring in the WIP
>>>> repository [1] is mostly done. Have a look at
>>>
>>> I'll take a look.
>>>
>>>> WAHandlerTrackingStrategy to get started. The missing things are:
>>>>  - more tests
>>>>  - class comments
>>>>  - WARegistryKeyHandlingTest and WAApplicationKeyHandlingTest need to
>>>> be refactored, there are some nonsensical tests there like two
>>>> sessions, one identified by a query field and one by a cookie
>>>
>>> Why is that a nonsensical test? It happens any time someone clicks on
>>> a link/bookmark and also has a cookie from a previous session hanging
>>> around...
>>
>> If the user agent accepts cookies the session key will not show up in
>> the URL. Only the very first link he clicks linking to an action page
>> will have it but the following render page won't. So it won't show up
>> the address bar. Also we probably do the wrong thing. We should
>> probably take the session from the cookie. However we currently give
>> preference to the query field because document handlers are tracked
>> using query fields.
>
> Our logic, which I still think is sound, is that *if* you pass in a
> session key on the URL you are asking to override what is in the
> cookie. I agree that in common usage that's not going to come up very
> often, but in cases where both are present in a request I still think
> priority should be given to the query field; the user has requested a
> specific resource at a specific URL that includes a session key, it
> seems weird to ignore that key if the session actually exists (and
> note that if it doesn't, we should actually maybe return a redirect
> response to a URL without the session key, which could then be handled
> using the cookie).

The test is still there. I just moved it from registry tests to
application tests because only sessions session support cookies and

>>>>  - configuration with the admin UI is broken, see previous mail
>>>>
>>>> A small side effect is that WARegistry uses '_s' as well to track
>>>> things instead of '_key'. Since it isn't used anyway I didn't bother
>>>> to preserve this behavior. This obviously breaks all users of
>>>
>>> I don't think that should be hardcoded. First, the _s clearly implies
>>> "session"; second, registries could easily be nested so having a
>>> hardcoded key sucks.
>>
>> For the last then years only _s has ever been used. Even for document
>> handlers. If you wanted something else you had to subclass
>> WAApplication. With the new solution you still have to subclass, just
>> the strategy instead of the application. So nothing really changed.
>>
>> I figured that in the hypothetical case when you wanted some other key
>> subclassing a strategy class and overriding a method is preferable to
>> introducing a configuration option that nobody ever uses.
>
> I'm not suggesting we need a configuration option. An instance
> variable is just fine. Hardcoding something arbitrary like '_s',
> particularly in a new more generic class, seems really silly to me.
> Initialize it with '_s' and leave the option open...

Has been '_s' for ten years, nobody ever wanted to change it. And we
initialize the instance variable with what? #keyFieldName from
registry? So if you want to change it you need to subclass registry
instead of strategy which buys you exactly what?

Cheers
Philippe