Re: [Postfixadmin-devel] Merging model/ and scripts/models-ext/

[Christian B. <pos...@cb...>]

Hello,

Am Samstag, 27. November 2010 schrieb Valkum:
> nice to see that something happened with my handlers.
> 
> I hope that you noticed that i added a config class to stay away from
> global vars.

Yes, I've seen it and had a very quick look at the code, but didn't 
really read all of it.

Just two questions:

a)
Lots of config options are boolean (yes/no). It would probably make 
sence to include the boolconf() function (currently in 
functions.inc.php) in the config class

b)
If I get function write right, you allow to edit the config. Is this 
really needed? IMHO it should be read-only, nothing in the code should 
be allowed to change what the config file provides.

When grepping the code, I didn't find many files that contain 
$CONF[whatever] = newvalue (with the exception of config.inc.php ;-)

index.php:$CONF['configured'] = FALSE;
-> directly before including config.inc.php -> harmless

setup.php:$CONF['show_header_text'] = 'NO';
setup.php:$CONF['theme_logo'] = 'images/logo-default.png';
setup.php:$CONF['theme_css'] = 'css/default.css';
-> setup.php does not include config.inc.php at this point, but includes 
   it before reading anything from $CONF -> harmless

smarty.inc.php:$CONF['theme_css']       = 
$CONF['postfix_admin_url'].'/'.htmlentities($CONF['theme_css']);
smarty.inc.php:$CONF['theme_logo']      = 
$CONF['postfix_admin_url'].'/'.htmlentities($CONF['theme_logo']);
-> I consider this a bug or at least strange behaviour. Should probably 
   be fixed/changed. The two variables are used by templates/header.tpl 
   (probably the modified version) and templates/header.php (used by 
   setup.php, most likely the original value)

> If we use my handlers we have to add this config class natively.

No problem ;-)

If you want, you can merge the code from scripts/ into the main code 
(please wait for Davids response on the possibly conflicting parts or 
check them yourself).

Unless you have a very good reason, please drop the functions that only 
"rename" existing PHP functions (low, up, r) because they make the code 
harder to read. FYI:
- low() is used in shells/*.php several times
- up() is used in shells/shell.php only once
- r() is not used at all - or my grep was too strict ;-)

- pr() is also not used, but we could keep it (with a more meaningful 
  name, maybe "debug_array") in functions.inc.php. BTW, I don't see the 
  need for the  Configure::read check in it - please remove the check.

> For l18n we should use something else to. Global Vars in touch with
> OOP bring headaches

From the style and purity side: yes, sure.
From the practical side: as long as the access is well-defined - does it 
really hurt? (Well, don't take this question too serious, please ;-) but 
we probably have more important things to fix.)

What about a class similar to the config class? (IIRC you have something 
like that already...)
From the functional side, the language class could be a copy of the 
config class, just that it needs $LANG as data ;-)


Regards,

Christian Boltz
-- 
>du meinst die "persönliche Erfahrungen" der hier schreibenden, ja?
>dann ist es gut, dass du hier nicht gefragt hast was du zum sortieren
>deiner mails benutzen sollst. denn ansonsten wäre das wohl procmail.
Hehe, 1:0 für Dich. [> Michael Meyer und Thorsten Haude in suse-linux]