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

_______________________________________________
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - http://mail.zope.org/mailman/listinfo/zope-announce
http://mail.zope.org/mailman/listinfo/zope )

Reply via email to