-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: Fix error when Plone site is not yet set as in first index_html call #21
Conversation
src/z3c/jbot/utility.py
Outdated
@@ -24,7 +25,7 @@ def getRequest(): | |||
try: | |||
return site.request | |||
except AttributeError: | |||
return site.REQUEST | |||
return getattr(site, "REQUEST", Zope2.app().REQUEST) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code always calls Zope2.app().REQUEST
but it only seems to be needed in rare cases. I'd suggest to refactor this line, so Zope2.app().REQUEST
is only computed when actually needed.
Michael Howitz wrote at 2024-11-27 23:28 -0800:
@icemac commented on this pull request.
> @@ -24,7 +25,7 @@ def getRequest():
try:
return site.request
except AttributeError:
- return site.REQUEST
+ return getattr(site, "REQUEST", Zope2.app().REQUEST)
This code always calls `Zope2.app().REQUEST` but it only seems to be needed in rare cases. I'd suggest to refactor this line, so `Zope2.app().REQUEST` is only computed when actually needed.
In addition: `Zope2.app().REQUEST` will not return a request object
but `'<Special Object Used to Force Acquisition>'` (i.e.
`Acquisition.Acquired`).
It is unlikely that this is a good return value for `getRequest`.
Maybe, `None` will do instead.
|
So Im' going to test "return None" instead of "return getattr(site, "REQUEST", Zope2.app().REQUEST)" on my use case. And if I haven't any errors I will change my PR |
Benoît Suttor wrote at 2024-11-28 00:53 -0800:
So Im' going to test "return None" instead of "return getattr(site, "REQUEST", Zope2.app().REQUEST)" on my use case. And if I haven't any errors I will change my PR
What about `return getattr(site, "REQUEST", None)`?
|
Indeed, lot better |
Pull Request Test Coverage Report for Build 12069008348Details
💛 - Coveralls |
Just released in https://pypi.org/project/z3c.jbot/2.1/. |
In first index_html call on Zope, for example on http://localhost:8080/index_html, REQUEST for Plone Site is not yet set.
If you go directly on Plone, for example on http://localhost:8080/Plone/index_html, request is set to Plone Site (by acquisition) and you have not this error.
This fix is only usefull for first call on http://localhost:8080/index_html