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

Fix for #387 - Fix calculation of eventaccessor urls #391

Merged
merged 10 commits into from
Apr 3, 2024

Conversation

1letter
Copy link
Contributor

@1letter 1letter commented Apr 2, 2024

Eventaccessor urls are diffrent if the come from ram cache or not. in some enviroments the links are not accessible. This fix extends the url calculation.

@mister-roboto
Copy link

@1letter thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@1letter 1letter requested review from thet and petschki April 2, 2024 08:19
@@ -1,6 +1,7 @@
"""Behaviors to enable calendarish event extension to dexterity content types.
"""

from plone import api
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core packages do not use plone.api... use

from zope.component.hooks import getSite

portal = getSite()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for hint. the import is removed.

@1letter
Copy link
Contributor Author

1letter commented Apr 2, 2024

@jenkins-plone-org please run jobs

@@ -1,5 +1,6 @@
from datetime import datetime
from datetime import timedelta
from dateutil.relativedelta import relativedelta
Copy link
Member

@petschki petschki Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces a new dependency. (see dependency checker)

@@ -45,8 +46,9 @@ def make_dates(self):
past = self.past = tz.normalize(now - timedelta(days=10))
future = self.future = tz.normalize(now + timedelta(days=10))
far = self.far = tz.normalize(now + timedelta(days=30))
scifi = self.scifi = tz.normalize(now + relativedelta(years=50))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with now + 50 * timedelta(days=365) and remove the dateutil import above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this is a little bit incorrect (leap year), this year have 366 days ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but for the test it could be okay

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you insist, you can add dateutil to setup.py in [test] extra_require ... but I do not know if the dependency checker is smart enough for "test dependencies" only ...

Comment on lines 351 to 355
portal_path = list(portal.getPhysicalPath())
event_path = list(self.context.getPhysicalPath())
path_without_portal = ""
if len(portal_path) > 0:
path_without_portal = "/".join(event_path[len(portal_path) :])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: in your issue you've mentioned the comments in OFS.Traversal ... there the method absolute_url_path() is mentioned. Would this help here too? Something like

f"{portal.absolute_url()}/{self.context.absolute_url_path()}"

https://github.com/zopefoundation/Zope/blob/c33b799be066e3e72d6d3ef0b37e0b63199ed9bf/src/OFS/Traversable.py#L79

Copy link
Contributor Author

@1letter 1letter Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the portal id is in the path segments and should be removed. Background: i have configured WEBDAV via apache2

# apache2 WEBDAV
<Location /my-site/>
  Dav On
  ProxyPass http://127.0.0.1:9090/my-site/
  ProxyPassReverse http://127.0.0.1:9090/my-site/
</Location>
[client1]
<= client_base
http-address    = ${buildout:ip_base}:${buildout:client_port_base}81 0.0.0.0:9090
zope-conf-additional +=
  webdav-source-port 9090
  http-realm Welcome at Zeo Client1

The URL which is generate in the portlet, point to the WEBDAV Location on the second load. The portal id is not needed in the url.

The call of f"{portal.absolute_url()}/{self.context.absolute_url_path()}" generate:

http://localhost:8080/my-site//my-site/my-event

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for asking. But it is strange that this change is needed at all. self.context.absolute_url() should always be an URL not a path as you mentioned in your issue. It might be a deeper problem in the caching logic introduced here 15696d9 from @frapell which absolutely makes sense in terms of performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to apologise.

first call without caching:

portal.absolute_url(): http://localhost:8080/my-site
self.context.absolute_url(): http://localhost:8080/my-site/my-event
self.context.absolute_url(True): my-site/my-event
self.context.absolute_url_path(): /my-site/my-event

second call with cache

portal.absolute_url(): http://localhost:8080/my-site
self.context.absolute_url(): /my-site/my-event
self.context.absolute_url(True): my-site/my-event
self.context.absolute_url_path(): /my-site/my-event

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes from @frapell calculates only the cache key. for me it feels right, that no hostname is in the url, if the object come from ram cache. but it's only a feeling not evidence. I'm not a Zope Guru ;-)

@petschki
Copy link
Member

petschki commented Apr 2, 2024

I've made some tests... the main reason for this is that on the first call self.REQUEST in OFS.Traversable.absolute_url is

<WSGIRequest, URL=http://localhost:8080/Plone2/events/in-der-zukunft/event_view>

and on the second (cached) call its

<WSGIRequest, URL=None>

which leads to the path only result.

I've simplified your solution to this which works for me as expected:

    @property
    def url(self):
        """ need to lookup globalrequest in order to calculate
            correct URL during cached lookup (eg. in event portlet renderer)
        """
        request = getRequest()
        absolute_url = request.physicalPathToURL(self.context.getPhysicalPath())
        return safe_text(absolute_url)

what do you think?

I'm also 99% sure that we do not need safe_text anymore in version 5.x ...

@1letter
Copy link
Contributor Author

1letter commented Apr 3, 2024

what do you think?

Thanks for help and the input! Great. I have adopted your suggestions. My local tests works for me as expected, include a multilingual setup.

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

Successfully merging this pull request may close these issues.

3 participants