1letter/fix#671 #675

1letter · 2023-10-19T12:28:41Z

Fix for #671

better constructing of the url, use explicit the portal url

mister-roboto · 2023-10-19T12:28:50Z

@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 · 2023-10-19T12:30:44Z

@jenkins-plone-org please run jobs

ale-rt · 2023-10-19T13:00:16Z

plone/app/contenttypes/browser/link_redirect_view.py

+                    (self.context, self.request), name="plone_portal_state"
+                )
+                portal_url = portal_state.portal_url()
+
                if obj:
                    url = "/".join(obj.getPhysicalPath()[2:])


I might be wrong, but if this [:2] is to remove something like "/app/Plone", then it is not valid...
You can have Plone site inside other objects, e.g. a Folder.
e.g. the path might be /app/folder/Plone or /app/folder/subfolder/Plone.

@ale-rt right, i will refactor this

Consider also VHM rules, and the use of _vh_ (for example:

location /zope { proxy_pass http://localhost:8080/VirtualHostBase/https/site.com/VirtualHostRoot/_vh_zope; }

using absolute_url() will do it.

pbauer · 2023-10-19T13:09:52Z

Stupid question: Why not simply use obj.absolute_url() for all of that?

yurj · 2023-10-19T13:41:59Z

Stupid question: Why not simply use obj.absolute_url() for all of that?

The uid will resolve to a catalog path. There's already a call in Zope to resolve from a catalog path to the object:
https://github.com/zopefoundation/Zope/blob/29e088e04db9feb9f5d3bc6bea8950d98f8283a1/src/ZPublisher/HTTPRequest.py#L244

so I think we just simply need to use this Zope function.

Looking better, I was wrong. The uid is resolved to the object itself, so you're plain right.

petschki · 2023-10-19T13:49:21Z

plone/app/contenttypes/browser/link_redirect_view.py

-                    url = "/".join(obj.getPhysicalPath()[2:])
-                    if not url.startswith("/"):
-                        url = "/" + url
+                url = obj.absolute_url()


I'd check for obj first (keep the if from line 114) ... it might be that the uuid is missing.

also uid = url.split("/")[-1] above is fragile ("/resolveuid/" vs "resolveuid/" vs "/a/relativepath/resolveuid"). Generally, you should find the index of 'resolveuid' in the path, and get the next one if it exists. It should be something like this (uid = getuid(path)):

def get_uid(path): paths = path.split('/') uid = None if 'resolveuid' in paths: ri = paths.index('resolveuid') if ri + 1 != len(paths): uid = paths[ri + 1] if uid == '': uid = None return uid

>>> get_uid('gfdgdf') >>> get_uid('') >>> get_uid('/resolveuid/') >>> get_uid('/resolveuidgfdgd/') >>> get_uid('/resolveuid/4342432') '4342432' >>> get_uid('/resolveuid/4342432/') '4342432' >>> get_uid('fdsfd/fdf/resolveuid/4342432/') '4342432' >>> get_uid('/fdsfd/fdf/resolveuid/4342432/') '4342432' >>>

Also /resolveuid/fb91bddde7eb46efbed20e9c10fb4929#autotoc-item-autotoc-1 should still work.

is resolve[uU]id still a thing?

@petschki maybe, but i'm not sure

At least plone.outputfilters still takes it into account: https://github.com/plone/plone.outputfilters/blob/master/plone/outputfilters/filters/resolveuid_and_caption.py#L31

def get_uid(path): paths = path.split('/') uid = None if 'resolveuid' in paths: ri = paths.index('resolveuid') if ri + 1 != len(paths): uid = paths[ri + 1] if '#' in uid: uid = uid.split('#')[0] if uid == '': uid = None return uid

>>> get_uid('gfdgdf') >>> get_uid('') >>> get_uid('/resolveuid/') >>> get_uid('/resolveuidgfdgd/') >>> get_uid('/resolveuid/4342432') '4342432' >>> get_uid('/resolveuid/4342432/') '4342432' >>> get_uid('fdsfd/fdf/resolveuid/4342432/') '4342432' >>> get_uid('/fdsfd/fdf/resolveuid/4342432/') '4342432' >>> get_uid('/resolveuid/fb91bddde7eb46efbed20e9c10fb4929#autotoc-item-autotoc-1') 'fb91bddde7eb46efbed20e9c10fb4929' >>>

@1letter solution to use urlparse is very good! Question: uid's are all lowercase? What if an UID has uppercase letters?

@yurj i use a copy of the list with lowercase values https://github.com/plone/plone.app.contenttypes/pull/675/files#diff-c9937c907b85ad438b19cb4ce7abe10606d94a443dc66bef1661a216690f3547R44

At least plone.outputfilters still takes it into account: https://github.com/plone/plone.outputfilters/blob/master/plone/outputfilters/filters/resolveuid_and_caption.py#L31

https://github.com/plone/plone.outputfilters/blob/278e2752c4790b5af2470bfc3c91a8aa26a9a80f/plone/outputfilters/filters/resolveuid_and_caption.py#L130

here is what plone.output filter does. It uses the re above to get all parts of the url. If there's an uid, find the object, translate it to the absolute_url, add the other parts.

If we want to do this the same way outputfilter does, we should use this code. This code also check IResolveUidsEnabler. Maybe this last part is not important (if a site disable uids and use resolveuid as a folder name...).

Generally, resoulveuid and catalog path resolution should be done in one place, I think in plone.api. I don't know about circular dipendencies...

Anyway, the actual code is ok, should works as expected and it is a real improvement.

yurj · 2023-10-19T13:51:22Z

absolute_url()

BTW, absolute_url() uses physicalPathToVirtualPath:

https://github.com/zopefoundation/Zope/blob/29e088e04db9feb9f5d3bc6bea8950d98f8283a1/src/OFS/Traversable.py#L51 (absolute_url() -> self.virtual_url_path())
https://github.com/zopefoundation/Zope/blob/29e088e04db9feb9f5d3bc6bea8950d98f8283a1/src/OFS/Traversable.py#L102C50-L102C75 (self.virtual_url_path() -> physicalPathToVirtualPath)

- tests for get_uid_from_path

- Make resolution of uid more robust - calculate a fragment of anchor if available

yurj

At least plone.outputfilters still takes it into account: https://github.com/plone/plone.outputfilters/blob/master/plone/outputfilters/filters/resolveuid_and_caption.py#L31

https://github.com/plone/plone.outputfilters/blob/278e2752c4790b5af2470bfc3c91a8aa26a9a80f/plone/outputfilters/filters/resolveuid_and_caption.py#L130

here is what plone.output filter does. It uses the re above to get all parts of the url. If there's an uid, find the object, translate it to the absolute_url, add the other parts.

If we want to do this the same way outputfilter does, we should use this code. This code also check IResolveUidsEnabler. Maybe this last part is not important (if a site disable uids and use resolveuid as a folder name...).

Generally, resoulveuid and catalog path resolution should be done in one place, I think in plone.api. I don't know about circular dipendencies...

Anyway, the actual code is ok, should works as expected and it is a real improvement.

yurj · 2023-10-20T11:55:21Z

plone/app/contenttypes/browser/link_redirect_view.py

-                    url = "/".join(obj.getPhysicalPath()[2:])
-                    if not url.startswith("/"):
-                        url = "/" + url
+                url = obj.absolute_url()


At least plone.outputfilters still takes it into account: https://github.com/plone/plone.outputfilters/blob/master/plone/outputfilters/filters/resolveuid_and_caption.py#L31

https://github.com/plone/plone.outputfilters/blob/278e2752c4790b5af2470bfc3c91a8aa26a9a80f/plone/outputfilters/filters/resolveuid_and_caption.py#L130

here is what plone.output filter does. It uses the re above to get all parts of the url. If there's an uid, find the object, translate it to the absolute_url, add the other parts.

If we want to do this the same way outputfilter does, we should use this code. This code also check IResolveUidsEnabler. Maybe this last part is not important (if a site disable uids and use resolveuid as a folder name...).

Generally, resoulveuid and catalog path resolution should be done in one place, I think in plone.api. I don't know about circular dipendencies...

Anyway, the actual code is ok, should works as expected and it is a real improvement.

add empty line at the end of news file

1letter · 2023-10-20T13:45:42Z

@jenkins-plone-org please run jobs

pbauer

LGTM

1letter · 2023-10-23T11:40:47Z

close Broken internal links #671
respect fragment id's in links

1letter added 2 commits October 19, 2023 14:27

Fix link_redirect_view

a251928

Add News

08635f7

Fix Test

f64ecd9

ale-rt reviewed Oct 19, 2023

View reviewed changes

1letter added 2 commits October 19, 2023 15:30

Fix path construction

4fae87e

Simplify url constrction in link_redirect_view

9a2e653

remove unused imports

7693b1a

petschki reviewed Oct 19, 2023

View reviewed changes

1letter marked this pull request as draft October 20, 2023 06:40

1letter added 2 commits October 20, 2023 09:24

Make resolution of uid more robust

6f805c8

Update Tests

e4c5d65

- tests for get_uid_from_path

1letter added the 22 status: in-progress label Oct 20, 2023

1letter added 2 commits October 20, 2023 10:36

Calculate a fragment of path

68f1f42

- Make resolution of uid more robust - calculate a fragment of anchor if available

Update tests

798963f

1letter marked this pull request as ready for review October 20, 2023 09:52

1letter removed the 22 status: in-progress label Oct 20, 2023

1letter requested review from pbauer, petschki and yurj October 20, 2023 09:57

yurj approved these changes Oct 20, 2023

View reviewed changes

Update 671.bugfix

614d5a4

add empty line at the end of news file

pbauer approved these changes Oct 23, 2023

View reviewed changes

petschki approved these changes Oct 23, 2023

View reviewed changes

1letter merged commit 5907233 into master Oct 23, 2023
13 checks passed

1letter deleted the 1letter/fix#671 branch October 23, 2023 11:40

petschki mentioned this pull request Oct 24, 2023

Simplify URL handling in pat-tinymce plone/mockup#1321

Open

yurj mentioned this pull request Oct 24, 2023

Fix TinyMCE external url handling plone/mockup#1320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1letter/fix#671 #675

1letter/fix#671 #675

1letter commented Oct 19, 2023

mister-roboto commented Oct 19, 2023

1letter commented Oct 19, 2023

ale-rt Oct 19, 2023

1letter Oct 19, 2023

yurj Oct 19, 2023

yurj Oct 19, 2023

pbauer commented Oct 19, 2023

yurj commented Oct 19, 2023 •

edited

Loading

petschki Oct 19, 2023

yurj Oct 19, 2023 •

edited

Loading

pbauer Oct 20, 2023

petschki Oct 20, 2023

1letter Oct 20, 2023

petschki Oct 20, 2023 •

edited

Loading

yurj Oct 20, 2023

yurj Oct 20, 2023

1letter Oct 20, 2023

yurj Oct 20, 2023

yurj commented Oct 19, 2023

yurj left a comment

yurj Oct 20, 2023

1letter commented Oct 20, 2023

pbauer left a comment

1letter commented Oct 23, 2023

1letter/fix#671 #675

1letter/fix#671 #675

Conversation

1letter commented Oct 19, 2023

mister-roboto commented Oct 19, 2023

1letter commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbauer commented Oct 19, 2023

yurj commented Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

yurj Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petschki Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurj commented Oct 19, 2023

yurj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1letter commented Oct 20, 2023

pbauer left a comment

Choose a reason for hiding this comment

1letter commented Oct 23, 2023

yurj commented Oct 19, 2023 •

edited

Loading

yurj Oct 19, 2023 •

edited

Loading

petschki Oct 20, 2023 •

edited

Loading