[Grok-dev] How I Will Have Broken Grok

[Brandon Craig Rhodes]

I have just checked in a change to "zope.traverser" that will destroy
Grok when it gets released, but, by a happy coincidence, I just ran
across this fact by accident while chasing down another question I had
about the Grok source code. :-)

My change improves the AbsoluteURL adapter, which - as I have already
described on this list - failed to adapt its context to ILocation
before accessing "__name__" and "__parent__".  My change fixes this.

But my recent change also removes the extraneous "_getContextName"
method on "AbsoluteURL", because:

  (a) That method was used by "AbsoluteURL.__str__" but not by
      "AbsoluteURL.breadcrumbs", whereas the two functions were
      clearly intended to work the same.

  (b) It does something perfectly obvious for "__name__" that's done
      in-line already in the code that grabs "__parent__".

  (c) It is mentioned nowhere else in the Zope 3 code base.

  (d) To be correct as a stand-alone call, "_getContextName" would
      also have to adapt its context to ILocation, which would have
      meant two adaption calls per call to "AbsoluteURL.__str__".

  (e) Most importantly: because no tests failed because of its
      removal. ;-)

So you can imagine my surprise when I ran across the following code in
grok.components!

------------------------------------------------------------------------
class GrokViewAbsoluteURL(AbsoluteURL):

    def _getContextName(self, context):
        return getattr(context, '__view_name__', None)
    # XXX breadcrumbs method on AbsoluteURL breaks as it does not use
    # _getContextName to get to the name of the view. What does breadcrumbs do?
------------------------------------------------------------------------

So I was not the only one wondering why "breadcrumbs" worked
differently. :-)

So, I had to ask: what on earth is this "GrokViewAbsoluteURL"?

Well, it turns out that it's a way to work around the fact that
AbsoluteURL was not properly coded. :-) In "grok/configure.zcml", we
find the comment:

------------------------------------------------------------------------
  <!-- we register special IAbsoluteURL views on grok views so that
       can have them inspect __view_name__ instead of __name__.  
       __name__ is already used as the class name, and overriding it
       may make error messages more confusing.  -->
------------------------------------------------------------------------

With AbsoluteURL now adapting properly, of course, I think the
solution becomes much simpler; we need to remove "GrokViewAbsoluteURL"
and in its place add something sleek like (but maybe with true ZCML in
place of the "grok" functions I'm using for the sake of example?):

------------------------------------------------------------------------
class GrokViewLocation(object):
    grok.implements(ILocation)
    grok.context(IGrokView)

    def __init__(self, context):
        self.__name__ = context.__view_name__
        self.__parent__ = context.__parent__
------------------------------------------------------------------------

Therefore:

 - Now that I know what it was for, I conclude that I was entirely
   correct to remove "_getContextName" from AbsoluteURL.  It was an
   unfortunate example of encouraging old-fashioned inheritance as a
   way to monkey around a problem that should have been solved cleanly
   with an adapter.

 - Whoever added the above class to Grok should probably have added a
   test case to Zope so that "_getContextName" would keep working.

 - I need some help, because this is very deep water, package-release-
   wise!  How do we coordinate having "grok" and "zope.traversing"
   change at exactly the same time so that they continue working for
   everyone?

   I suppose that we may need to put the terrible "_getContextName"
   back in place in "zope.traversing" for a few weeks, to give us time
   to calmly move Grok from using "GrokViewAbsoluteURL" to using
   something like my suggested "GrokViewLocation" above, and then
   finally release a version of "zope.traversing" that removes
   "_getContextName" once things look safe.

   Let me know.  This is my first experience with a product released
   as so many pieces. :-)

-- 
Brandon Craig Rhodes   brandon at rhodesmill.org   http://rhodesmill.org/brandon