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

[Leonardo Rochael Almeida]

Hi Chris,

Em Qui, 2006-11-16 às 07:33 +0000, Chris Withers escreveu:
> Leonardo Rochael Almeida wrote:
> > @@ -615,8 +615,8 @@
> >      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:
> > +        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 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


Here it is, after 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), None)
        if obj is None:
            if REQUEST is None:
                REQUEST=self.REQUEST
            obj = self.resolve_url(self.getpath(rid), REQUEST)
        return obj

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

If the intent was not to mask any errors at all, even the ones normal to
traversal, then the "if not obj" that was there before would only be
triggered in situations where there it was a bug: an object was found
but it's got a .__len__ or .__nonzero__ method.

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.

I'm open to reversing the unrestrictedTraverse call to the old
semantics, as soon as we agree what those where, so I propose 2 options
and I'll implement whatever this list decides:

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?

Cheers, Leo

-- 
Leonardo Rochael Almeida
Enfold Systems
http://www.enfoldsystems.com
phone. +1.713.942.2377 Ext 215
fax. +1.832.201.8856