[Zope-CMF] Re: Bug with recursing into opaque items (__recurse in CMFCatalogAware)

[yuppie]

Hi!


Gregoire Weber wrote:
> I have a fix for the bug Tim found. '__recurse' in CMFCatalogAware has to be fixed.
> As this is a central part of the code beeing called on all add/clone/move and
> delete actions I'd like to put the solution under discussion.
> 
> Index: CMFCatalogAware.py
> ===================================================================
> RCS file: /cvs-repository/Products/CMFCore/CMFCatalogAware.py,v
> retrieving revision 1.21
> diff -r1.21 CMFCatalogAware.py
> 194,200c194,213
> <         for subobjects in values, opaque_values:
> <             for ob in subobjects:
> <                 s = getattr(ob, '_p_changed', 0)
> <                 if hasattr(aq_base(ob), name):
> <                     getattr(ob, name)(*args)
> <                 if s is None: ob._p_deactivate()
> <
> ---
> 
>>        for ob in values:
>>            s = getattr(ob, '_p_changed', 0)
>>            if hasattr(aq_base(ob), name):
>>                getattr(ob, name)(*args)
>>            if s is None: ob._p_deactivate()
>>
>>        # opaque items are subitems of self and not ofsubitems
>>        # self's parent. So before recursing the arguments have to be set:
>>        #    arg[0] to self (the opaque item)
>>        #    arg[1] to item (the item the opaque item is attachted at)
>>        # the container of the item isn't relevant for recursing into
>>        # opaque items
>>        args = [self, args[0]][:len(args)]
>>        for ob in opaque_values:
>>            s = getattr(ob, '_p_changed', 0)
>>            if hasattr(aq_base(ob), name):
>>                getattr(ob, name)(*args)
>>            if s is None: ob._p_deactivate()
>>
> 
> 
> I'm pretty sure the old soultion is a bug that never appeared because
> the wrong parameters passed were never really used by the 'talkback'
> object.

If this is a bug, it's a general bug, not one related to opaque items.

After staring for a while at this code, at ZCatalog.CatalogAwareness and 
at OFS.ObjectManager, I came to these conclusions:

- Nowhere is defined which values should be passed to the 'item' and 
'container' arguments of the manage_before*/manage_after* methods. Most 
manage_before*/manage_after* methods don't use these arguments at all.

- The recursive manage_before*/manage_after* methods in CatalogAware and 
ObjectManager pass 'item' and 'container' just through, so by default 
'item' is always the object and 'container' the container of the object 
on which the method was called first. This is also the behavior of 
CMFCatalogAware.


'item' and 'container' as used by default seem to be quite useless, so 
giving them a new meaning in ICallableOpaqueItemEvents would not really 
hurt. But on the other hand the proposed meaning - 'item' for the 
container of the opaque item and container defunct - makes things not 
more intuitive.

Why not just taking the parent from the acquisition context and ignoring 
'item' and 'container' completely?


Cheers,
	Yuppie