[dev] BC break IMP_6_2 API.

[Michael J Rubinsky]

Quoting Michael M Slusarz <slusarz at horde.org>:

> Quoting Michael J Rubinsky <mrubinsk at horde.org>:
>
>> Quoting Michael J Rubinsky <mrubinsk at horde.org>:
>>
>>> In IMP_6_1, the $registry->mail->mailboxList() used to return the  
>>> mailbox attributes in an 'a' element. This is no longer returned  
>>> in IMP_6_2, and potentially breaks any code consuming IMP's public  
>>> API. Currently, completely breaks ActiveSync mail synchronization.
>>> -- 
>>> mike
>>>
>>> The Horde Project (www.horde.org)
>>> mrubinsk at horde.org
>>
>>
>> Ping? This is a must-fix, as it is a BC break in a public facing API.
>
> I have no recollection of removing this.  But reviewing the change,  
> I can absolutely see why I removed it.
>
> This appears to be the return from that API call:
>
>  * - a: (integer) The mailbox attributes.
>
> First, what are the attributes?  i.e. if 'a' is 3, what does that  
> mean?  From an API perspective, you have to know what is being  
> returned without looking at any of the code of the application  
> itself.  As documented, this looks like a relic of old code.   
> Regardless, this is a worthless API call - calling code can't count  
> on this returning anything other than a number that has no meaning,  
> since that is what the defined return value is.
>
> Second, by tracing the code these attributes appear to be internal  
> representation data of the IMP_Imap_Tree.  Which is very concerning  
> because this is internal data for IMP that has now been incorrectly  
> leaked outside the application.  These internal constants of the  
> IMP_Imap_Tree object never had a guarantee of not changing, since  
> they are not tied to any sort of API.  In fact, those constants did  
> change in IMP 6.2 - since IMP_Imap_Tree completely disappeared.
>
> From a theoretical perspective, I have no problems removing this  
> code.  The documentation provides no guarantee of what is being  
> returned, so the data returned is ambiguous at best.  The actual  
> data returned is irrelevant because the only way calling code would  
> know what it was is to trace the internal app code which is not  
> acceptable for a remote API call.
>
> From a *practical* perspective, obviously there is some code that is  
> using it.  The only thing I could find was in  
> Horde_Core_ActiveSync_Imap_Factory.  And my fears were realized -  
> the constant MARK_SUBSCRIBED is defined with a value of "8" which  
> apparently was the internal integer representation of the  
> IMP_Imap_Tree state at some point in time.  But this is not  
> something that could have every been determined from the API of that  
> call.
> So I would be open to returning only this value in a "hidden" 'a'  
> parameter.  The 'a' parameter itself should be removed from the  
> documentation, as explained above, for being too vague to be  
> practically useful.  It is not an API break since the API was not  
> sufficiently documented in the first place to be usable,

I hear what you are saying, but this is still an API break. I could  
easily add phpdoc to that API method explaining the value of the  
bitmask in IMP 6.1.x before IMP 6.2 is released. Regardless if it was  
the correct way to originally implement it or that it wasn't  
documented fully doesn't make it NOT a BC break.

> but allows existing code to function until that code can be fixed.
>
> As a replacement, the question becomes: is this data obtainable  
> through other existing API means (e.g. through the Imap_Client  
> object)?  Or do we need to add an additional array key - such as  
> 'subscribed' - to the return of mailboxList?

I need to be able to determine if the mailbox is subscribed or not.  
Reading the phpdoc of Horde_Imap_Client_Mailbox I don't see any way to  
get that data from the object. Besides, we shouldn't be relying on  
objects being passed via the API when at all possible. So, how about a  
's' array key?

-- 
mike

The Horde Project (www.horde.org)
mrubinsk at horde.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5849 bytes
Desc: S/MIME Signature
URL: <http://lists.horde.org/archives/dev/attachments/20131021/c858642a/attachment.bin>