Re: [SM-DEVEL] API Implementation

[Paul L. <pa...@sq...>]

On 11/14/06, Marc Groot Koerkamp <ma...@sq...> wrote:
> 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 ;)

Thanks.  I was definitely frustrated several times.  :-)

> 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.

Yeah, sitting the cache in some place invisible to the caller is of course best.

> > 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.

Right, I was under the impression that it was only useful for catching
errors that are triggered.  I was proposing that we need an error
class that can encapsulate an error code and corresponding textual
message and can then be used as a return value from a function.  Can
the class you created be used in that way?

> >  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

I'm not entirely sure if this addresses the problem with errors deep
in core calls to things like sqimap_get_message and so on.  Those
functions can use trigger_error() and get caught by the error handler,
so that's one approach, but I think it's better to let them construct
some kind of error object themselves and return it so it can percolate
back up to the original caller who then will have a better idea of
what to do with it.  If you've ever worked in Java, somewhat analogous
to the try/catch thing...

> errors.php where all the errors are defined. Each type of error is unique
> and can have an unique error number.

OK, that's a good start.  We should add to that file substantially if
we are to clean up the core as suggested.

> > 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.

As they say in Japanese, yoroshiku onegaishimasu!  :-)