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

TinyMCE tries to be too smart with external links #1116

Closed
erral opened this issue Jan 25, 2022 · 14 comments
Closed

TinyMCE tries to be too smart with external links #1116

erral opened this issue Jan 25, 2022 · 14 comments

Comments

@erral
Copy link
Member

erral commented Jan 25, 2022

When we enter an external link pointing to the current portal TinyMCE tries to be too smart and tries to build an internal link from it

Our use-case is the following:

In our portal we have many URLs coming from an external database and their urls are traversal based URLs, for instance: http://portal.com/en/course/slug where course is the view that handles the traversal and slug the identifier we use to query the database.

If we want to link that URL from some other content in Plone, we need to add the link as an external link, but then TinyMCE starts doing a lot of fancy stuff with those external links and converts them to ../../../en/course/slug which in the end renders invalid links.

I think that when entering external links TinyMCE should just accept anything incoming and let it as is and not try to be too smart.

@erral
Copy link
Member Author

erral commented Jan 25, 2022

We are handling this overriding Plone's resolve_uid_and_caption filter, because TinyMCE does mark the link as an external with a data-linktype="external" and data-val="<original-url>" attributes, so we just shortcut the output filter to change the value of the href attribute entered by TinyMCE and override it with the value of the data-val attribute when data-linktype is external

@Rudd-O
Copy link

Rudd-O commented Jan 31, 2022

I think the right solution would be to simply not convert the link if it cannot be resolved to an UID. You are right that the assumption that the whole domain is handled by Plone is not a correct assumption (inside out hosting and all that stuff).

@petschki
Copy link
Member

I've just set other_options in the TinyMCE controlpanel to {"relative_urls": false} and it seems to fix this issue ... but I have to test this outside in the VirtualHostMonster world first.

@erral
Copy link
Member Author

erral commented May 16, 2023

I would say, quoting the Zen of Python "explicit is better than implicit": if I want to add an "external" link, leave it as it is please.

@petschki
Copy link
Member

OK, the url handling settings of TinyMCE only has these options: https://www.tiny.cloud/docs/configure/url-handling/
I think it won't help us if we set them to "local domain" urls since the editors can come from different (sub) domains editing the pages which should then resolve to the correct "absolute local" url for the viewers. So I propose to handle this in plone.outputfilters and check for data-linktype="externalImage(Link)" and replace the src with the correct absolute url from the REQUEST

@petschki
Copy link
Member

I came accross this one https://www.tiny.cloud/docs/configure/url-handling/#exampleusingurlconverter_callback ... we can use this to determine, whether we place an external or interal link and skip Tiny's url handling completely for external urls... since plone.outputfilters does not handle absolute urls at all I think its better to teach Tiny the Zen of Python/Plone 😄

@erral
Copy link
Member Author

erral commented May 16, 2023

I think that the internal link thing works pretty OK with Tiny and later UID based transformations.

Can't we just disable the external link handling, which I think is in a JS file of our own, and let plone.outputfilters handle it without transforming it at all?

@petschki
Copy link
Member

petschki commented May 16, 2023

Unfortunately this is a Tiny thing ... our plonelink/ploneimage plugins always generates absolute paths like /<plonesite>/resolveuid/xxx for internals or returns the external URL as is (even if its on the same domain) ... now Tiny comes and checks the src/href of the generated markup and rewrites it according to its relative_url and remove_script_host settings, and that resolves it per default to relative urls.

We now can hook into this behavior with the urlconverter_callback in pat-tinymce to leave the absolute url as is.

The transform itself has an early exit if he finds absolute url schemas and doesn't need to be changed.

As I'm in need for this too in a project, I'm about to prepare a PR now to outline my idea ... I'd be happy if you can review it then.

@erral
Copy link
Member Author

erral commented May 16, 2023

OK, if it's tiny's call... I will be happy to review it of course.

@thet
Copy link
Member

thet commented May 16, 2023

The PR #1320 in it's current state would break a scenario where different domains are used for the same site and people add links with absolute URLs pointing to the same site.

For example:

  • There is a public production site www.domain.com, a site for editors: edit.domain.com.
  • If editors don't use the reference widget and add links with absolute URLs pointing to the same site in edit.domain.com these links would be broken in www.domain.com.

That scenario would have been supported until now.

I‌ see these options:

  • We could argue that external links should never be transformed and apply Fix TinyMCE external url handling #1320 (breaking change).
  • We add a checkbox to not transform external links if they are from the same domain.

For reference, this is the Link widget from the Plone Classic UI demo:

image

@petschki
Copy link
Member

petschki commented May 16, 2023

I would say, TinyMCE should keep its fingers off from URL transforming completely. Maybe we should always return the original url in the urlconverter_callback and let plone.outputfilters do the logic in the transform chain.

@thet
Copy link
Member

thet commented May 16, 2023

Actually, I also favor bypassing the urlconverter_callback because it's just confusing if TinyMCE transforms URLs by itself.

If we agree that the scenario outlined above is not an issue, let's go forward with #1320 and let's even bypass the urlconverter at all.

It seems more natural to not do any url transformation when using the "External" link feature.

@petschki
Copy link
Member

The mentioned PR is merged and released in plone.staticresources>=2.0.11,>=2.1.2 so I'll close this.

@erral
Copy link
Member Author

erral commented May 17, 2023

Thanks!

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

4 participants