traversing macro doesn't works, testing upgrade to Plone 5.2.2 (pending)

mamico commented 4 years ago

What I did:

Upgraded my buildout to Plone 5.2.2 (pending release)

What I expect to happen:

It is a minor change, everything should be fine!

What actually happened:

Some custom views are broken, Traceback:

...
Module zope.pagetemplate.engine, line 85, in __call__
Module zope.traversing.adapters, line 158, in traversePathElement
   - __traceback_info__: (<Document at /Plone/page>, 'custom_macros')
Module zope.traversing.adapters, line 46, in traverse
   - __traceback_info__: (<Document at /Plone/page>, 'custom_macros', ['foo'])
zope.location.interfaces.LocationError: zope.location.interfaces.LocationError: (<Document at /Plone/page>, 'custom_macros')

This is an excerpt from my code:

document.pt:

...
<div metal:use-macro="context/custom_macros/foo" />
...

configure.zcml:

  <browser:page
    for="*"
    name="custom_macros"
    template="macros.pt"
    permission="zope2.View"
    />

macros.pt:

<html xmlns="http://www.w3.org/1999/xhtml" 
      xml:lang="en" 
      xmlns:tal="http://xml.zope.org/namespaces/tal" 
      xmlns:metal="http://xml.zope.org/namespaces/metal">

  <metal:foo define-macro="foo">
    <div>macro</div>
  </metal:foo>
...
</html>

Differences between compiled templates in Chamelon cache:

Plone 5.2.1 / Zope 4.2.1:

from Products.PageTemplates.expression import TrustedBoboAwareZopeTraverse as _TrustedBoboAwareZopeTraverse
_static_140708896619584 = _TrustedBoboAwareZopeTraverse()
...
                        # <Value 'context/custom_macros/foo' (17:34)> -> __macro
                        __token = 758
                        __macro = _static_140708896619584(getitem('context'), econtext, True, ('custom_macros', 'foo', ))

Plone 5.2.2 / Zope 4.4.4:

from Products.PageTemplates.engine import _compile_zt_expr as __compile_zt_expr
_static_140007861886448 = __compile_zt_expr
...
                       # <Value 'context/custom_macros/foo' (17:34)> -> __macro
                        __token = 758
                        __macro = _static_140007861886448('path', 'context/custom_macros/foo', econtext=econtext)(_static_140007861886208(econtext))

There is something wrong with our code that for some reason was working on Plone 5.2.1/Zope 4.2.1 or a bug/change feature in Plone 5.2.2/Zope 4.4.4 ?

What version of Plone/ Addons I am using:

Plone 5.2.2-pending (Zope 4.4.4)

mauritsvanrees commented 4 years ago

Probably related to https://github.com/plone/Products.CMFPlone/issues/3135. Or rather, a different problem with likely the same cause: Zope 4.4 has changes in how chameleon/zope.tales/pagetemplates handle expressions.

mamico commented 4 years ago

If I change my tal expression like that, it works again:

- <div metal:use-macro="context/custom_macros/foo" />
+ <div metal:use-macro="context/@@custom_macros/foo" />

d-maurer commented 4 years ago

The traceback you report look really strange. It actually should have the form

  File "05b25f65b6569b8f428e6e1163bcfae3.py", line 112, in render
  Module zope.tales.expressions.py", line 241, in __call__
    return self._eval(econtext)
  Module Products.PageTemplates.Expressions.py", line 152, in _eval
    ob = self._subexprs[-1](econtext)
  Module zope.tales.expressions.py", line 146, in _eval
    ob = self._traverser(ob, element, econtext)
  Module Products.PageTemplates.Expressions.py", line 80, in boboAwareZopeTraverse
    request=request)
  Module zope.traversing.adapters.py", line 153, in traversePathElement

As you can see, the modules/functions mentioned in your traceback are different from those in the above traceback. The important function is boboAwareZopeTraverse - it implements path traversal and with it obj/xxx automatically falls back to obj/@@xxx. In your case, this function apparently not called.

Are you using a custom template class? Templates can define the method pt_getEngine. Its return value defines the TALES engine to be used for the template. Before Zope 4.4, this engine was mostly ignored; from Zope 4.4 on, it determines (as it should) how TALES expressions are interpreted. It looks that in your case a non-standard interpretation of "path expression"s is effective -- one not using boboAwareZopeTraverse.

Up to now, I have analyzed within a pure Zope setup (i.e. no Plone).I If you tell me that you are not using a custom template class, I will analyze in a true Plone setup.

d-maurer commented 4 years ago

The problem is very likely understood - but has no easy solution.

The intelligent path traversal (the above mentioned boboAwareZopeTraverse) is defined by Products.PageTemplates. The latter builds on zope.pagetemplate but its templates have additional Zope related intelligence (e.g for path traversal). Almost surely, the template in which you observe the problem is not a Products.PageTemplates template but (only) a zope.pagetemplate template. As explained earlier, the interpretation of TALES expressions for a template is determined by the return value of its pt_getEngine method. Before Zope 4.4, this return value was mostly ignored: it was just used to guess whether the template (likely) wants a trusted or an untrusted TALES implementation - all other things were ignored. This has hidden the problem. With Zope 4.4, the return value of a template's pt_getEngine is fully honored -- and some templates used in the Plone world (those directly deriving from zope.pagetemplate and not from Products.PageTemplates) effectively loose their Zope related intelligence.

I know that z3c.form derives its default templates directly from zope.pagetemplate. But other packages (originally developed for "Zope3") could do this as well.

d-maurer commented 4 years ago

When my assumption above is correct, then the problem will disappear if you give your template class a method pt_getEngine with the following definition:

from Products.PageTemplates.Expressions import getTrustedEngine
...
    def pt_getEngine(self):
        return getTrustedEngine()

mauritsvanrees commented 4 years ago

Quick thought: using the trusted engine is not a good idea for templates made TTW. Think PloneFormGen or the custom skin. But that should be less common these days.

Op za 4 jul. 2020 08:43 schreef Dieter Maurer notifications@github.com:

When my assumption above is correct, then the problem will disappear if you give your template class a method pt_getEngine with the following definition:

from Products.PageTemplates.Expressions import getTrustedEngine ... def pt_getEngine(self): return getTrustedEngine()

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/plone/Products.CMFPlone/issues/3141#issuecomment-653729610, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABTNGYWYQF3PQBG7BO5X2DRZ3FSDANCNFSM4OO3IENQ .

d-maurer commented 4 years ago

Maurits van Rees wrote at 2020-7-4 02:14 -0700:

Quick thought: using the trusted engine is not a good idea for templates made TTW. Think PloneFormGen or the custom skin. But that should be less common these days.

It is this what the former (Zope before 4.4) chameleon integration did: if the engine returned by the template's pt_getEngine is not Zope's standard untrusted engine (returned by Products.PageTemplates.Expressions.getEngine()), then use the integration's trusted engine.

Any template directly based on zope.pagetemplate (and not Products.PageTemplates) does not use Zope's standard untrusted engine - and effectively was rendered with a trusted engine whether or not the originally wanted engine was trusted or untrusted.

The problem here is that a TALES engine has no (easily inspectable) concept "trusted". Whether a given engine should be considered trusted or untrusted depends on implementation details of its supported expression types. For example, a trusted engine could use TrustedPythonExpr and TrustedZopePathExpr to implement its "python" and "path" expression types, while an untrusted engine could use RestrictedPythonExpr and ZopePathExpr. An untrusted zope.pagetemplate (in contrast to a Products.PageTemplates) engine would use PathExpr (rather than ZopePathExpr) to implement its path expressions. For those reasons, it is difficult to decide whether a given engine should be considered trusted or untrusted.

mamico commented 4 years ago

@d-maurer you are right. You seem to have hit all the points. I don't have a custom template class, but the template I ran into was the template of a z3c.form widget. In my own case, it seems that the problems are solved by changing TAL expressions.

vincentfretin commented 4 years ago

@mamico Can you test again with https://raw.githubusercontent.com/plone/buildout.coredev/5.2/versions.cfg and <div metal:use-macro="context/custom_macros/foo" /> and tell us if this is fixed?

vincentfretin commented 4 years ago

It should be fixed. Reopen if it's not. Thanks.

mamico commented 4 years ago

@vincentfretin

Can you test again with https://raw.githubusercontent.com/plone/buildout.coredev/5.2/versions.cfg and <div metal:use-macro="context/custom_macros/foo" /> and tell us if this is fixed?

Sorry... same error. (using https://raw.githubusercontent.com/plone/buildout.coredev/5.2/versions.cfg so Zope 4.5)

But, if I read correctly, Plone somewhere should define an IZopeAwareEngine adapter for z3c.form template. (I'm wrong?)

cc @d-maurer

d-maurer commented 4 years ago

Mauro Amico wrote at 2020-7-14 04:28 -0700:

@vincentfretin

Can you test again with https://raw.githubusercontent.com/plone/buildout.coredev/5.2/versions.cfg and <div metal:use-macro="context/custom_macros/foo" /> and tell us if this is fixed?

Sorry... same error. (using https://raw.githubusercontent.com/plone/buildout.coredev/5.2/versions.cfg so Zope 4.5)

But, if I read correctly, Plone somewhere should define an IZopeAwareEngine adapter for z3c.form template. (I'm wrong?)

cc @d-maurer

It is correct that Plone must register an adapter. However, it does not adapt a template but an engine (the engine used by the template).

If the main interest is to get Zope 4.3 behavior the adapter can be registered for "*" (i.e. anything) and simply return the Zope trusted engine. For a more faithful solution, the adapter would look at the engine to determine whether or not it trusted and then return the corresponding Zope engine.

mauritsvanrees commented 4 years ago

I can't reproduce this, given the steps you showed in your first comment. I tried with the current versions, and also older versions. I tried on python 2. Did you maybe try on Python 3 instead? It would be strange that this would cause such a difference, but it's possible.

I tried adding the template in document.pt in plone.app.contenttypes. Is that were you did this as well?

Can you create a branch of plone.app.contenttypes with your changes, so I know I am using the same code?

d-maurer commented 4 years ago

Maurits van Rees wrote at 2020-7-20 09:53 -0700:

I can't reproduce this, given the steps you showed in your first comment. I tried with the current versions, and also older versions. I tried on python 2. Did you maybe try on Python 3 instead? It would be strange that this would cause such a difference, but it's possible.

The template must come from a z3c.form: the problem never happened with a Products.PageTemplate template.

The reason: z3c.form uses templates directly based on zope.pagetemplate (and not Products.PageTemplates). zope.pagetemplate uses a TALES engine which a Zope unaware path expression. That said, you can reproduce the problem with any template directly derived von zope.pagetemplate.

mamico commented 4 years ago

@mauritsvanrees I'm sorry, opening the issue I had simplified the use case too much, the z3c.form part was missing. A simple product to reproduce my use case here: https://github.com/mamico/issue3141

mauritsvanrees commented 4 years ago

Thanks, that helps. I can reproduce the problem with your code.

If I understand correctly, the reason it fails here, is because context/custom_macros is not found: the chosen engine does not know about Zope, so it does not know about traversal and cannot reach custom_macros. Changing it to @@ custom_macros helps because the engine can find the view with this name .

I am preparing a PR.

d-maurer commented 4 years ago

Maurits van Rees wrote at 2020-7-20 13:31 -0700:

If I understand correctly, the reason it fails here, is because context/custom_macros is not found: the chosen engine does not know about Zope, so it does not know about traversal and cannot reach custom_macros. Changing it to @@ custom_macros helps because the engine can find the view with this name .

Correct.

mauritsvanrees commented 4 years ago

PR for Plone 5.2: https://github.com/plone/Products.CMFPlone/pull/3149

mauritsvanrees commented 4 years ago

PR merged. That should fix it on coredev 5.2.

plone / Products.CMFPlone