cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sylvain Wallez <sylv...@apache.org>
Subject Re: svn commit: r149408 - in cocoon/branches/BRANCH_2_1_X/src: deprecated/java/org/apache/cocoon/components/source/ java/org/apache/cocoon/components/ java/org/apache/cocoon/components/source/impl/ java/org/apache/cocoon/components/treeprocessor/ java/org/apache/cocoon/environment/ java/org/apache/cocoon/environment/wrapper/ webapp/samples/test/pass-through/
Date Tue, 01 Feb 2005 17:57:33 GMT
Carsten Ziegeler wrote:

> Sylvain Wallez wrote:
>
>> Carsten Ziegeler wrote:
>>
>>> sylvain@apache.org wrote:
>>>
>>>> Author: sylvain
>>>> Date: Tue Feb  1 07:43:40 2005
>>>> New Revision: 149408
>>>>
>>>>      /**
>>>> -     * Remove attribute from the current instance, as well as from 
>>>> the
>>>> -     * wrapped environment.
>>>> +     * Remove attribute from the current instance.
>>>>       *
>>>>       * @param name a <code>String</code> value
>>>>       */
>>>>      public void removeAttribute(String name) {
>>>>          super.removeAttribute(name);
>>>> -        this.environment.removeAttribute(name);
>>>>      }
>>>>  
>>>
>>>
>>>
>>> Hmm, isn't this an incompatible change? Ok, I always thought that 
>>> the implementation was not the way one would expect, but we 
>>> shouldn't imho introduce such changes during maintenance releases. 
>>> I'm fine with changing this for 2.2.
>>
>>
>>
>>
>> I tracked down all uses of removeAttribute in the code base (there 
>> are really few):
>> - ActionSetNode.call : the previous implementation could lead to bugs 
>> when an action-set was calling a cocoon: that itself uses an action-set
>> - MountNode.invoke : same bug if a pass-through calls a cocoon: that 
>> has a pass-through
>> - CocoonComponentManager.leaveEnvironment : called only when env 
>> stack is empty, meaning we aren't in a wrapped environment
>>
>> So this change actually fixes some bugs ;-)
>>
>> Now my changes in pass-through handling aren't finished, as I found 
>> some cases where it breaks. Working on it...
>>
> I really value your work, but you are changing a lot of things and I'm 
> really wondering what site effects this may cause. The internal 
> environment handling has always been a very fragile part of Cocoon. 
> Remember all the problems with internal redirects, redirects from flow 
> and so on, so I'm really questioning if it's worth to go through this 
> just because you need this pass-through feature in 2.1.x?


Well, we have based some important parts of our project based on the 
assumption that it worked, and well... I need to make it work now :-/

Now if I find all these bugs in 2.1, I guess that also because our 
project makes a good test case for this fragile thing. So if our project 
runs, it's very likely that everything is ok. I'm also building some 
test cases in webapp/samples/tests/pass-through along with my bugfix 
work. We'll see how 2.2 behaves ;-)

> I think it would have been better if you had made an internal fork and 
> not commit this into 2.1.x and once 2.2 is stable you could forget 
> about your fork and use that. But apparently it's too late, I guess :(


Mmmh... yes...

> But hopefully everything will run smoothly after your changes and 
> noone is affected by the incompatible changes.


Hopefully :-) And in the meantime, this allows me to better understand 
the internals of the environment management, which was still a dark area 
for me. I know this has changed a lot in 2.2, but this knowledge won't 
be lost and will allow me to more easily understand the reasons that led 
to the 2.2 refactoring.

Also, if I don't succeed in fixing 2.1, I promise to roll back the 
current changes.

Sylvain

-- 
Sylvain Wallez                                  Anyware Technologies
http://www.apache.org/~sylvain           http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }


Mime
View raw message