Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we be applying the ThemeTransform on FilesystemFile? #207

Open
frapell opened this issue Apr 13, 2022 · 3 comments
Open

Should we be applying the ThemeTransform on FilesystemFile? #207

frapell opened this issue Apr 13, 2022 · 3 comments

Comments

@frapell
Copy link
Member

frapell commented Apr 13, 2022

I am having an issue with a theme that defines some [theme:parameters] and a request that tries to get a plone.resource.file.FilesystemFile, such as Mosaic trying to get the Basic layout (Somethig like http://localhost:8080/Plone/++contentlayout++default/basic.html)

This particular issue happens in

params = prepareThemeParameters(
findContext(self.request),
self.request,
parameterExpressions,
cache
)

where findContext(self.request) would return a <FilesystemResourceDirectory object at default>

and so, you would get a zExceptions.NotFound

This can be reproduced by adding

[theme:parameters]
url = context/absolute_url

in plonetheme/barceloneta/theme/manifest.cfg and reapplying the theme. So when trying to edit a mosaic page you will get a generic Error loading layout specified for this content. Falling back to basic layout. which loops forever. And you can see in the terminal

2022-04-13 11:15:42,717 ERROR   [plone.transformchain:69][waitress-0] Unexpected error whilst trying to apply transform chain
Traceback (most recent call last):
  File "/trabajo/plone/buildout.coredev/eggs/plone.transformchain-2.0.2-py3.8.egg/plone/transformchain/transformer.py", line 59, in __call__
    newResult = handler.transformIterable(result, encoding)
  File "/trabajo/plone/buildout.coredev/eggs/plone.app.theming-5.0.0a5-py3.8.egg/plone/app/theming/transform.py", line 175, in transformIterable
    params = prepareThemeParameters(
  File "/trabajo/plone/buildout.coredev/eggs/plone.app.theming-5.0.0a5-py3.8.egg/plone/app/theming/utils.py", line 778, in prepareThemeParameters
    params[name] = quote_param(expression(expressionContext))
  File "/trabajo/plone/buildout.coredev/eggs/zope.tales-5.1-py3.8.egg/zope/tales/expressions.py", line 250, in __call__
    return self._eval(econtext)
  File "/trabajo/plone/buildout.coredev/src/Zope/src/Products/PageTemplates/Expressions.py", line 213, in _eval
    ob = self._subexprs[-1](econtext)
  File "/trabajo/plone/buildout.coredev/eggs/zope.tales-5.1-py3.8.egg/zope/tales/expressions.py", line 153, in _eval
    ob = self._traverser(ob, element, econtext)
  File "/trabajo/plone/buildout.coredev/src/Products.CMFPlone/Products/CMFPlone/earlypatches/expressions.py", line 173, in boboAwareZopeTraverse
    result = shared_traverse(object, path_items, request)
  File "/trabajo/plone/buildout.coredev/src/Products.CMFPlone/Products/CMFPlone/earlypatches/expressions.py", line 114, in shared_traverse
    found = traversePathElement(base, name, path_items,
  File "/trabajo/plone/buildout.coredev/eggs/zope.traversing-4.4.1-py3.8.egg/zope/traversing/adapters.py", line 156, in traversePathElement
    return traversable.traverse(nm, further_path)
  File "/trabajo/plone/buildout.coredev/eggs/zope.traversing-4.4.1-py3.8.egg/zope/traversing/adapters.py", line 58, in traverse
    return subject[name]
  File "/trabajo/plone/buildout.coredev/eggs/plone.resource-2.1.4-py3.8.egg/plone/resource/directory.py", line 252, in __getitem__
    return self.publishTraverse(None, name)
  File "/trabajo/plone/buildout.coredev/eggs/plone.resource-2.1.4-py3.8.egg/plone/resource/directory.py", line 244, in publishTraverse
    raise NotFound
zExceptions.NotFound

I believe there is no use in applying the ThemeTransform when returning a file from the filesystem. (As a side effect we would get a faster response time for resources).

Now, I am not 100% sure which Interface should be better to adapt. I've replaced Interface with ILocation (from zope.location.interfaces.ILocation) in @adapter(ILocation, IThemingLayer) and it does fix the issue, but I am not entirely sure if this is the Interface I should be using.

So, is this a desired change? if so, do you have a better suggestion on which Interface to use?

@frapell
Copy link
Member Author

frapell commented Apr 13, 2022

@mauritsvanrees @petschki @thet @MrTango @pbauer Thoughts?

@mauritsvanrees
Copy link
Member

I would expect the theme transform to be run on all files. Well, on Page Templates anyway. But really anything with html.

I wonder if it would help if the theme transform does not run on subrequests, or if that has unwanted side effects.

@frapell
Copy link
Member Author

frapell commented Apr 19, 2022

@mauritsvanrees Mmmh, I see your point, however it shouldn't fail, even on a direct request to the resource... Maybe what needs to be fixed then is the findContext(self.request) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants