From: Marc G. K. <ma...@sq...> - 2006-11-14 16:58:35
|
On Tue, November 14, 2006 2:04 am, Paul Lesniewski wrote: > > * Implementation of the "IMAP_Message" class: > This is really ugly. Marc's going to have to look this over very > closely and give his thoughts. We need to give a LOT of thought to whether > or not there are problems with things like: how the message attributes > are handled, how the mailbox cache is hacked in, how we can better share > IMAP connections (I would rather pool and share them from > sqimap_login rather than basically keeping a more or less static connection > in each IMAP_Message object), etc. > I took a quick look at the IMAP_Message class and it must have been a hell of a job to get this thing working ;) But you are right, this needs a lot of thought. In the long term we should end with an imap_api_class and an imap parser class. The mailbox cache probably can probably be situated in between the imap api class and the imap parser class. > There are lots of "FIXME" comments in the proposed files that also > have relevant notes. > > The mailbox caching mechanism is horribly hacked (in the sense > that it is known as a global value and manipultaed in numerous places in > the code), so pulling that into the IMAP_Message class presents a big > challenge, given the completely different sense of scope and state of a OO > implementation. It also isn't really documented at all, so my > implementation is very very uninformed and probably errant in some > fundamental ways. Note also that because it is used as a global all over > the place, part of the problem is that the core (at least the un-fixed > places) needs to then retrieve it back from the IMAP_Message object... > then if it is modified, it needs to be given back to the IMAP_Message, > etc. Very ugly. Yup. Globals SUCK. > > Also, the actual usage of the IMAP_Message class (replacing the > message retrieval code and deletion code in read_body.php) works for me, > but I'm not sure it is at all correct. People need to look at this before > I make any commits to the SM CVS. > I realy need to find some time to dive into this but i've been too busy with work. Hopefully I can find a couple of hours this week to work on the imap part. > > * There is also a HUGE problem with the SM core in that errors are > almost entirely handled *on-the-spot* by spitting out some error to the > browser. This is entirely unacceptable (not to mention somewhat naive > architecturally) when we are dealing with an API request that cannot > return output in this manner. This is pervasive and rooting it out from > the core will take some serious effort. Before we embark on that mission, > IMO we need to first decide on a better error handling > architecture. Do we let the API just start an output buffer and capture > errors on its own (NO WAY!)? Do we set a special error handler for the > API calls and throw errors from the core that can be > caught and returned in the right format for API clients? We have already an error class which catches errors. > Do we re-write > the core to return error messages (an error class object?) whenever an > error happens and make it percolate all the way up through the core > functions so the original caller (the API, for instance) gets to decide > what to do with the error? I believe we should also create a known set of > integer constants for every possible error SM generates. > We need a central error handler (we already have one). I also added a file errors.php where all the errors are defined. Each type of error is unique and can have an unique error number. > > I would normally commit stuff to CVS, but, particularly with the > IMAP_Message class implementation, this departs and replaces the SM > core code to such a drastic extent that I want to have consensus that we > are headed in the right direction first. Marc, you mentioned that you had > thoughts on the API design, so you want to pipe up? > On my todo list and since today I raised the priority of that todo item. > The biggest and most difficult of this for me was trying to understand > how the hell to pick out the message code from read_body.php and pull it > into a class format. That code is just sooooo embedded into a global > programming scheme that it was very difficult and the implementation is > probably qutie flawed. The delete code is still set to call a non-class > method -- a global function that was originally written to handle form > requests -- IMO we also need to pull this apart and put it in the > IMAP_Message class, but here, too, the handling of > the message and the mailbox cache was just too much for me to tackle, so as > long as it worked for proof-of-concept, I left it alone. Because I'm pretty familiar with all message object and imap related code I think I realy should take some days off and start coding. Regards, Marc Groot Koerkamp. |