[dev] Horde_Session thoughts

[Jan Schneider]

Zitat von Chuck Hagenbuch <chuck at horde.org>:

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

I don't buy this argument. We already do something similar with the  
Registry which works just fine, and with your original array access,  
you haven't been able to pass options either. I'd like to use magic  
getters/setters at least optionally, e.g. where we don't need to pass  
options.

>> 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
>
> -- 
> Horde developers mailing list - Join the hunt: http://horde.org/bounties/
> Frequently Asked Questions: http://horde.org/faq/
> To unsubscribe, mail: dev-unsubscribe at lists.horde.org
>



Jan.

-- 
Do you need professional PHP or Horde consulting?
http://horde.org/consulting/