Re: [Postfixadmin-devel] refactoring

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

Hello,

Am Dienstag, 22. Juli 2008 schrieb David Goodwin:
> I'm wondering if we should refactor the codebase into having a
> distinct object layer which :
>
> a) Could be exposed via xmlrpc or soap (useful for 3rd party
> integration)
>
> b) Could be more easily tested (unit tests etc)
>
> c) Will move some logic from the controllers into a model layer.

Just a short note: I'm also thinking about refactoring since some time. 
My focus was more on making programming and changes easier, but having 
an automated interface would be a nice side effect.

My idea in short:

- create a basic class with the required functions/methods. Most 
  important items: (intentionally skipping the parameter lists)
  * [non-public] table structure as array (like $fm_struct in 
    fetchmail.php)
  * function get_list() (for list view)
  * function get_list_for_domain() (for list view)
    - both get_list* should have an optional $search parameter
  * function get_item() (current values, for edit form)
  * function edit() (called when submitting the edit form)
    parameters given as array with named keys
    edit() calls:
    - [non-public] function validate_by_type() (simple check against 
      field type given in table structure)
    - [non-public] function validate_special() (other checks that are 
      not covered by type check)
  * function add() (basically like edit())
  * function delete()
  * [non-public] check_domain_permission() (check if the admin has 
    permissions for this domain)
  * [non-public] check_other_permission() (check other permissions, for 
    example if editing mailbox aliases is allowed)
  * other non-public functions as needed - target should be to have most 
    code in the common class and as least as possible in the 
    mailbox/alias/whatever class.

- make each object type (admin, domain, mailbox, alias) a PHP class 
  based on the basic class described above

- create/use a class for rendering lists
  Maybe we can use the Table class from http://exorsus.net/software/, 
  example on http://exorsus.net/software/table_class/test_complete.php
  (I'm using this class at other places already)

- create a class for rendering edit forms (see fetchmail.php for ideas)
  or use the one from http://exorsus.net/software

- create a small class rendering the domain dropdown we have at nearly 
  every list page

> Pseudo example:
>
> Mailbox object:
>   -> login($u, $p);
>   -> addMailbox($name, $domain)
>   -> deleteMailbox($name, [$domain]);
>   -> updateMailbox($assoc_array_of_params);

Since you already propose separate objects for mailbox, domain etc, 
I'd prefer to have common names like "add", "edit", "delete".

> Vacation object:
>   -> setAway($msg, $mailbox);
>   -> setReturned($mailbox);
>
> Domain object:
>   -> addNewDomain($name, $other, $parameters);
>   -> listDomains();
>   -> addMailbox($name);
>
> Alias object:
>   -> addNewAlias($source, $dest);
>   -> listAliasesForDomain($domain_name, $paging, $parameters);
>   -> removeAlias($source, $dest);

Admin object:
   -> list admins
   -> list domains for admin
   -> create admin
   -> add domain to admin

> So these objects would encapsulate all the required SQL logic, and
> should also have authentication checks (as appropriate). They could
> then be exposed via e.g. xmlrpc or soap - however this might require
> all methods having an additional two parameters (username, password)
> etc, or writing a simple proxy object.

A central proxy object is probably easier to maintain... (as always: if 
you can do it at a central place, don't spread the same code across 10 
files ;-)

The basic permission checks (is access to this domain allowed) could be 
handled in the basic class. If more details have to be checked, each 
class can do it additionally.

> I've made a vaguely related blog post -
> http://codepoets.co.uk/using-soap-and-xmlrpc-php5-newbies-findings

Just had a quick look at it - sounds interesting, but I'll need some 
more time to read it. Right now, I like this the best:
    SOAP is really now just called "SOAP", I think they've dropped 
    the "Simple..." bit from the name as it can be anything but simple.
;-)

> I've also released postfixadmin 2.2.1.

Good to hear.

As always ;-) I found some small problems.

You have missed some small fixes, see the attached patch.

In case you wonder: I generated this patch with (the following 3 lines 
should be entered as a single one):
svn diff 
https://postfixadmin.svn.sourceforge.net/svnroot/postfixadmin/branches/postfixadmin-2.2.1 
https://postfixadmin.svn.sourceforge.net/svnroot/postfixadmin/trunk 
and then removed everything related to alias domains.

Please read the patch and don't blindly apply it to the 2.2(.2) branch - 
at some places I have replaced big changes by short comments 
(upgrade.php, language files)

As a side effect of not including alias domains, you have caused a small 
problem with database upgrade - upgrade.php is currently at Revison 392 
(in trunk) and 397 (in 2.2.1 branch).
This means the functions to create the alias_domain table won't run when 
merged back in because it has a lower number (upgrade_300 and 
upgrade_362 - I just wonder why we have it twice...)
The problem isn't really big - we have to rename the functions to a 
higher number - and we need IF NOT EXISTS in the CREATE TABLE statement 
for those using the svn version ;-)

Oh, and IMHO we should include all database changes from trunk in future 
releases even if it produces unused tables. Causes less trouble ;-)


On the positive side, I have updated the RPM packages and also uploaded 
an openSUSE RPM to SF. I also updated the changelog and updated 
debian/changes in trunk.


Regards,

Christian Boltz
-- 
Meine Katze hat zu der Maus auch gesagt: "Kannst ganz beruhigt sein,
ich tu Dir nichts!" Und vom Fressen hat die Katze kein Ton gesagt.
[Rolf-Hubert Pobloth in suse-linux]