Variable interpolation for internal links is not operative, even in classic

[sneridagh]

In classic, it claims that variable interpolation still works:

but there's no way to set the link "manually" because the widget is in the middle, and forces you to choose it.

Bug in the navigation portlet (in classic, that revolves into the ContextNavigation component in Volto):

1. If you set up an internal link (using the widget)
2. make it public
3. Enable the navigation portlet to see the link content type listed
4. logout, check the navigation portlet

then it shows:

A resolveuid URL pointing to: https://volto.kitconcept.dev/Plone/resolveuid/d368b2a5931a454f8a1340035519b0a9

rooted in /Plone (even with a proper domain and VHM in place (the name of the Plone instance in ZMI).

So it kind of works, because the resolveuid and acquisition :) so the bug is hidden in classic but the internals are completely broken.

Back in p.restapi and Volto, we tried to comply with the classic spec, using the variable interpolation, but since it's broken, the result query to the catalog returns:

href (coming from https://github.com/plone/plone.app.contenttypes/blob/master/plone/app/contenttypes/indexers.py#L156) and getRemoteUrl index contents, index "/name_of_the_Plone_instance/whatever" then the ContextNavigation can't show correctly the remoteURL

[sneridagh]

My 5c, let's rip this out, even from Plone 6 at some point, since it's both unusable and broken.

[sneridagh]

Any reason to treat an internal link differently than an external one?

[tiberiuichim]

but there's no way to set the link "manually" because the widget is in the middle

Would it work if you use the External tab?

[tisto]

My 2c: let's rip it out. Both in Classic and Volto things should be hidden behind a proper widget.

[sneridagh]

but there's no way to set the link "manually" because the widget is in the middle

Would it work if you use the External tab?

Didn't have time to check :(

[mauritsvanrees]

String interpolation does work in classic UI, but it is a bit unintuitive. You have to go to External to fill it in, and you don't see the help text there. I did that, the link worked, and I checked with bin/instance debug and it was as I filled in: ${navigation_root_url}/Members. I then used the Internal tab and clicked to make a link to news. In debug I saw the result: ${portal_url}/resolveuid/3a26e134a9174b7d94158d0dd33ad339. So the backend tries to save it with interpolation if it can.

The Classic UI could be improved, maybe simply move some of the help text from internal to external, though I can imagine you don't want to be the one making this change. :-)

plone.restapi should be able to read such a value. I am not sure if it should report the original or the interpolated url. Probably the interpolated one makes most sense. Maybe report both.

plone.restapi should be able to write such a value, if the frontend (Volto or something else) explicitly asks for it. But I expect that Volto just passes on a resolveuid.

[sneridagh]

@mauritsvanrees from the user experience and usability, it's not unintuitive, it's just unusable and misleading... tell a user that just wants to put an internal link, that he has to set it in "external". Then explain to him (if it's not a power user or has a developer background) what a variable interpolation is, then explain what a navigation root is, and the difference from the portal root. After that, how many types of different navigation roots you can have in Plone...

Just too much trouble for the user for a so simple feature. The user only wants to set an internal link, and select it from a browser. The system has to convert that to resolveuid internally and resolve it back again on render. For power users, copy and paste support is nice (so it can come from other tab URL field copy), then the whole dance again. It's how the object browser widget in Volto works now, and I think, it's more than enough.

Simplify things if they are overcomplicated or half baked, or they just have degraded with time and refactor after refactor.

[tiberiuichim]

I'm trying to imagine scenarios where it's useful to have the variable interpolation and it isn't something that can be deduced by the system. I can only come up with "user has a link that's relative to the navigation root and wants to copy/paste the richtext in multiple locations". Seems like a really narrow use case.

[mauritsvanrees]

It's not about internal links in rich text. It is about the Link contenttype. At least: that is what I am talking about. I know no Plone version where interpolation works in rich text.

I am not saying we have to tell normal users to use interpolation if they just want to add an internal link. For them, the widget is fine. For them, we could remove the entire help text so we don't confuse them.

The interpolation is a feature for power users who know what they are doing, and are faster at typing ${portal_url}/my/deep/link than navigating or searching. Or they want to point to ${portal_url}/django-app which is a page that does not exist in Plone, but the webserver directs traffic to a Django app. And when they copy the production Data.fs to their laptop, they want these links to point to localhost:8080/Plone/django-app instead of the live site.

'External' is perhaps not the best name for the tab where they can do this, but it fits the 90/95 percent use case, so it's fine. The help text of this tab already says the url can also be relative within this site. The help text of the field could be moved here. In Plone 6 classic, the main help text is already under the widget, making it more likely that normal users will just use the internal widget without actually reading the help text:

So: move the help text and this works perfectly. I see no reason to rip this out of classic. In Volto we can choose differently.

Just for reference, the code for interpolating is in plone.app.contenttypes.utils.

[sneridagh]

@mauritsvanrees

It's not about internal links in rich text. It is about the Link contenttype. At least: that is what I am talking about. I know no Plone version where interpolation works in rich text.

I'm talking all the time about the link content type, not rich text.

I am not saying we have to tell normal users to use interpolation if they just want to add an internal link. For them, the widget is fine. For them, we could remove the entire help text so we don't confuse them.

If we are able to improve it, it would be great.

Just pointing to improvable things, and trying not to commit the same errors or inherit them in Volto UI again. I am aware that do this kind of changes in both sides would be very difficult and time consuming.

'External' is perhaps not the best name for the tab where they can do this, but it fits the 90/95 percent use case, so it's fine. The help text of this tab already says the url can also be relative within this site. The help text of the field could be moved here. In Plone 6 classic, the main help text is already under the widget, making it more likely that normal users will just use the internal widget without actually reading the help

If appears for all the tabs (and I guess it's something not easy fixable), we should amend and improve the help text ->

https://github.com/plone/plone.app.contenttypes/blob/master/plone/app/contenttypes/schema/link.xml#L8

https://github.com/plone/plone.app.contenttypes/issues/608

Also check this if you have time today (it will reset this midnight):

https://volto.kitconcept.dev/api

See the links in the sections and in the navigation portlet (as anonymous). It's broken... it works because a number of coincidences...

[mauritsvanrees]

Also check this if you have time today (it will reset this midnight):

https://volto.kitconcept.dev/api

See the links in the sections and in the navigation portlet (as anonymous). It's broken... it works because a number of coincidences...

I see.

Wait. From looking at your PR:

• In the current release, when the deserializer gets a remoteUrl value that begins with the url of the portal, then it stores the value as ${portal_url}/the-rest. With your PR it simply keeps the value, without trying to replace it. This change seems fine to me.
• With your PR, the serialiser stays the same, it still calls replace_link_variables_by_paths.
• Turning internal links into resolveuid links seems fine too.

Apparently in Classic (tried on 5.2) even when you use the widget to navigate to a page, the value that gets saved is ${portal_url}/resolveuid/abcdef.... I didn't expect this, and I think this is strange. You obviously knew this, but this wasn't initially clear to me. This issue and PR sounded like you wanted to rip out the entire string interpolation, but that seems not the idea after all.

What I would hope plone.restapi does, is to keep the string interpolation if it is already there:

• If the remoteUrl field contains a string interpolation, keep it: the serialiser should report it without change. The edit form can then display it like it is, and preferably set the focus on the External tab.
• If the frontend submits a remoteUrl field with string interpolation, keep it: the deserialiser should save it without change.
• In both cases though, it seems fine to rip out string interpolation when there is a valid resolveuid.

Does that make sense? I think your PR comes close to that. I only suppose that if the serialiser cannot find an object, then it should return the original value, instead of stripping away ${portal_url}.

[sneridagh]

@mauritsvanrees

The PR already had support for them, I wrote a couple of tests to proof it:

https://github.com/plone/plone.restapi/pull/1198/commits/e107dcffc0440107cdccc1c8fc988f048c23dcac https://github.com/plone/plone.restapi/pull/1198/commits/ce0f739c7dc42079faf96915fee0d85a1fcdf7eb

I think the PR is ready now.

[tisto]

@mauritsvanrees I had a look at the PR together with @sneridagh. IMHO we can merge this. Volto does not support the variable interpolation, because it does not make any sense to us, since it is not very user/editor friendly and the Classic UI is highly confusing IMHO. Therefore Plone Classic will continue to work as it is. Since Plone Classic does not use plone.restapi, we do not have any conflicting issues here whatsoever.

[mauritsvanrees]

First things first: I now think the PR is fine. Thanks for adding the extra tests!

I wanted to ask for one extra test in the serializer:

# If interpolation leads to an unknown document,
# it is likely a document outside of Plone hosted on the same domain.
# We do not touch the interpolation then.
dm.set("${portal_url}/an-external-app")
self.assertEqual(serializer(), "${portal_url}/an-external-app")

This test would fail, because the return value is stripped down to only "/an-external-app". And that is probably fine, because it boils down to the same thing. (Except locally you end up on localhost:8080/an-external-app which may not be what you want, but I will happily ignore that.)

So, after some thought, it seems fine now.

But I do want to react to this comment by @tisto:

Since Plone Classic does not use plone.restapi, we do not have any conflicting issues here whatsoever.

Plone Classic itself does not use plone.restapi, that is true. But Volto is not the only consumer of plone.restapi. You probably did not mean it like that, but that is how it sounds to me. Just last week I created an endpoint on top of plone.restapi for a client on Plone 5.2 without Volto. It does nothing with links, but that is not the point.

The point is that the PR changes plone.restapi to fix a bug in Volto, and this has the danger of breaking it for other users of plone.restapi. This is what I reacted to. In the end it turned out to be false alarm. But just because a change is good for Volto, does not mean it is a good change for plone.restapi.

Partially it also simply communication:

• This issue says that interpolation does not even work in classic. This is not true. The UI is confusing, but it works.
• The PR says it removes variable interpolation support. This is also not true. A restapi user can still PATCH a Link to set the remoteUrl to ${portal_url}/something. It is just saved a bit differently.

Anyway, it seems likely that sometime in the future one of us will make a change in restapi that unintentionally breaks Volto or breaks Classic. It will probably sooner be me, because I still hardly have experience with Volto. ;-) Let's be careful and hope that it won't happen too often.

As an afterthought: maybe some of these changes should have happened in plone.app.contenttypes itself. Turning the value into a resolveuid if possible, makes sense, to me anyway. But never mind that.

[tisto]

But I do want to react to this comment by @tisto:

Since Plone Classic does not use plone.restapi, we do not have any conflicting issues here whatsoever.

Plone Classic itself does not use plone.restapi, that is true. But Volto is not the only consumer of plone.restapi. You probably did not mean it like that, but that is how it sounds to me. Just last week I created an endpoint on top of plone.restapi for a client on Plone 5.2 without Volto. It does nothing with links, but that is not the point.

You are correct in assuming I never meant to say that Volto is the only consumer of Plone REST API, and if you look at my work in the past >5 years this is pretty obvious I guess. Though, Volto is the new default frontend for Plone 6 and therefore the main and most important consumer of Plone REST API. It should be treated like this, in case we have conflicts and problems that are not easily solvable (which is not the case in this PR).

I will try to generalize this a bit because I think this is an important and long-overdue discussion. Please bear with me. :)

Since day one of Volto (and plone.restapi) we are working around quirks and half-baked features of Plone, and this becomes more and more tiring and frustrating tbh. There are lots of features in Plone that have evolved over time and it is often unclear how they were meant to be used and if there are used at all (like the string interpolation). We are working our asses off to get those features right in REST API for both Volto and all possible consumers. Though our resources are limited and we can not both innovate and keep everything as it is at the same time.

Plone REST API is the connection point between the old and the new and it supports multiple Plone versions, multiple backends, and frontends at the same time. We did a pretty good job so far as a community to make this work. Though, we can not just assume that it will continue like this since forever. As said, our resources are limited and the number of people that possess the understanding and knowledge of how things work in Plone Classic and Volto at the same time is limited.

I am not saying we should deliberately break things if we don't have to, and I guess this PR and discussion shows that we do our best to keep things together. Though, at some point, we might have to make decisions and compromises. Volto is the new default frontend for Plone 6 and it is the main consumer of Plone REST API. Therefore it should be the priority, in case we have to make those compromises. Plone Classic does not use Plone REST API, if it would, lots of the problems that we had to solve would have been solved already (because they become obvious as soon as you try to have a sane and well-documented API for a feature).

Since Plone Classic does not use REST API any future decision would be a tradeoff between Volto and individual consumer apps that we know nothing about.

Therefore, if we have to make a decision (in the future) between having a broken feature in Volto (which is what we currently have) and having a missing feature in a third-party app that maybe someone uses (or not), the decision should be obvious. If we can not agree on this, we have to re-think how Volto is currently integrated into Plone.

REST API is a generic API for Plone for any consumer for sure, there is no question about that. Though, we just can not prioritize the imaginary usage of a feature over the new default frontend for Plone 6 (and I guess you never meant that in the first place).

If we just agree that Volto is not the only but the most important consumer of RESTAPI and we continue to do our best to keep both worlds together, as we did in the past, we are good I guess. :)

[mauritsvanrees]

It is good to have this discussion.

I agree that you and others did a lot of good work here. Probably at least 90 percent of plone.restapi was written by people who want to use it in Volto. Non-Volto users profit from this.

BTW, I can imagine that eventually Plone Classic will use plone.restapi for parts of its UI. For example, it may make sense to rewrite the CMFEditions templates to use the @versions endpoint. Or maybe the sharing tab. But those will be isolated spots.

With my Plone 5.2 release manager hat on, I could actually say: it does not matter what you do on plone.restapi master, because 5.2 will officially keep using 7.x.x. And in Plone 6 it is fine to have breaking changes. So maybe I should not look so closely at changes like these.

I agree that the first and foremost goal of RESTAPI is to be the bridge between Volto and Plone backend.

Its second goal is to be a general API, so it should be stable. In this specific case, the API stays the same: users can still call the same endpoint with the same arguments. It is just a slight change of behavior under the hood. There is a chance that someone needs to chance their code, but it seems small.

When Volto and Plone Classic move in a different direction in one part, the RESTAPI should bend and not break. I think we have the track record and skill to keep the breakage minimal. As you say: "we continue to do our best to keep both worlds together".

Now onwards to Plone 6, aka Volto!