[dev] Horde_Session thoughts

[Michael M Slusarz]

Quoting Jan Schneider <jan at horde.org>:

> Zitat von Michael M Slusarz <slusarz at horde.org>:
>
>> Quoting Jan Schneider <jan at horde.org>:
>>
>>> Zitat von Michael M Slusarz <slusarz at horde.org>:
>>>
>>>>  Allow ability to search session subkeys.
>>>>
>>>>  We store values with key & subkey together to save on  
>>>> storage/serialization
>>>>  costs. To retrieve array like output, simply need to call
>>>>  $session['app:name/'] - returns an array with subkeys as keys and session
>>>>  values as values.
>>>
>>> This sounds like a bit too much magic for a small performance  
>>> gain. I'd rather like this to be a real array for  
>>> readability/maintainability, and loose a bit of performance. Can't  
>>> be too much anyway.
>>
>> Disagree.  I personally am finding that storing something with a  
>> session index like:
>> ['imp:imp_mailbox/INBOX']
>> is *much* clearer (since all the pertinent information is in the  
>> same string and not separated by PHP-ish cruft) in the code than:
>> ['imp']['imp_mailbox']['INBOX']
>> not to mention easier to write and less prone to typos/errors.
>>
>> Especially since I have only found *1* instance in the code so far  
>> where we were actually using the iterative/collective property of  
>> arrays (the horde:auth_app variable).  In every other instance, we  
>> are carrying around the overhead of an array data structure solely  
>> for the fact that it might make things a bit clearer in the code -  
>> but, as I argue above, that is not true anymore either.
>>
>> But nothing I have added will prevent anybody from storing arrays  
>> if they want.
>
> I feel rather strong about this being counter-intuitive. Any other  
> opinions about this?

My (belated) take on this...

I have very strong feelings that the way I have implemented is *much*  
preferable - and these opinions are because I have spent 5+ hours  
converting things and tweaking the API syntax to make things as easy  
as possible to both code, read, and debug.  But I need to pick my  
battles and since everyone else seems to be against it, I'm going to  
concede this issue for now.

Well... only somewhat concede because there have been several other  
comments about implementation changes that are DEFINITELY not  
happening.  Hopefully I touch on all points below

1. API

The API must be as follows:
$horde_session->get($app, $name[, $options_mask])
$horde_session->set($app, $name, $value[, $options_mask])
$horde_session->remove($app, $name)

Writing this out, it is painfully apparent how much cleaner the array  
syntax is.  E.g.,
$horde_session['app:foo'] = $value;
vs.
$horde_session->set('app', 'foo', $value);
But I digress...

Most important, and this goes for classes/objects other than  
Horde_Session, you absolutely CAN NOT USE MAGIC PROPERTY ACCESS  
METHODS (i.e. $horde_session->variable name).  First, in the  
Horde_Session case, it can't be used since session data is not a  
simple key->value lookup.  It is stored by app name.  You could  
theoretically do fancy manipulation by stacking objects on top of  
objects, (i.e. $horde_session->horde->variable_name) but what's the  
point besides adding a bunch of unneeded complexity?  And you can't  
pass options using this syntax.

Most important, using magic properties is simply a terrible design  
decision for data objects where the key name is entirely dynamic, for  
the obvious reason that the possible keys are limited.  This was  
precisely why I used array access in the first place - to prevent this  
limitation.  The only time using __get() makes sense for dynamic  
property accesses is when the keys are already defined and limited  
(see, e.g., IMP_Mailbox_Element - property access makes sense here to  
prevent us from having to write 30 mini-methods of 1-2 lines each to  
return a dynamic value).

This is precisely why I added an array access possibility to  
Horde_Prefs, but there can not be a parallel object property access API.

2. Storage in the session needs to be as flat as possible.

Don't have the link in front of me, but read somewhere recently that a  
PHP array with a single element requires 300+ bytes (as opposed to  
something like 68 bytes for a scalar value).  So deep arrays need to  
be avoided at all costs.

Through my process of converting so far, my conclusion is that there  
is absolutely no reason that 99% of session variables need to be  
living in an array format.  The vast majority of session variables are  
scalar values and don't need to be living in an array.

Debugging is not a valid argument against this.  If we need a way to  
pretty print the current session, it is trivial to add a debug method  
to Horde_Session.

And this argument is moot anyway, since actual storage format can be  
easily changed in the future once all session reading/writing is  
filtered through Horde_Session.

This leads to...

3. For technical reasons, DIRECT ARRAY ELEMENT ACCESS ISN'T POSSIBLE ANYWAY

This is no longer possible:
$_SESSION['app']['foo']['bar']['enabled'] = 1;

Nothing you can do about it.  This MUST be done this way (with the  
proposed API from above):
$a = $horde_session->get('app', 'foo');
$a['bar']['enabled'] = 1;
$horde_session->set('app', 'foo', $a);

This is a technical limitation due to the fact that we need to track  
if a session element has been altered in order to determine if we need  
to update the session data on shutdown.  Right now we use md5()  
checksums to do this check, but the new method is magnitudes faster  
(just like using md5 checksums is magnitudes faster than writing to  
the session storage backend on every pageload, even if session data  
doesn't change).

As a compromise to limiting array access, this is why I added...

4. The *EXTREMELY* useful 'varname/subname' format

There are certain situations where it is useful to allow at least a  
basic key->value lookup for certain data items.  For example, the  
authentication data stored in horde.  It is much clearer code to store  
things as 'auth/appname' than creating a bunch of individual keys like  
'auth-appname'.  More important, it gives us the ability to later be  
able to work with these data elements as a whole - like if we wanted  
to reset all authentication tokens at once.  Thus the need for a  
shorthand format to allow us to do this.

I really don't see how this is "counter-intuitive".  If you don't like  
this feature, don't use it.  It doesn't affect anything else, while  
providing shortcuts to allow iteration and easy removal.

I will reserve comment on removing our Horde globals for another time  
other to see I _deeply_ disagree with that decision.  I have  
absolutely no problem with providing a small number (for us it is 10  
or less) of globally available variables for items that are  
universally used.  These globals are available only to applications.   
Asking an application developer to start to have to inject EVERYTHING  
into a page seems to be a GIANT step backwards in terms of making the  
framework easier to use.  There is absolutely nothing wrong with  
providing intelligent shortcuts to make coding easier.  This idea  
seems more like a quest for perfect OO-coding purity than a decision  
to make actual coding better.

I will start work on moving converted code to the new API.

michael

-- 
___________________________________
Michael Slusarz [slusarz at horde.org]