Routes fail validation in link field widgets

Created a patch to fix the issue.

Hi,

Reviewed patch #3 and it applied successfully for the core module Link on drupal versions: 8.9.x-dev and 9.0.0-beta2. And also works fine as now able to edit the title and saved it successfully instead of getting validation error as mentioned above.

Thanks & Regards
Mrinalini

Changing issue priority.

Closed #3209656: LinkWidget::getUriAsDisplayableString() can not handle route: URIs as duplicate of this issue.

It's unclear right now whether route: schemas are intended to be supported by the link field. The method used by the link field to generate URLs can easily handle routes, however routes are generally a developer tool and not well understood by content editors. I think we need some guidance, maybe from the UX team, regarding whether it should be possible to link to routes in all link widgets.

If the answer is no, then we need some more validation to rule out route: schemas (bar the exceptions).

If the answer is yes, then we need to discuss a way forward. Personally I think leaving the schema visible when displaying the value is both the easiest approach and the most understandable to the end user.

Usability review

We discussed this issue at #3253969: Drupal Usability Meeting 2021-12-17. That issue has a link to a recording of the meeting.

One consideration overrides usability concerns: we cannot make changes that break existing sites. If there are sites that currently have route:... in some of their link fields, then we cannot invalidate them. I think we cannot take away the option of editing them, either.

If we are going to keep the feature, then we should document it. Unfortunately, that means adding some pretty technical information to the help text (description).

Looking at the configuration of a link field, I see that there are already several options: internal/external/either; link text required/allowed/forbidden. We could add another option: allow route:... for the link. Then

• Show the additional help text when the new option is enabled.
• Make the new option disabled by default for new fields.
• Make the new option enabled for existing fields, to avoid breaking existing sites.

With or without the new option, it might work well to rewrite the help text using an unordered list for the different kinds of links.

I forgot to mention: once we officially support the option, we should support it better. That means do not strip the "route:" prefix when creating the default value for the form field.

In other words, +1 to the last paragraph of #12.

I haven't tried it out properly but this approach (in the MR) should deal with the bug. I haven't started on any of the extra parts outlined in #13, just giving it a kick-start.

Another issue with the current getUriAsDisplayableString approach is that route:entity.group_content.canonical;group_content=135&group=3 gets truncated to ntity.group_content.canonical;group_content=135&group=3. That is an outright bug.

Here we add some test coverage and rebase this as a patch on 10.1.x.

Why did we remove the Text-only links. test cases? Because there is dedicated test coverage, it wasn't needed and the regex parsing was causing errors on those test cases.

Random test failure.

Ran the test locally without the fix and it failed as expected

Behat\Mink\Exception\ExpectationException : The string ""route:entity.node.canonical;node=1"" was not found anywhere in the HTML response of the current page.

So removing the tag.

The change in question seems good though.

+++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
@@ -160,11 +162,6 @@ protected function doTestURLValidation() {
-      // Text-only links.
-      '<nolink>' => '&lt;nolink&gt;',
-      'route:<nolink>' => '&lt;nolink&gt;',
-      '<none>' => '&lt;none&gt;',

why are we removing these?

Can we also get a red/green set of patches here demonstrating the new test failures?

Thanks

I think we also want a change notice for this detailing the official support for route:

Removing getUriAsDisplayableString makes sense.

It fixes the situation for me too.

+1 for patch in #20.

Just been hit by the problem today.

Spent a bunch of time to understand what was going on and finally found this issue.

Hoping to have it fixed one day 🙏 😉

re #25:

See #21. More specifically, testNoLinkUri. Since doTestURLValidation has duplicate coverage that is difficult to handle with the way that regex works, I removed it as opposed to adding one-off logic for it.

Added a CR and uploading a test-only patch. The full patch is a duplicate of #20.

Please review.