postfixadmin-devel

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

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

Hello,

The classes in scripts/models-ext/ contain some changes that affect the 
public interface of the classes. 

@David: Can you please review them and add some comments? I don't want 
to break the XMLRPC interface or anything else ;-)

- is model/
+ is scripts/models-ext/

AliasClass:
-    public function get($all=false) {
-        $username = escape_string($this->username);
+    public function get($alias, $all=false) {
+        $alias = escape_string($alias);
...
-            return $list;
+            $this->return = $list;
+            return 0;
         }
-        return array();
+        return 1;

UserHandler:
-    public function change_pass($old_password, $new_password) {
+    public function change_pw($new_password, $old_password, 
+                              $match = true) {

This will break for sure (changed function name and parameter order), 
also the code inside the function changed a lot.

$match = true means that the old password is checked before doing 
anything. For admin mode, it would probably be easier to make 
$old_password optional (which would also require the changed parameter 
order ;-) (Or would this be less secure?)

If I get xmlrpc.php right, the functions are not directly accessable 
over the API, but are wrapped (in this case in the UserProxy, function 
change_pass) - right?

That would mean if we change the function parameters, we have to change
- xmlrpc.php
- users/password.php
(Again: Right?)

UserHandler in scripts/models-ext/ also contains several new functions, 
but this shouldn't break anything ;-)


I really have to (find some time to) dig deeper in those classes...


Regards,

Christian Boltz
-- 
"Oh, hallo ersma... ick schein zu laufen... Denn wolle mer ma kucken,
wat hier so los is... also erstmal isset gut, wenn icke ne CPU finden
taet. Denn auf irgendwat schein ick jo ausjefuehrt zu wern, nae?  Und
denn, was hammwer denn noch so allet?  Ah, eine HD? Jut.  Denn kucken
wer ma, wat da so allet druff is... (usw.) ... ah, unn wat zum Krach-
machen hamwer oooch?..." [David Haller in suse-linux zum Bootvorgang]

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

[Valkum <va...@go...>]

Hello,

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.
If we use my handlers we have to add this config class natively.
For l18n we should use something else to. Global Vars in touch with OOP
bring headaches


Rudi

Am 27.11.2010 00:40, schrieb Christian Boltz:
> Hello,
>
> The classes in scripts/models-ext/ contain some changes that affect the 
> public interface of the classes. 
>
> @David: Can you please review them and add some comments? I don't want 
> to break the XMLRPC interface or anything else ;-)
>
> - is model/
> + is scripts/models-ext/
>
> AliasClass:
> -    public function get($all=false) {
> -        $username = escape_string($this->username);
> +    public function get($alias, $all=false) {
> +        $alias = escape_string($alias);
> ...
> -            return $list;
> +            $this->return = $list;
> +            return 0;
>          }
> -        return array();
> +        return 1;
>
> UserHandler:
> -    public function change_pass($old_password, $new_password) {
> +    public function change_pw($new_password, $old_password, 
> +                              $match = true) {
>
> This will break for sure (changed function name and parameter order), 
> also the code inside the function changed a lot.
>
> $match = true means that the old password is checked before doing 
> anything. For admin mode, it would probably be easier to make 
> $old_password optional (which would also require the changed parameter 
> order ;-) (Or would this be less secure?)
>
> If I get xmlrpc.php right, the functions are not directly accessable 
> over the API, but are wrapped (in this case in the UserProxy, function 
> change_pass) - right?
>
> That would mean if we change the function parameters, we have to change
> - xmlrpc.php
> - users/password.php
> (Again: Right?)
>
> UserHandler in scripts/models-ext/ also contains several new functions, 
> but this shouldn't break anything ;-)
>
>
> I really have to (find some time to) dig deeper in those classes...
>
>
> Regards,
>
> Christian Boltz

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]