Re: Changes to ZPublisher

G'day,

thought I'd throw in my 2c as a person who has written/dealt with quite
a few __bobo__traverse methods inside applications.

On Thu, 2004-10-07 at 00:00, Sidnei da Silva wrote:
> Hello,
> 
> I've made some changes on the way ZPublisher deals with
> __bobo_traverse__. 
> 
> Here's the branch:
> svn+ssh://svn.zope.org/repos/main/Zope/branches/dc-bobo_traverse-branch
> 
> Before the changes, exceptions on __bobo_traverse__ would not be
> handled, so if a AttributeError would happen, it would pop up
> untouched, resulting on a 500 response status, instead of 404.
> 
> It would also try __bobo_traverse__ only, if __bobo_traverse__ exists,
> completely ignoring __getattr__ and __getitem__.
> 
> After my change, AttributeError and KeyError in __bobo_traverse__
> will be converted to a NotFound, resulting in a 404 response status.

This seems fine to me at first glance. A colleague was concerned that
KeyError more often means you have screwed up your application code, so
converting them to a 404 might mask things.

> It will also continue looking through __getattr__ and __getitem__,
> which is where the semantics really differ. I'm more worried about
> changing the semantincs because there are other places where
> __bobo_traverse__ is used, namely restrictedTravers on OFS.Traversable
> and PageTemplates.Expressions.
> 
> This change would simplify the implementations of __bobo_traverse__,
> which would not have to worry about looking at __getattr__ and
> __getitem__ themselves, but I would like to know what people think
> first.

I think what you are suggesting is falling back to __getattr__ and
__getitem__ after __bobo_traverse__ raises a KeyError or AttributeError,
yeah? Or is that only on returning None?

I don't like this. At the application level we write __bobo_traverse__
methods to override how it behaves. This means we don't want it falling
back to default behaviour when our __bobo_traverse__ method says "no, 
you can't traverse there". Possibly even worse is falling back to
default behaviour when our application code is buggy and raises
unexpected exceptions.

Thinking about it some more, I guess I'm -1. 

Exceptions raised in __bobo_traverse__ currently mean, and usually
indicate, a bug in your application code. If you really want a 404
response, you do so by returning None. Changing things so that
AttributeError _also_ means 404, or even worse, means fall back to
default __getattr__ behaviour, will confuse things.

-- 
Donovan Baarda <[email protected]>
http://minkirri.apana.org.au/~abo/