[Zope-dev] zope.traversing's ILocation behavior

[Martijn Faassen]

Hi there,

in zope.traversing.browser.absoluteurl there was a change a while back 
that breaks my code, now that I'm upgrading to it.

The code in the absolute URL generation code used to be this:

          container = getattr(context, '__parent__', None)

This simply gets the __parent__ of the context so that it can ask for 
its absolute URL. If no container can be found the answer will be None 
and the system will raise a TypeError, complaining about insufficient 
context.

The code was then changed to this:

         context = ILocation(context, context)
         container = getattr(context, '__parent__', None)

While this code looks initially confusing, what it does is try to adapt 
context to ILocation, but if it fails, falls back to 'context' to look 
up parent, preserving backwards compatibility but adding the feature 
that people can register custom ILocation adapters for models.

But then, in dependency work by Christian Theune, the code got changed 
to this:

         context = ILocation(context)
         container = getattr(context, '__parent__',  None)

In addition, in zope.location an adapter got added for everything 
(Interface) that creates a LocationProxy.

This means that the ILocation adaptation *always* succeeds, and might 
return a location proxy with __parent__ and __name__ set to None.

This rather confused me initially, as this is one of those magic 
transparent proxies (except for __parent__ and __name__!). 'context' has 
a __parent__ *until* the point where ILocation() is called on it, and 
then the proxied context suddenly loses it __parent__!

This broke the behavior of code that relied on __parent__ being checked 
first. I realize that this was depending implicit behavior, as models 
don't *really* provide ILocation. But on the other hand working code is 
broken in a really obscure way.

The fix in my code was to make my model provide ILocation itself, so 
that it would adapt itself and therefore avoid the proxy. I think that 
might be correct but at the same time is really obscure; in my code the 
__parent__ is provided by a completely different subsystem (traject). 
Models that didn't provide ILocation used to work, and now they fail. Do 
we really want to require everybody to start providing ILocation in all 
their models?

I propose the following adjustment:

   try:
      container = context.__parent__
   except AttributeError:
      container = ILocation(context).__parent__

So, try to find the __parent__ the old way first, without adapter 
lookup. If that fails because the attribute is missing, look up the 
adapter. That might get one the proxy again, but at least not when a 
__parent__ is actually available. Still, this deserves some commenting 
for future people trying to debug what's going on.

I've verified that at least in my application this unbreaks the code. 
I'll also try to add a test that demonstrates the expected behaviors so 
that this doesn't get broken again.

Opinions?

Regards,

Martijn

P.S. I want a publisher with location support but without proxy magic. :)