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.

Sign up for GitHub

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

Jump to bottom

Fix TinyMCE external url handling #1320

Merged
merged 1 commit into from
May 17, 2023
Merged

Fix TinyMCE external url handling #1320

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/pat/tinymce/tinymce--implementation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,20 @@ export default class TinyMCE {
}
}

// add "urlconverter_callback" to leave external URLs/Images as is
tinyOptions["urlconverter_callback"] = (url, node, on_save, name) => {
if (url.indexOf("http") === 0) {
// if url starts with "http" return it as is
return url;
}
// otherwise default tiny behavior
if (self.tiny.settings.relative_urls) {
return self.tiny.documentBaseURI.toRelative(url);
}
url = self.tiny.documentBaseURI.toAbsolute(url, self.tiny.settings.remove_script_host);
Comment on lines +282 to +291
Copy link
Member

Choose a reason for hiding this comment

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

According to the discussion on #1116 (comment) ff, this can be simplified to:

Suggested change
tinyOptions["urlconverter_callback"] = (url, node, on_save, name) => {
if (url.indexOf("http") === 0) {
// if url starts with "http" return it as is
return url;
}
// otherwise default tiny behavior
if (self.tiny.settings.relative_urls) {
return self.tiny.documentBaseURI.toRelative(url);
}
url = self.tiny.documentBaseURI.toAbsolute(url, self.tiny.settings.remove_script_host);
tinyOptions["urlconverter_callback"] = (url) => {

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just tried that locally and this breaks plone.outputfilters regex matching resolveuid in an url. So no transformation is done for /Plone/resolveuid/<uid> which is returned by the plonelink/ploneimage plugin for internal links/images.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's interesting. Then just ignore my suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

yap ... unfortunately this cannot work without further changes to plone.outputfilters though it's a simple non-breaking change to the resolveuid_re from

resolveuid_re = re.compile("^[./]*resolve[Uu]id/([^/]*)/?(.*)$")

to

resolveuid_re = re.compile("^.*/resolve[Uu]id/([^/]*)/?(.*)$")

otherwise url patterns like /<plonesiteid>/resolveuid/<uid> will not get resolved.

Note that this is what you get if you edit your Plone site without virtual hosting. My local virtualhosting test resolves to /resolveuid/<uid> which the outputfilter would match though ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's be not too intrusive for now and keep this as a fix for the external urls. We can fix/generalize internal url transformation later...

Copy link
Member

@thet thet May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

<thinking/>

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. 🤯

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

Copy link
Member

Choose a reason for hiding this comment

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

Created an issue out of this possible enhancement: #1321

Copy link
Contributor

Choose a reason for hiding this comment

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

yap ... unfortunately this cannot work without further changes to plone.outputfilters though it's a simple non-breaking change to the resolveuid_re from

resolveuid_re = re.compile("^[./]*resolve[Uu]id/([^/]*)/?(.*)$")

to

resolveuid_re = re.compile("^.*/resolve[Uu]id/([^/]*)/?(.*)$")

otherwise url patterns like /<plonesiteid>/resolveuid/<uid> will not get resolved.

Note that this is what you get if you edit your Plone site without virtual hosting. My local virtualhosting test resolves to /resolveuid/<uid> which the outputfilter would match though ...

As in plone/plone.app.contenttypes#675 the problem is that plone.outputfilters and the WYSIWYG editor (TinyMCE) does not share the same code for UID resolving. The plone.outputfilters uid resolver should be an util imported and exposed by a plone.api call which than TinyMCE can call via a browser view. The same code should be imported in plone.restapi. So one place, one policy, one service and everybody is happy.

return url;
}

tinymce.init(tinyOptions);
self.tiny = tinymce.get(self.tinyId);

Expand Down