[Zope-dev] Re: [Zope-Checkins] SVN: Zope/branches/Zope-2_8-branch/ fix #2235 for real now

[Chris Withers]

Leonardo Rochael Almeida wrote:
>>> -        obj = self.aq_parent.unrestrictedTraverse(self.getpath(rid))
>>> -        if not obj:
>>> +        obj = self.aq_parent.unrestrictedTraverse(self.getpath(rid), None)
>>> +        if obj is None:
>> Please revert this change. You've changed the semantics of this 
>> statement and it will now mask errors in traversing to the path.
>>
>> This is bad...
> 
> I agree with you the semantics have changed, but I argue that they've
> changed to what they were intended to be in the first place :-)

This argument has been had many times before, please google zope-dev's 
archives for the numerous discussions on this and then check the current 
implementation in 
Products/ZCatalog/CatalogBrains.py:AbstractCatalogBrains.getOject.

> This is the full method before my changes:
> 
>     def getobject(self, rid, REQUEST=None):
>         """Return a cataloged object given a 'data_record_id_'
>         """
>         obj = self.aq_parent.unrestrictedTraverse(self.getpath(rid))
>         if not obj:
>             if REQUEST is None:
>                 REQUEST=self.REQUEST
>             obj = self.resolve_url(self.getpath(rid), REQUEST)
>         return obj

This code is just plain rubbish, there's no single bug here.

I'd like someone to give a use case where unrestrictedTraverse fails and 
yet where resolve_url works.

> Traversing with a default argument will only mask traversal errors, not
> any other errors that might happen during traversing.

Sure it will, see previous discussions...

> In the version with my changes, but with the old unrestrictedTraverse
> call, the "if obj is None" would only ever be reached in the
> pathological situation where the attribute at the end of the path is
> None. Which means the ".resolve_url(self.getpath(rid), REQUEST)" would
> hardly ever be called at all.

Correct. .resolve_url(self.getpath(rid), REQUEST) hardly ever gets called.

> A) Keep my changes, but use a marker "object()" instead of None for the
> "default" parameter of the .unrestrictedTraverse() call.
> 
> B) Reverse my changes, but also remove the "if not obj" block, which is
> buggy whichever way you look at it with the old unrestrictedTraverse
> call.
> 
> Which one should it be?

I'd go for B.

cheers,

Chris

-- 
Simplistix - Content Management, Zope & Python Consulting
            - http://www.simplistix.co.uk