Merge lp:~wallyworld/zope.pagetemplate/fix-isinstance into lp:zope.pagetemplate

[Ian Booth]

I have made a small improvement to the short circuit in the traversal logic if the object subclasses dict.

The code in engine.py used to say:

if getattr(object, '__class__', None) == dict:

It has been changed to:

if isinstance(object, dict):

The reason for the change is that I have made some optimisations to Launchpad menu rendering and now there's instances of MenuLinksDict(dict) classes instead of vanilla dicts. These new instances extend dict and so the original check in engine.py failed. My change arguably is the better way to do the check and it makes the short circuit work again for Launchpad menus :-)

[Tres Seaver (tseaver)]

Overall, the patch looks fine, but it needs a test showing that the
subclassing bit (the part you care about) works.

review: Needs Fixing

[lp:~wallyworld/zope.pagetemplate/fix-isinstance]

59. By Ian Booth on 2010-10-26

Add unit tests

[Ian Booth (wallyworld)]

Hi Tres

Thanks for the review. I've added a new unit test to test_engine.py
which verifies that the traversal works as expected. Let me know if
there's any issues.

On 26/10/10 11:22, Tres Seaver wrote:
> Review: Needs Fixing
> Overall, the patch looks fine, but it needs a test showing that the
> subclassing bit (the part you care about) works.

[Tres Seaver (tseaver)]

Thanks for adding the tests -- looks good.

review: Approve

[Colin Watson (cjwatson)]

Since this had apparently fallen off the radar and in any case failed tests when rebased, I resubmitted this in a modified form as https://github.com/zopefoundation/zope.pagetemplate/pull/3.

Unmerged revisions

59. By Ian Booth on 2010-10-26

Add unit tests

58. By Ian Booth on 2010-10-15

Improve isinstance call

=== modified file 'src/zope/pagetemplate/engine.py'
--- src/zope/pagetemplate/engine.py	2010-06-03 16:09:29 +0000
+++ src/zope/pagetemplate/engine.py	2010-10-26 04:26:42 +0000
@@ -57,9 +57,9 @@
 
         while path_items:
             name = path_items.pop()
-
+            
             # special-case dicts for performance reasons
-            if getattr(object, '__class__', None) == dict:
+            if isinstance(object, dict):
                 object = object[name]
             else:
                 object = traversePathElement(object, name, path_items,
@@ -141,10 +141,10 @@
         """evaluateMacro gets security-proxied macro programs when this
         is run with the zopeTraverser, and in other untrusted
         situations. This will cause evaluation to fail in
-        zope.tal.talinterpreter, which knows nothing of security proxies.
+        zope.tal.talinterpreter, which knows nothing of security proxies. 
         Therefore, this method removes any proxy from the evaluated
         expression.
-
+        
         >>> output = [('version', 'xxx'), ('mode', 'html'), ('other', 'things')]
         >>> def expression(context):
         ...     return ProxyFactory(output)

=== modified file 'src/zope/pagetemplate/tests/test_engine.py'
--- src/zope/pagetemplate/tests/test_engine.py	2010-06-03 16:09:29 +0000
+++ src/zope/pagetemplate/tests/test_engine.py	2010-10-26 04:26:42 +0000
@@ -14,6 +14,7 @@
 """Doc tests for the pagetemplate's 'engine' module
 """
 import unittest
+from zope.traversing.interfaces import ITraversable
 
 class DummyNamespace(object):
 
@@ -79,12 +80,52 @@
         self.assertRaises(NameError, expr, DummyContext())
 
 
+class ZopeTraverserTests(unittest.TestCase):
+
+    class TraverserDict(dict):
+        def __init__(self):
+            self.item_requested = None
+
+        def __getitem__(self, item):
+            self.item_requested = item
+            return dict.__getitem__(self, item)
+
+    class DummyTraverser():
+        implements = ITraversable
+
+        def __init__(self, obj):
+            self.traversed = obj
+
+        def traverse(self, name, furtherPath):
+            return name+self.traversed
+
+    def test_dict_shortcut(self):
+        from zope.pagetemplate.engine import ZopeTraverser
+        traverser = ZopeTraverser()
+
+        obj = ZopeTraverserTests.TraverserDict()
+        obj['foo'] = 'bar'
+        result = traverser(obj, ['foo'], {})
+        self.assertEqual(obj.item_requested, 'foo')
+        self.assertEqual('bar', result)
+
+    def test_nondict_traversal(self):
+        from zope.component import provideAdapter
+        from zope.pagetemplate.engine import ZopeTraverser
+        traverser = ZopeTraverser()
+        provideAdapter(
+            ZopeTraverserTests.DummyTraverser, (None,), ITraversable, name='')
+
+        result = traverser('bar', ['foo'], {})
+        self.assertEqual('foobar', result)
+
 def test_suite():
     from doctest import DocTestSuite
     suite = unittest.TestSuite()
     suite.addTest(DocTestSuite('zope.pagetemplate.engine'))
     suite.addTest(unittest.makeSuite(EngineTests))
     suite.addTest(unittest.makeSuite(ZopePythonExprTests))
+    suite.addTest(unittest.makeSuite(ZopeTraverserTests))
     return suite