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

Simplify URL handling in pat-tinymce #1321

Open
thet opened this issue May 16, 2023 · 1 comment
Open

Simplify URL handling in pat-tinymce #1321

thet opened this issue May 16, 2023 · 1 comment

Comments

@thet
Copy link
Member

thet commented May 16, 2023

For internal URLs, don't insert the resolveuid URL‌ with the full path but instead insert it as relative-to-the-context URL. It's converted to a relative-to-the-context URL by TinyMCE urlconverter_callback anyways AFAIK. If we do that, we could also fully bypass the urlconverter_callback as suggested in: #1320 (comment)

See the whole discussion in #1320 (comment) and especially:

Actually, that kind of url converting is probably really needed. Otherwise the portal path could end up being added to the text, which is not what we want.

I was wondering why this resolveuid is inserted - with the - full path, where in this case a relative-to-the-context URL could be inserted in the first place and thus not needing to rely on the TinyMCE urlconverter_callback.

I did some software archeology. The appendToUrl setting was initially relative-to-the-context. It become an relative URL with the full path due to the wrong assumptions in this commit: plone/Products.CMFPlone@243773d - also see the comments there.

So, there would be another angle to fix the issue:‌ Reverting the change where the full path was inserted in CMFPlone, adding @@resolveuid instead only resuloveuid as mentioned in the comment of the commit referenced above and allowing an @@ in the resoloveuid_and_caption filter in plone.outputfilters. exploding_head

Since that's getting absurdly complex, let's stick with your PR as-is!

@petschki
Copy link
Member

There's maybe a similar problem solved? plone/plone.app.contenttypes#675

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