[dev] Horde_Session thoughts

[Chuck Hagenbuch]

Quoting Michael M Slusarz <slusarz at horde.org>:

> 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.

I'm fine with all of this. Thank you. On the globals I will say that  
no one is rushing to just delete all of them; we'll see how things are  
as we progress.

-chuck