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.

I created a new MR based on 11.x but unit tests are failing.

LinkFieldTest failed on the "Text-only links" section of doTestURLValidation(). The code to remove "route:" from the links before displaying them in the widget was removed. This doesn't only apply to routes like views. It also applies to the special routes. Lines 160-164 of LinkFieldTest would need to be updated like this:

      // Text-only links.
      '<nolink>' => 'route:&lt;nolink&gt;',
      'route:<nolink>' => 'route:&lt;nolink&gt;',
      '<none>' => 'route:&lt;none&gt;',
      '<button>' => 'route:&lt;button&gt;',

To be honest, I don't like this very much. I don't think "route:" should appear before these links. It may be confusing to users. In my opinion we should continue to strip "route:" from them.

I added a condition for <nolink>, <none> and <button> because these routes have a special handling in LinkWidget::getUserEnteredStringAsUri().

@prudloff thank you for working on this. I left a comment on the MR due to some code duplication.

I added my own suggestion that was mentioned in #40.

Tested following the steps in the IS

On a standard install I went and added a link.
Title: Test
URL: route:view.frontpage.page_1
It saved fine = EXPECTED
When I edited I just see view.frontpage.page_1 = BUG
Changed the title to "Test2"
I see the error = BUG
Applied the MR
I was happy to see I didn't have to change anything or save it just fixed itself
I see route:view.frontpage.page_1 and can save.

https://git.drupalcode.org/issue/drupal-3115188/-/jobs/7801499 shows the test coverage

Summary appears clear to me.

LGTM

Committed/pushed to main and 11.x, thanks!

Apologies to @dcam who I somehow left off the commit message, but did credit on the contribution record.