Using "${navigation_root_url}" in Link edit on External lead to a wrong url

[yurj]

plone.app.contenttypes/plone/app/contenttypes/utils.py

Lines 50 to 52 in 450888e

	def _replace_variable_by_path(url, variable, obj):
	path = "/".join(obj.getPhysicalPath())
	return url.replace(variable, path)

I think this should use:

from plone.base.navigationroot import get_navigation_root
get_navigation_root(obj)

because getPhysicalPath() contains '/Plone/' (or the path to Plone instance). On localhost:8080 this works ok. But with VHM and web site with a domain, a link using ${navigation_root_url} returns the navigation root + '/Plone' + path from the nav root instead of the navigation root + path from the nav root.

You can see easily on https://classic.demo.plone.org/en/demo/a-link:

• login as Manager
• edit the link
• on "External" insert ${navigation_root_url}/sitemap and Save

you get this:

with the link pointing to:

https://classic.demo.plone.org/Plone/en/sitemap (note the /Plone part)

[yurj]

on localhost they return the same value ('/en' is a nav root):

(Pdb) pp get_navigation_root(obj)
'/Plone/en'
(Pdb) pp "/".join(obj.getPhysicalPath())
'/Plone/en'

but on a site with VHM still returns /Plone/en instead of /en :-/

[yurj]

This function is a no sense. It just return the Zope object path instead of the virtual path from the root of the site. People expect ${navigation_root_url} to be replaced with https://mysite.com/mynav_root/ instead they get /mynav_root.

What is the use case here?

[yurj]

BTW, we've the greatest Zope function to manipulate urls an paths. If we want to get the path relative to the portal or navigation root for an object we've the zope REQUEST physicalPathToURL function. So we can use it with getPhysicalPath and relative = 1 to have the relative url:

obj.REQUEST.physicalPathToURL(obj.getPhysicalPath(), relative=1)

This works with every combination of VHM (including _vh_, using mysite.com/Plone/ as public address) and with navigation root.

If you omit relative=1 (or use relative=0) you get the full http address, basically + REQUEST['SERVER_URL'].

This should replace every '/'.join(obj.getPhysicalPath()[2:]) we have in Plone code like in:

plone.app.contenttypes/plone/app/contenttypes/browser/link_redirect_view.py

Line 114 in 450888e

meta = "/".join(obj.getPhysicalPath()[2:])

@petschki @1letter what do you think? I think this could be also a plone.api improvment, so people haven't to mess up with paths/relative urls.

[petschki]

it should be enough to do meta = "/" + obj.absolute_url(1) in your mentioned line above.

For variable replacing I'd also assume to see the full url (because it says so) so we should consider renaming those util functions herer https://github.com/plone/plone.app.contenttypes/blob/master/plone/app/contenttypes/utils.py#L29-L52 to replace_ ... _by_url and use absolute_url() on the objects. This ensures correct urls for (non)vh hostet sites.

[1letter]

earlier in this year, i stumpled over url calculation and caching see: plone/plone.app.event#391

we should checked many use cases ;-)

[petschki]

Oh ... thanks for the reminder 👍🏼

[yurj]

it should be enough to do meta = "/" + obj.absolute_url(1) in your mentioned line above.

https://github.com/zopefoundation/Zope/blob/04b75cf7956425fe3e4e1f4184bac4d377502c59/src/OFS/Traversable.py#L62-L65

absolute_url_path() is very similiar, but slightly better so we can use obj.absolute_url_path() instead of obj.REQUEST.physicalPathToURL(obj.getPhysicalPath(), relative=1)

For variable replacing I'd also assume to see the full url (because it says so) so we should consider renaming those util functions herer https://github.com/plone/plone.app.contenttypes/blob/master/plone/app/contenttypes/utils.py#L29-L52 to replace_ ... _by_url and use absolute_url() on the objects. This ensures correct urls for (non)vh hostet sites.

That is why I've asked what was the use case. The original (11 year ago circa) did this:

                if "${navigation_root_url}" in context.remoteUrl:
                    navigation_root_url = portal_state.navigation_root_url()
                    url = context.remoteUrl.replace(
                        "${navigation_root_url}",
                        navigation_root_url
                    )

and did it the full url way. Than a refactoring changed it to use relative urls.

I think it is safe to use absolute_url directly with no relative paths. But I would also fix:

plone.app.contenttypes/plone/app/contenttypes/browser/link_redirect_view.py

Line 114 in 450888e

meta = "/".join(obj.getPhysicalPath()[2:])

with

meta = "/" + obj.absolute_url_path()

@1letter
This:
https://github.com/plone/plone.app.event/blob/3ffad5f4934cbf91754dd58613f4d00bbec7ed55/plone/app/event/dx/behaviors.py#L349-L350
don't work with VHM subsites with virtual urls and /PloneSiteID sites, for example. You fixed it in:

https://github.com/plone/plone.app.event/blob/aedf39df2ef91c60a9cc48bb6429e961f85b7fdc/plone/app/event/dx/behaviors.py#L356-L362

Doing:

    def url(self):
        """calculate the path, is required as long the ram.cache is activ in portlets renderer
        the return value of self.context.absolute_url() differs
        with ram cache: portal/testtermin
        without ram cache: http://site.local/testevent
        """
        portal_url = api.portal.get().absolute_url()
        event_path =self.context.absolute_url_path()
        url = f"{portal_url}/{path_without_portal}"

is maybe better and don't add the getRequest() stuff, less code too. But the original bug would be in RAMCache here.

[yurj]

we've this clone too!

https://github.com/plone/plone.app.z3cform/blob/3bf3c73d7139cd1a2fddeb7aa694e3b7b1734b9e/plone/app/z3cform/utils.py#L68

used in the converter:

https://github.com/plone/plone.app.z3cform/blob/3bf3c73d7139cd1a2fddeb7aa694e3b7b1734b9e/plone/app/z3cform/converters.py#L524

but https://github.com/plone/plone.restapi/blob/main/src/plone/restapi/serializer/dxfields.py

uses:

from plone.app.contenttypes.utils import replace_link_variables_by_paths

[yurj]

#711 I've created a PR on it

[1letter]

Should replace_link_variables_by_paths moved to plone.base ?

[petschki]

we've this clone too!

that should definitely be fixed! moving to plone.base.utils in order to avoid cyclic dependencies would be great.

[yurj]

it involves plone.app.contenttypes, plone.base, plone.app.z3cform and (optionally) plone.restapi. I've opened a specific issue: #712

[davisagli]

Fixed in #711 and #713