[dev] [commits] Horde branch master updated. 807ce308a199bb099eba18e853d2d9f94f3a8780

[Jan Schneider]

Zitat von Michael M Slusarz <slusarz at horde.org>:

> Quoting Jan Schneider <jan at horde.org>:
>
>> Zitat von Michael M Slusarz <slusarz at horde.org>:
>>
>>> 399111b phpdoc; bump up timeframe to be labeled a 'slow' command
>
> Tweak.
>
>>> cac2e79 [mms] Move IMAP mailboxes sorting into new  
>>> Horde_Imap_Client_Mailbox_List class.
>
> API bugfix.  Imap_Client methods that return mailboxes need to  
> return this object (instead of an array) so that we can potentially  
> add further features **without having to break the API every minor  
> release**.

That doesn't make it a bug fix. It's a refactoring for forward  
compatibility. I don't question its reasoning, it's still changing the  
API, and adding and removing classes.

>>> 0960738 [mms] Remove Horde_Imap_Client_Utils#removeBareNewlines().
>
> API bugfix.  This should have been removed months ago with the  
> switch to Horde_Imap_Client_Data_Format_*.
>
>>> e9f9906 [mms] Remove Horde_Imap_Client_Utils#escape().
>
> Same.
>
>>> f960868 [mms] Move IMAP sequence string generation/parsing to the  
>>> Horde_Imap_Client_Ids object.
>
> Bugfix - mailbox information could potentially leak into situations  
> where only IMAP sequence string is supported.  Regardless, this  
> fixed a hackish/BC way this was implemented in  
> Horde_Imap_Client_Base: Base objects needed to store the correct  
> Utils **and** Ids class name, when this can be accomplished with a  
> single classname.  And this just makes about a million times more  
> sense from an API perspective (see below).

See above. Might make sense, but is an API change, and removing classes.

>>> 4584d5e [mms] Move base subject parsing to new  
>>> Horde_Imap_Client_Data_BaseSubject class.
>
> This is a refactor, but it was the only method left in Utils class.   
> Move to a properly named class and implement using proper  
> OO-techniques.  Used in *1* location outside of Horde_Imap_Client.   
> Allows us to get rid of the confusing (and unnecessary) reliance on  
> getUtils()/utils in the base object.

See above.

>>> fcb84dc [mms] Move IMAP/POP URL parsing to new  
>>> Horde_Imap_Client_Url object.
>
> API Bugfix.  Was implemented this way in 1.x for BC reasons.  NO  
> reason not to switch to an object, especially since we now use this  
> code outside of Horde_Imap_Client.  Used in *2* locations outside of  
> Horde_Imap_Client.
>
>> Please revert. This is far from what's acceptable for bug fixing.  
>> This is refactoring.
>
> See above.  These fixes were made *precisely* because I am in  
> release mode: they occurred because I am currently stress testing  
> with every IMAP server I can get a hold of.  It is precisely these  
> kind of fixes that would NEVER occur during development since I have  
> little/no incentive to do this kind of comprehensive testing.  These  
> are exactly the kind of things that need to be fixed when preparing  
> for a release.  Example: I have forced myself to try to install  
> Horde from a clean install; this is the basis for all the cleanup I  
> have been doing in the install Test classes.
>
> Not to mention I wrote a bunch of additional unit tests to test  
> these changes.
>
> Additionally, I was reviewing the API documentation of this class  
> and noticed it had a bunch of calls that are not needed.  I would  
> really like to push the Imap Client more in release 2 - as such, the  
> API needs to be as clean as possible.  That needs to happen NOW,  
> before the API is frozen, as opposed to later.

The point is, our APIs *are* more or less frozen. API cleanups of this  
size are *not* bug fixing and need to be finished *before* feature  
freezes.

Another point is: you are not the only one testing the IMAP code. We  
have spent days and weeks of Kolab development and testing to get  
everything in shape for the release. If you keep changing APIs you  
makes a good bunch of this work useless, because things have to be  
tested *again*.
I appreciate your stress testing and incentive to get the API as clean  
and extensible as possible and admit your limited time, but I  
announced the freeze sufficiently early. And there is always something  
that could use more cleanup. At one point we need to make a cut. But  
with your last minute changes you risk to delay the release yet  
further, and cost other people's time and money.

> And I'll prepare you now: there are two nasty issues in Imap_Client  
> with the way we handle EXPUNGE/VANISHED responses and with a race  
> condition in QRESYNC (see  
> http://tools.ietf.org/id/draft-kundrat-qresync-arrived-00.html).   
> This is requiring a rewrite of how we handle sequence->UID mappings,  
> and it is not an easy or trivial fix.  But it needs to be done or  
> else messages CAN be lost in the mailbox when using caching - and  
> will NEVER be seen unless the cache is deleted.
>
> michael
>
> ___________________________________
> Michael Slusarz [slusarz at horde.org]


-- 
Jan Schneider
The Horde Project
http://www.horde.org/