Repository: zopefoundation/zope.tales

Issue:
"zope.tales.tales.Context" has bad "getValue"
`zope.tales.tales.Context` sets up (among others) attributes `vars` (the current "variable -> value" mapping) and `_vars_stack` (a stack of such mappings). Thereby, `_vars_stack[0]` represents the initial and `_vars_stack[-1]` the most recent variable mapping. One would expect that `getValue(self, name, default)` is defined as `self.vars.get(name, default)` but the definition instead looks like:
```
    def getValue(self, name, default=None):
        value = default
        for vars in self._vars_stack:
            value = vars.get(name, default)
            if value is not default:
                break
        return value
```
i.e. it prefers older bindings over newer ones.

Discussion:
@mgedmin: How did this bug went undiscovered for so long?

Do you have a high-level example (e.g. a page template) of where it shows up?@d-maurer: Marius Gedminas wrote at 2020-3-26 02:03 -0700:
>How did this bug went undiscovered for so long?

Templates do not use `getValue` (but directly `vars`).

The bug would affect only applications/components which use
the official API (the interface describes `getValue` but not `vars`)
and not the implementation details (`vars`).

>Do you have a high-level example (e.g. a page template) of where it shows up?

No.

I currently discuss with @malthe the differences between
`chameleon`s `Scope` and its equivalent `zope.tales.tales.Context`
(see conversation around
"https://github.com/malthe/chameleon/issues/301#issuecomment-604264247").
In this context, I looked at the code and found the unexpected `getValue`
definition.

This discussion also indicates that `Context` implements `setGlobal`
in a peculiar way: a `setGlobal` is able to override a local variable binding.
I believe that this is buggy.
@jamadden: > How did this bug went undiscovered for so long?

https://github.com/zopefoundation/zope.tales/pull/20/files/1130a2352beaccc106d360a70d89c70d2200052e#diff-99a3b96df81fef7a2df80eb23ff4733c describes how the current scope is always a complete copy of all previous contexts, so the only answer we need is in the current scope.

>> This discussion also indicates that `Context` implements `setGlobal`
in a peculiar way: a `setGlobal` is able to override a local variable binding.
I believe that this is buggy.

That same comment should show that the changes in that PR make this behavior of `setGlobal` critical and definitely not a bug. If it stops doing what it's doing, then copying `vars` into every scope stops working correctly (or vice versa; the two behaviours are linked).@malte: Hello, please take me out of this email-List, i am mistaken for another
"Malte"
all the best, Malte Euler

Am Fr., 27. März 2020 um 12:12 Uhr schrieb Jason Madden <
notifications@github.com>:

> How did this bug went undiscovered for so long?
>
>
> https://github.com/zopefoundation/zope.tales/pull/20/files/1130a2352beaccc106d360a70d89c70d2200052e#diff-99a3b96df81fef7a2df80eb23ff4733c
> describes how the current scope is always a complete copy of all previous
> contexts, so the only answer we need is in the current scope.
>
> This discussion also indicates that Context implements setGlobal
> in a peculiar way: a setGlobal is able to override a local variable
> binding.
> I believe that this is buggy.
>
> That same comment should show that the changes in that PR make this
> behavior of setGlobal critical and definitely not a bug. If it stops
> doing what it's doing, then copying vars into every scope stops working
> correctly (or vice versa; the two behaviours are linked).
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/zopefoundation/zope.tales/issues/19#issuecomment-604943699>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAEMC2LCHUQPFRMI3FFEFDRJSC2JANCNFSM4LUANOZQ>
> .
>


-- 

Malte Euler
Karl-Marx-Straße 15, 12043 Berlin
Mob 0179 / 66666 14
www.malteeuler.com
@jamadden: @malte I've updated the tag to refer to the intended @malthe, but you may need to manually unsubscribe from this issue in Github (there is an 'unsubscribe' link at the bottom of email notifications). Sorry for the confusion!@d-maurer: Jason Madden wrote at 2020-3-27 04:12 -0700:
> ...
>>> This discussion also indicates that `Context` implements `setGlobal`
>in a peculiar way: a `setGlobal` is able to override a local variable binding.
>I believe that this is buggy.
>
>That same comment should show that the changes in that PR make this behavior of `setGlobal` critical and definitely not a bug.

It is a "behavioural" bug as it allows a macro use to effectively
change a (local) variable value - which can lead to real surprises.

>If it stops doing what it's doing, then copying `vars` into every scope stops working correctly (or vice versa; the two behaviours are linked).

I am aware that it cannot be easily changed (and I did not try).


Solution patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index a199b94..262ae5e 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -9,6 +9,10 @@
 
 - Drop support for Python 3.7, 3.8.
 
+- ``tales.Context.getValue`` now returns the innermost (not outermost)
+  value for the given variable. Fixes
+  `#19 <https://github.com/zopefoundation/zope.tales/issues/19>`_.
+
 
 6.0 (2023-07-12)
 ================
diff --git a/src/zope/tales/tales.py b/src/zope/tales/tales.py
index b11a131..fea8ebb 100644
--- a/src/zope/tales/tales.py
+++ b/src/zope/tales/tales.py
@@ -737,12 +737,16 @@ def setGlobal(self, name, value):
             vars[name] = value
 
     def getValue(self, name, default=None):
-        value = default
-        for vars in self._vars_stack:
-            value = vars.get(name, default)
-            if value is not default:
-                break
-        return value
+        """return the current value of variable *name* or *default*."""
+        # ``beginScope`` puts a copy of all variables into ``vars``
+        #  and pushes it onto ``_vars_stack``,
+        # ``endScope`` pops the last element of ``vars_stack`` into ``vars``,
+        # ``setGlobal`` updates all variable bindings in ``_vars_stack``
+        # (and thereby, implicitly, ``vars``).
+        # Consequently, the current value of a variable can
+        # always be found in ``vars``
+        # (no need to iterate over ``_vars_stack``).
+        return self.vars.get(name, default)
 
     def setRepeat(self, name, expr):
         expr = self.evaluate(expr)


Test patch:
diff --git a/src/zope/tales/tests/test_tales.py b/src/zope/tales/tests/test_tales.py
index dffce63..47e4068 100644
--- a/src/zope/tales/tests/test_tales.py
+++ b/src/zope/tales/tests/test_tales.py
@@ -130,6 +130,7 @@ def testVariables(self):
 
         c = ctxt.vars
         self.assertEqual(c['v1'], 1, 'Variable "v1"')
+        self.assertEqual(ctxt.getValue('v1'), 1, 'Variable "v1"')
 
         ctxt.beginScope()
         ctxt.setLocal('v1', 3)
@@ -137,14 +138,19 @@ def testVariables(self):
 
         c = ctxt.vars
         self.assertEqual(c['v1'], 3, 'Inner scope')
+        self.assertEqual(ctxt.getValue('v1'), 3, 'Inner scope')
         self.assertEqual(c['v2'], 2, 'Outer scope')
+        self.assertEqual(ctxt.getValue('v2'), 2, 'Outer scope')
         self.assertEqual(c['g'], 1, 'Global')
+        self.assertEqual(ctxt.getValue('g'), 1, 'Global')
 
         ctxt.endScope()
 
         c = ctxt.vars
         self.assertEqual(c['v1'], 1, "Uncovered local")
+        self.assertEqual(ctxt.getValue('v1'), 1, "Uncovered local")
         self.assertEqual(c['g'], 1, "Global from inner scope")
+        self.assertEqual(ctxt.getValue('g'), 1, "Global from inner scope")
 
         ctxt.endScope()
 
@@ -216,7 +222,7 @@ def test_getValue_nested(self):
         self.context.vars['it'] = 1
         self.context.beginScope()
         self.context.vars['it'] = 2
-        self.assertEqual(self.context.getValue('it'), 1)
+        self.assertEqual(self.context.getValue('it'), 2)
 
     def test_evaluate_boolean(self):
         # Make sure it always returns a regular bool, no matter