Refactor user module

This is a first alpha version of my work. To keep Seagull running and other modules unaffected I created a new DA_User2 to work with. With this patch only UserMgr and RegisterMgr rely on the new DA_User2. The patch is built against latest svn bugfix branch.

DA_UserTemplate

• Public API
• Implements the Template Method Pattern

DA_User2

• Transactions are handled in the template, DA_User2 contains only "primitive methods"
• Consistent error handling and returning

UserMgr

• Much data access stuff moved to DA_User2
• Some minor changes

Leftover work

• Public methods will do extensive parameter checking
• All other managers in user module
• Improve "change user status" stuff
• Remove admins' ability to reset a user password since sending the new password via (plain-text) email is pretty unsecure
• Move login stuff to DA_User
• Solution for raw SQL queries in manager methods when using a pager
• Unit tests for public methods

Questions

• Please tell me what you (conceptionally) think about the _deleteUserPermissions() method in DA_User2. I did it this way because I dislike dozens of methods like
  • deletePermsByUserId
  • deletePermsByUserModuleId
  • deletePermsByThisAndThat
  • deletePermsByFooButNotBar

You see what I mean? Another reason why I dislike it is that it's often simple code duplication with minor changes, e.g. in a WHERE section

• Comments on ResultCounter and WhereStack are also greatly appreciated

Hi Tamme

Thanks for this patch, have just been looking over the proposed code.

I like most of the ideas you have here, here's what i think works well with some comments:

• template method pattern: this is a mostly abstract class, i would just rename it to DA_AbstractUser or even better AbstractUserDAO which more clearly communicates its role
• abstracting transaction stuff is a big improvement
• each concrete class I think should be done in the standard PEAR way, ie modules/user/AbstractUserDAO.php (DAO is an improvement on DA_*), then modules/user/Container/DbDAO.php, modules/user/Container/LdapDAO.php etc.
• you haven't suggested it but i wonder if using the observer pattern would make for cleaner code, just attach pref/perm/org related "sub-tasks" as listeners
• WhereStack? and ResultCounter? are nice, why not add them to SGL_Sql so we can take advantage of namespacing
• i think your _deleteUserPermissions() method is a good improvement

Here's a few things to watch out for:

• don't rely on PEAR docs for return values, quite often they are out of date and wrong, use your debugger
• _deleteUserPermissions($aOptions = null) ... then if (is_null($aOptions) return SGL::raiseError ... does not make sense, a default argument cannot be wrong
• RC_SUCC no need to truncate constant, and prefix it with SGL_

In addition:

// no logMessage allowed here, // Tamme: WHY???

This is because when session is being initialised, the DA_User call is made and logging is not setup yet.

Overall I think the patch is almost fine to go in to 0.7 trunk (that i need to sort out), why not make the few proposed fixes and double check returns values because i think i used false instead of pear::error for a good reason, then i'll commit it.

Milestone 0.7.0 deleted