[dev] Horde 5 was: Re: [commits] Horde branch develop updated. fa956a4d83a7227e2a5d47bfe87185854e151fbc

[Michael J Rubinsky]

Quoting Jan Schneider <jan at horde.org>:

> Zitat von Michael M Slusarz <slusarz at horde.org>:
>
>> Quoting Michael J Rubinsky <mrubinsk at horde.org>:
>>
>>> Quoting Michael M Slusarz <slusarz at horde.org>:
>>>
>>>> Re-setting this discussion, since I don't even know where we are  
>>>> at.  Here's what I see:
>>>>
>>>> * Notifications and Alarms setup HAVE to be cached going forward.  
>>>>  Nobody has proven to me that these changes broke anything, or  
>>>> are not BC.
>>>>
>>>> The only thing that has changed (that I can see) in the  
>>>> notification/alarm code is as follows:
>>>> - Applications are only initialized once per session.
>>>> - Non-authenticated apps are initialized (or initialized before  
>>>> the entire Horde framework is initialized).
>>>
>>>> The first shouldn't be an issue (and is the whole reason for the  
>>>> change).  The second is NOT A BC BREAK!  Not only was this  
>>>> behavior not documented, and thus not part of the API, but this  
>>>> is how we did things at the release of Horde 4.
>>>
>>> I stand by my earlier statement - this *IS* a BC break. How can it  
>>> not be? If Core is updated, existing application functionality  
>>> breaks. It doesn't much matter what any documentation does or does  
>>> not say - a user is only going to care that their synchronization,  
>>> or some custom application that uses RPC access is broken.
>>
>> You are still wrong.  It is NOT a BC break.  Just because KRONOLITH  
>> is incorrectly using the API does not make anything in Core wrong.
>>
>> There are only 2 ways you can break backward compatibility:
>>
>> 1. You explicitly change an API element.  We all agree that is not  
>> the case here (these changes don't change what a method either  
>> accepts or returns).
>> 2. Alternatively, it could be argued that in the absence of a  
>> clearly-defined API, you change some critical system as it was  
>> performed hen 4.0 was released.  As already described, in 4.0 the  
>> defined behavior is that non-authenticated applications would have  
>> init() called BEFORE the full Horde environment was created.
>>
>> To reiterate: init() is solely a bootstrapping function to allow  
>> enough of the application environment to be setup so that it could  
>> be accessed by the Registry.  That's all it was ever designed to  
>> do.  As amply shown, it was never guaranteed to have the full Horde  
>> environment (this is why authAuthenticateCallback() exists).
>>
>>> For instance, the Ansel uploader plugins for iPhoto and Aperture  
>>> rely on existing RPC behavior. If initializing the share system of  
>>> applications when accessed via RPC is broken (which is currently  
>>> is) - these uploaders will be broken too.
>>
>> If Ansel is not following the limitations described above, then the  
>> correct fix is to fix Ansel.  There's nothing wrong with Core -  
>> just how Ansel uses Core.
>> As far as versions - this is the whole point of package.xml.  Will  
>> the next version of Kronolith 3.x require Horde_Core 1.6.0 (or  
>> whatever the next release is)?  Yes.  That's just the reality of  
>> the situation.

..and this requirement is fine. It's the other direction that is the issue.

>> Can you upgrade Horde_Core without upgrading Kronolith?  No.  But  
>> that's also the reality of the situation.

.and IMO, makes this a BC break.

>> Imagine this scenario: MIME encoding of an address.  Let's say an  
>> application parsed the results of an encode for the  
>> quoted-printable signature to determine if the message was encoded,  
>> and performed an action based on the encoding status.  In a future  
>> version of the MIME library, it was decided to change  
>> quoted-printable encoding into binary encoding.
>>
>> This change would "break" the existing application if the library  
>> is updated but not the Application.  But this is not a BC break.   
>> This is an example of an application making incorrect assumptions  
>> about the API.  Nobody could possibly argue that we would have to  
>> maintain the old way of MIME encoding just because a downstream  
>> application won't be able to handle it properly.

I guess we will have to agree to disagree. If the MIME encoding is  
done in a stand alone library, and the behaviour of that library  
changes in such a way that any existing code written against it is  
broken due to the change, that is a BC break and warrants a bump of  
the library's major version number.

It is VERY frustrating as a developer to have your code break after  
some minor update to a library not under your control. As an example,  
at one point ImageMagick's utilities were breaking BC with almost  
every minor (and even with only revision number increments) release  
they made. They, too, had made arguments that the previous behavior  
was not totally correct. It didn't much matter, existing scripts  
broke. Their mailing list archive is full of complaints about this.  
That's one reason why Horde_Image has so much version sniffing in it.  
It was a major source of frustration to developers and caused some to  
move on to other, more stable tools. I expect a jump from 1.0 -> 2.0  
to have breaks, not from 1.0 -> 1.1 or even 1.0.1.

>>>> We switched to only checking authenticated apps in this commit, a  
>>>> week after Horde 4 was released:
>>>>
>>>> commit dcdd9f52eabc7cd8df5b2ea661097effe039833e
>>>> Author: Michael M Slusarz <slusarz at curecanti.org>
>>>> Date:   Thu Apr 14 23:48:24 2011 -0600
>>>>
>>>>  Bug #9733:  Don't setup notification handlers in applications  
>>>> that are not yet authenticated
>>>
>>> Which is a change you committed after commenting that you didn't  
>>> know why we were polling unauthenticated apps for notifications.  
>>> What changed with the documentation of the API since then that  
>>> makes the earlier behavior more appropriate?
>>
>> Because this was a broken assumption.  This prevents  
>> non-authenticated applications from setting notification handlers,  
>> which is perfectly acceptable under the original API (as it should  
>> be).  So the original bug-fix was flat wrong.
>>
>>>> Thus, if Kronolith has been initializing things in _init() that  
>>>> aren't working because the Horde session itself is not fully  
>>>> initialized, KRONOLITH IS BROKEN AND MUST BE FIXED (in 3.0 also).  
>>>>  FYI: this is the whole reason for the authAuthenticateCallback()  
>>>> - to do things that require the ENTIRE Horde framework to be  
>>>> initialized.
>>>
>>> Another issue to consider is that rpc requests initialize Horde  
>>> with authentication => 'none' (we delegate authentication to the  
>>> Rpc classes), and when I attempted to refactor things using this  
>>> callback, it was *never* called from rpc requests.
>>
>> This seems like a limitation in the Registry_Application API then  
>> that was not contemplated before Horde 4 came out.  Sounds like you  
>> will have to manually do Authentication checking in Kronolith.  Or  
>> add a new method that is called in this situation.  But you can't  
>> hijack the init() function to do something it wasn't supposed to do.
>>
>>
>> I'll be honest: revisiting this kind of BC discussion/argument is  
>> exactly what I was fearful of a year ago.  This adherence to BC is  
>> the single biggest problem of Horde, as it has been for the last 10  
>> years.

To be fair, this isn't a problem unique to Horde. I'm sure you are  
well aware that this is a fact of life when you are programming  
against any set of published stand-alone libraries. In a more general  
sense, no one is saying you can't break BC. You just can't break it  
within the same major version; We need to bump the major version  
number. As discussed below, this is exactly what we said should be  
done in these cases during the initial Horde 4 discussions. The  
problem with the Horde 3 -> Horde 4 change was that it just took *way*  
too long between those releases. BC was a thorn in everyone's side.  
That's something we all agreed should not happen again.

>>  I have had preliminary thoughts of forking IMP for precisely this  
>> reason so I could avoid all this headache.  I spend way too much  
>> time worrying about these sorts of issues - something like  
>> releasing versions of IMP that require the latest version of all  
>> required components would eliminate a tremendous amount of wasted  
>> time for me.  There's a bunch of stuff that I have wanted to do,  
>> but have not been able to because of these limitations.  Not my  
>> intention to sound combative, but it accurately portrays the  
>> frustration I have felt the last few months.  Or maybe my goals are  
>> simply diverging from others'.
>
> While I'm still with with Mike in this discussion, we don't seem to  
> be able to come to a conclusion.
>
> When we discussed the new release model for Horde 4, one of the  
> important points was indeed that we don't want to fear breaking BC  
> if we consider it necessary to go forward. I say we have reached  
> this point, and instead of spending too much time in such  
> discussions and causing frustrations, we should make the next  
> release Horde 5.
> I already considered to bring this up before this thread started,  
> because if we really manage to get the new design in place for the  
> next release, this would probably be a large enough change for the  
> end users that it warrants and a new major version, even if we  
> didn't break BC.

This is exactly what we discussed should happen in a case like this.  
Let's do this and move on.

-- 
mike

The Horde Project (www.horde.org)
mrubinsk at horde.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-keys
Size: 2200 bytes
Desc: PGP Public Key
URL: <http://lists.horde.org/archives/dev/attachments/20120220/ab801ce8/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 6096 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.horde.org/archives/dev/attachments/20120220/ab801ce8/attachment-0003.bin>