Merge pull request #1374 from pallets/docs-globals

Template globals use ChainMap, more docs about globals

4 files changed, 139 insertions, 78 deletions

diff --git a/CHANGES.rst b/CHANGES.rst
index 7c0284f..26c928b 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -36,8 +36,9 @@ Unreleased
     being accessed in custom context functions. :issue:`768`
 -   Fix a bug that caused scoped blocks from accessing special loop
     variables. :issue:`1088`
--   Fix a bug that prevented cached templates from registering new globals.
-    :issue:`295`
+-   Update the template globals when calling
+    ``Environment.get_template(globals=...)`` even if the template was
+    already loaded. :issue:`295`
 
 
 Version 2.11.3
diff --git a/docs/api.rst b/docs/api.rst
index 47ecb9f..9ae47ef 100644
--- a/docs/api.rst
+++ b/docs/api.rst
@@ -114,10 +114,10 @@ useful if you want to dig deeper into Jinja or :ref:`develop extensions
 
     .. attribute:: globals
 
-        A dict of global variables.  These variables are always available
-        in a template.  As long as no template was loaded it's safe
-        to modify this dict.  For more details see :ref:`global-namespace`.
-        For valid object names have a look at :ref:`identifier-naming`.
+        A dict of variables that are available in every template loaded
+        by the environment. As long as no template was loaded it's safe
+        to modify this. For more details see :ref:`global-namespace`.
+        For valid object names see :ref:`identifier-naming`.
 
     .. attribute:: policies
 
@@ -180,9 +180,20 @@ useful if you want to dig deeper into Jinja or :ref:`develop extensions
 
     .. attribute:: globals
 
-        The dict with the globals of that template.  It's unsafe to modify
-        this dict as it may be shared with other templates or the environment
-        that loaded the template.
+        A dict of variables that are available every time the template
+        is rendered, without needing to pass them during render. This
+        should not be modified, as depending on how the template was
+        loaded it may be shared with the environment and other
+        templates.
+
+        Defaults to :attr:`Environment.globals` unless extra values are
+        passed to :meth:`Environment.get_template`.
+
+        Globals are only intended for data that is common to every
+        render of the template. Specific data should be passed to
+        :meth:`render`.
+
+        See :ref:`global-namespace`.
 
     .. attribute:: name
 
@@ -814,12 +825,27 @@ A template designer can then use the test like this:
 The Global Namespace
 --------------------
 
-Variables stored in the :attr:`Environment.globals` dict are special as they
-are available for imported templates too, even if they are imported without
-context.  This is the place where you can put variables and functions
-that should be available all the time.  Additionally :attr:`Template.globals`
-exist that are variables available to a specific template that are available
-to all :meth:`~Template.render` calls.
+The global namespace stores variables and functions that should be
+available without needing to pass them to :meth:`Template.render`. They
+are also available to templates that are imported or included without
+context. Most applications should only use :attr:`Environment.globals`.
+
+:attr:`Environment.globals` are intended for data that is common to all
+templates loaded by that environment. :attr:`Template.globals` are
+intended for data that is common to all renders of that template, and
+default to :attr:`Environment.globals` unless they're given in
+:meth:`Environment.get_template`, etc. Data that is specific to a
+render should be passed as context to :meth:`Template.render`.
+
+Only one set of globals is used during any specific rendering. If
+templates A and B both have template globals, and B extends A, then
+only B's globals are used for both when using ``b.render()``.
+
+Environment globals should not be changed after loading any templates,
+and template globals should not be changed at any time after loading the
+template. Changing globals after loading a template will result in
+unexpected behavior as they may be shared between the environment and
+other templates.
 
 
 .. _low-level-api:
diff --git a/src/jinja2/environment.py b/src/jinja2/environment.py
index e7c42cd..2bbdcb4 100644
--- a/src/jinja2/environment.py
+++ b/src/jinja2/environment.py
@@ -5,6 +5,7 @@ import os
 import sys
 import typing as t
 import weakref
+from collections import ChainMap
 from functools import partial
 from functools import reduce
 
@@ -811,59 +812,76 @@ class Environment:
             if template is not None and (
                 not self.auto_reload or template.is_up_to_date
             ):
-                # update globals if changed
-                new_globals = globals.items() - template.globals.items()
-                if new_globals:
-                    # it is possible for the template and environment to share
-                    # a globals object, in which case, a new copy should be
-                    # made to avoid affecting other templates
-                    if template.globals is self.globals:
-                        template.globals = dict(template.globals, **globals)
-                    else:
-                        template.globals.update(dict(new_globals))
+                # template.globals is a ChainMap, modifying it will only
+                # affect the template, not the environment globals.
+                if globals:
+                    template.globals.update(globals)
+
                 return template
-        template = self.loader.load(self, name, globals)
+
+        template = self.loader.load(self, name, self.make_globals(globals))
+
         if self.cache is not None:
             self.cache[cache_key] = template
         return template
 
     @internalcode
     def get_template(self, name, parent=None, globals=None):
-        """Load a template from the loader.  If a loader is configured this
-        method asks the loader for the template and returns a :class:`Template`.
-        If the `parent` parameter is not `None`, :meth:`join_path` is called
-        to get the real template name before loading.
-
-        The `globals` parameter can be used to provide template wide globals.
-        These variables are available in the context at render time.
-
-        If the template does not exist a :exc:`TemplateNotFound` exception is
-        raised.
+        """Load a template by name with :attr:`loader` and return a
+        :class:`Template`. If the template does not exist a
+        :exc:`TemplateNotFound` exception is raised.
+
+        :param name: Name of the template to load.
+        :param parent: The name of the parent template importing this
+            template. :meth:`join_path` can be used to implement name
+            transformations with this.
+        :param globals: Extend the environment :attr:`globals` with
+            these extra variables available for all renders of this
+            template. If the template has already been loaded and
+            cached, its globals are updated with any new items.
+
+        .. versionchanged:: 3.0
+            If a template is loaded from cache, ``globals`` will update
+            the template's globals instead of ignoring the new values.
 
         .. versionchanged:: 2.4
-           If `name` is a :class:`Template` object it is returned from the
-           function unchanged.
+            If ``name`` is a :class:`Template` object it is returned
+            unchanged.
         """
         if isinstance(name, Template):
             return name
         if parent is not None:
             name = self.join_path(name, parent)
-        return self._load_template(name, self.make_globals(globals))
+
+        return self._load_template(name, globals)
 
     @internalcode
     def select_template(self, names, parent=None, globals=None):
-        """Works like :meth:`get_template` but tries a number of templates
-        before it fails.  If it cannot find any of the templates, it will
-        raise a :exc:`TemplatesNotFound` exception.
+        """Like :meth:`get_template`, but tries loading multiple names.
+        If none of the names can be loaded a :exc:`TemplatesNotFound`
+        exception is raised.
+
+        :param names: List of template names to try loading in order.
+        :param parent: The name of the parent template importing this
+            template. :meth:`join_path` can be used to implement name
+            transformations with this.
+        :param globals: Extend the environment :attr:`globals` with
+            these extra variables available for all renders of this
+            template. If the template has already been loaded and
+            cached, its globals are updated with any new items.
+
+        .. versionchanged:: 3.0
+            If a template is loaded from cache, ``globals`` will update
+            the template's globals instead of ignoring the new values.
 
         .. versionchanged:: 2.11
-            If names is :class:`Undefined`, an :exc:`UndefinedError` is
-            raised instead. If no templates were found and names
+            If ``names`` is :class:`Undefined`, an :exc:`UndefinedError`
+            is raised instead. If no templates were found and ``names``
             contains :class:`Undefined`, the message is more helpful.
 
         .. versionchanged:: 2.4
-           If `names` contains a :class:`Template` object it is returned
-           from the function unchanged.
+            If ``names`` contains a :class:`Template` object it is
+            returned unchanged.
 
         .. versionadded:: 2.3
         """
@@ -874,7 +892,7 @@ class Environment:
             raise TemplatesNotFound(
                 message="Tried to select from an empty list of templates."
             )
-        globals = self.make_globals(globals)
+
         for name in names:
             if isinstance(name, Template):
                 return name
@@ -888,9 +906,8 @@ class Environment:
 
     @internalcode
     def get_or_select_template(self, template_name_or_list, parent=None, globals=None):
-        """Does a typecheck and dispatches to :meth:`select_template`
-        if an iterable of template names is given, otherwise to
-        :meth:`get_template`.
+        """Use :meth:`select_template` if an iterable of template names
+        is given, or :meth:`get_template` if one name is given.
 
         .. versionadded:: 2.3
         """
@@ -901,18 +918,40 @@ class Environment:
         return self.select_template(template_name_or_list, parent, globals)
 
     def from_string(self, source, globals=None, template_class=None):
-        """Load a template from a string.  This parses the source given and
-        returns a :class:`Template` object.
+        """Load a template from a source string without using
+        :attr:`loader`.
+
+        :param source: Jinja source to compile into a template.
+        :param globals: Extend the environment :attr:`globals` with
+            these extra variables available for all renders of this
+            template. If the template has already been loaded and
+            cached, its globals are updated with any new items.
+        :param template_class: Return an instance of this
+            :class:`Template` class.
         """
         globals = self.make_globals(globals)
         cls = template_class or self.template_class
         return cls.from_code(self, self.compile(source), globals, None)
 
     def make_globals(self, d):
-        """Return a dict for the globals."""
-        if not d:
-            return self.globals
-        return dict(self.globals, **d)
+        """Make the globals map for a template. Any given template
+        globals overlay the environment :attr:`globals`.
+
+        Returns a :class:`collections.ChainMap`. This allows any changes
+        to a template's globals to only affect that template, while
+        changes to the environment's globals are still reflected.
+        However, avoid modifying any globals after a template is loaded.
+
+        :param d: Dict of template-specific globals.
+
+        .. versionchanged:: 3.0
+            Use :class:`collections.ChainMap` to always prevent mutating
+            environment globals.
+        """
+        if d is None:
+            d = {}
+
+        return ChainMap(d, self.globals)
 
 
 class Template:
diff --git a/tests/test_regression.py b/tests/test_regression.py
index 945061a..a49356b 100644
--- a/tests/test_regression.py
+++ b/tests/test_regression.py
@@ -717,36 +717,31 @@ End"""
         # show up outside of it
         assert tmpl.render() == "42\n0\n24\n0\n42\n1\n24\n1\n42"
 
-    def test_cached_extends(self):
+    @pytest.mark.parametrize("op", ["extends", "include"])
+    def test_cached_extends(self, op):
         env = Environment(
             loader=DictLoader(
-                {"parent": "{{ foo }}", "child": "{% extends 'parent' %}"}
+                {"base": "{{ x }} {{ y }}", "main": f"{{% {op} 'base' %}}"}
             )
         )
-        tmpl = env.get_template("child", globals={"foo": "bar"})
-        assert tmpl.render() == "bar"
+        env.globals["x"] = "x"
+        env.globals["y"] = "y"
 
-        tmpl = env.get_template("parent", globals={"foo": 42})
-        assert tmpl.render() == "42"
+        # template globals overlay env globals
+        tmpl = env.get_template("main", globals={"x": "bar"})
+        assert tmpl.render() == "bar y"
 
-        tmpl = env.get_template("child")
-        assert tmpl.render() == "bar"
-
-        tmpl = env.get_template("parent")
-        assert tmpl.render() == "42"
-
-    def test_cached_includes(self):
-        env = Environment(
-            loader=DictLoader({"base": "{{ foo }}", "main": "{% include 'base' %}"})
-        )
-        tmpl = env.get_template("main", globals={"foo": "bar"})
-        assert tmpl.render() == "bar"
+        # base was loaded indirectly, it just has env globals
+        tmpl = env.get_template("base")
+        assert tmpl.render() == "x y"
 
-        tmpl = env.get_template("base", globals={"foo": 42})
-        assert tmpl.render() == "42"
+        # set template globals for base, no longer uses env globals
+        tmpl = env.get_template("base", globals={"x": 42})
+        assert tmpl.render() == "42 y"
 
+        # templates are cached, they keep template globals set earlier
         tmpl = env.get_template("main")
-        assert tmpl.render() == "bar"
+        assert tmpl.render() == "bar y"
 
         tmpl = env.get_template("base")
-        assert tmpl.render() == "42"
+        assert tmpl.render() == "42 y"