[dev] Registry_Application's prefs methods flawed?

[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>:
>>
>>> While fixing some issue with populating preference values in  
>>> Kronolith, I noticed that the prefs methods in the  
>>> Horde_Registry_Application sub-classes uses pref groups  
>>> ($ui->group) to determine which preference to manipulate  
>>> ($ui->override). This seems flawed because the individual prefs  
>>> can be assigned different groups in the config/prefs.php  
>>> configuration files.
>>
>> This is expected, and it is a compromise.  It is not possible to  
>> move things around between pref groups easily at this time because  
>> the conditional logic is hardcoded in the App_Registry_Application  
>> file, as you discovered.  This was a compromise since the  
>> alternative would be to have all of this conditional code living in  
>> prefs.php instead, or (more likely) having each individual  
>> preference be overloaded with configuration options.  Or, even more  
>> extreme, have each preference live in a separate class/object.   
>> Unfortunately, prefs are one of those things that need to be  
>> exposed to ALL admins so we have to keep the API somewhat workable.
>>
>> IMHO, we really shouldn't be having prefs appear on multiple  
>> screens anyway.  It just adds to the prefs clutter and is  
>> duplicative and confusing.  If its one of those things where one  
>> pref might control prefs living in a different group, then we are  
>> probably Doing It Wrong; these prefs should logically be broken  
>> into and a new, separate group.  I would rather see usage of  
>> linkage between groups pages via links rather than a duplicated  
>> pref entry.  And some prefs simply can't be split up (identity  
>> prefs) because they rely on other elements in the page (javascript;  
>> specific handling based on the group name).
>>
>> Conversely, if you really do want to have prefs appear in multiple  
>> groups, then you can simply add the necessary code to deal with  
>> that individual pref in the App_Registry_Application methods.  The  
>> valid list of prefs for the current group can always be obtained via:
>> $ui->getChangeablePrefs($ui->group)
>
> I wasn't actually referring to having prefs in several groups,  
> though this obviously experience the same problems I described. I  
> referred to the admin's capability to assign a pref to a different  
> group than originally intended.
>
> Regarding identities the idea was (and I actually thought this  
> already happened during one of the pref rewrites) that the identity  
> stuff is flexible enough to theoretically allow adding and removing  
> any prefs to/from identities. Of course this would still require  
> some code that actually uses the identity to receive a pref value,  
> instead of retrieving it from the prefs object directly. But this  
> could be a custom application for example.

This is still the case.  What I am saying is that identities prefs  
MUST live in the identities group, because they are handled  
differently by the Prefs UI handling code.  e.g. The identities page  
automatically inserts the identity switching JS code into the page -  
this would get VERY complicated/messy if you would allow identities  
prefs to appear in any group.

> Back to grouping prefs: I think it makes a lot of sense to have some  
> preferences in multiple groups. It was actually me who did this in  
> the first place because some users are expecting prefs in a  
> different group than others. Beside that, it simply makes sense to  
> access certain prefs from different places. Most prominent and  
> obvious examples are the special folders. It makes perfectly sense  
> to have a group where all special folders can be managed, while  
> still being able to set the trash folder from the group for deleting  
> messages.

I don't necessarily disagree with you, but the other solution (that  
makes more sense to me, btw) is to move each mailbox pref into a  
different group.  Thus, all preference options dealing with the trash  
folder (trash mailbox location, use virtual trash?, login tasks to  
purge the trash mailbox) should appear in a single Trash preference  
group.

> From the 3 possible approaches you outlined in your first paragraph,  
> the second one sounds like the one we want to use, I don't really  
> understand why you don't deem that one appropriate. After all this  
> happens in lib/Application.php which is not a place for admin's to  
> go, and whether we identify special handling for prefs by pref group  
> or pref name doesn't seem to make much of a difference.

I think you are referring to "conditional code living in prefs.php  
instead, or (more likely) having each individual preference be  
overloaded with configuration options."  And I'm not sure how that  
would work(?)  We should never be conditionally excluding individual  
prefs entries with if () blocks.  And for various prefs/groups, we  
don't know whether to show/hide prefs until AFTER we parse the  
prefs.php file.  So that's not going to work unless we start doing  
something like defining functions to run post-parsing.  And all this  
does is completely clutter up the preferences file.  My intent for  
prefs.php is to keep it as simple as possible - I am for any solution  
to allow this over the ability to let an admin switch pref groups  
(which few admins will ever do).

michael

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