Link field validation constraints don't give enough detail

I'll work on a patch to improve these messages. :)

This patch adds new messages for each exception case. I tried adding some tests too to check for these messages, but I wasn't able to do it (I still lack some knowledge).

Also, it seems that I have found another issue on the LinkNotExistingInternalConstraintValidatorTest.php.

When I check the non-existent url against the MissingMandatoryParametersException (or even InvalidParameterException) the test still passes with no fails. Apparently the current tests don't check correctly for the Exception type.

Sorry, I sent the wrong patch. Here is the right one.

Edit: It will fail because of tests/src/Functional/LinkFieldTest.php. It needs to be updated to handle the new added messages.

Edit 2: Here is a partial example of an update for LinkFieldTest::testURLValidation. It is far from ideal though. It doesn't check for paths without required parameters, and also don't verify about the internal/external link requirement (see LinkTypeConstraintValidator).

// Define some invalid URLs.
$validation_error_1 = "The path '@link_path' is invalid.";
$validation_error_2 = 'Manually entered paths should start with one of the following characters: / ? #';
$validation_error_3 = "The path '@link_path' is inaccessible.";
$validation_error_4 = "The path '@link_path' doesn't exist.";
$validation_error_5 = "The path '@link_path' has an invalid parameter.";
$validation_error_6 = "The path '@link_path' is missing a required parameter."; // Not being checked
$invalid_external_entries = [
  // Invalid protocol
  'invalid://not-a-valid-protocol' => $validation_error_1,
  // Missing host name
  'http://' => $validation_error_1,
];
$invalid_internal_entries = [
  'no-leading-slash' => $validation_error_2,
  'entity:non_existing_entity_type/yar' => $validation_error_4,
  // URI for an entity that doesn't exist, with an invalid ID.
  'entity:user/invalid-parameter' => $validation_error_5,
];

Feels like a task more than a bug

Thanks @joachin for the orientation!
Here is a new patch according to comment #8. I'm not sure how to update the tests though, so still needs work.

Thanks @joachin for pointing these out! I'm gonna work on it.

Regarding the tests, I'm able to make it pass, but I don't know how to cover some cases like: missing parameter (see #5), path should be external, and path should be internal.
As far as I could see these are not covered by the actual tests.

Here is the new patch with partially done tests. Even if it pass the bot I still believe it need more test coverage because some of the new error messages introduced here are not tested (see #11).

#10.1 Done
#10.2 Done
#10.3 Fixed

Sorry, forgot to attach the patch.

hi,
i'll work on it :)

Hi!

I applied the patches and re-rolled to 9.5.x.

Everything seems to be correct and apparently the only thing that is missing are the tests as mentioned in comment #11. I'm not really sure how I should implement those tests, so if someone can give me some references and guidence, I can try to work on it :)

Hi, i'll be trying to do those changes on the tests

I tried to make the tests but i couldn't do the "The path '@link_path' is missing a required parameter.", but the tests on internal and external uri was implemented.

I'll be working on this one.

After spending some time and asking for help trying to find a possible way to test it, I'm changing to RTBC. I'm doing this because it is not possible to manually add a path with an empty mandatory parameter. I tried different combination but could not test it manually.

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraintValidator.php
@@ -35,15 +35,15 @@ class LinkTypeConstraintValidator extends ConstraintValidator {
       // restriction.
       if ($uri_is_valid && $link_type !== LinkItemInterface::LINK_GENERIC) {
         if (!($link_type & LinkItemInterface::LINK_EXTERNAL) && $url->isExternal()) {
-          $uri_is_valid = FALSE;
+          $this->context->addViolation($constraint->onlyInternalMessage, ['@uri' => $link_item->uri]);
         }
         if (!($link_type & LinkItemInterface::LINK_INTERNAL) && !$url->isExternal()) {
-          $uri_is_valid = FALSE;
+          $this->context->addViolation($constraint->onlyExternalMessage, ['@uri' => $link_item->uri]);
         }
       }
 
       if (!$uri_is_valid) {
-        $this->context->addViolation($constraint->message, ['@uri' => $link_item->uri]);
+        $this->context->addViolation($constraint->invalidMessage, ['@uri' => $link_item->uri]);
       }
     }
   }

I think inspired by the refactoring above to remove $allowed we should do the same here to $uri_is_valid. And we can remove other unnecessary logic here too - any performance improvement by checking LINK_GENERIC is likely to be immeasureable.

  /**
   * {@inheritdoc}
   */
  public function validate($value, Constraint $constraint) {
    if (isset($value)) {
      /** @var \Drupal\link\LinkItemInterface $link_item */
      $link_item = $value;

      // Try to resolve the given URI to a URL. It may fail if it's schemeless.
      try {
        $url = $link_item->getUrl();
        // If the link field doesn't support both internal and external links,
        // check whether the URL (a resolved URI) is in fact violating either
        // restriction.
        $link_type = $link_item->getFieldDefinition()->getSetting('link_type');
        if ($url->isExternal() && !($link_type & LinkItemInterface::LINK_EXTERNAL)) {
          $this->context->addViolation($constraint->onlyInternalMessage, ['@uri' => $link_item->uri]);
        }
        elseif (!$url->isExternal() && !($link_type & LinkItemInterface::LINK_INTERNAL)) {
          $this->context->addViolation($constraint->onlyExternalMessage, ['@uri' => $link_item->uri]);
        }
      }
      catch (\InvalidArgumentException $e) {
        $this->context->addViolation($constraint->invalidMessage, ['@uri' => $link_item->uri]);
      }
    }
  }

Fixing the feedback mentioned in #21
All the functional tests are still passing on local.

Running tests against 10.1

Confirm the feed back from #21 has been addressed
Passed the 10.1 tests too.

Code looks good to me so +1 for RTBC

Sorry folks, there is commented out code.

+++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
@@ -169,6 +169,13 @@ public function testURLValidation() {
+    // The line below is not being checked.
+    /* $validation_error_6 = "The path '@link_path' is missing a required parameter."; */

Adding commented out code is not a good idea. What is the purpose? This almost reads like a reminder to add a test. Is it? By the way, there is an issue to find and resolve all instances #2909362: [meta] Commented-out code in Drupal.

The line of code is additional and not in use. So I've removed it. please review the patch.

Tagging for usability for the new sayings.

If someone could please update the issue summary with what was being updated

Give more detail in the validation messages.

This is a little vague.

Changes look good.

+1 from me after the usability review

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraintValidator.php
@@ -16,34 +16,26 @@ class LinkTypeConstraintValidator extends ConstraintValidator {
     if (isset($value)) {
...
 
       /** @var \Drupal\link\LinkItemInterface $link_item */

This is just suggestion, but since we're already refactoring this code, I thought we could change this isset statement to be $value instanceof LinkItemInterface, this way we're already validating if it's isset or not and we could remove both lines 21 and 22 because using instanceof, the LSP will be able to understand the variable type. Something like:

if (!$value instanceof LinkItemInterface) {
  return;
}

// Try to resolve the given URI to a URL. It may fail if it's schemeless.
try {
  $url = $value->getUrl();
  // If the link field doesn't support both internal and external links,
  // check whether the URL (a resolved URI) is in fact violating either
  // restriction.
  $link_type = $value->getFieldDefinition()->getSetting('link_type');
  if ($url->isExternal() && !($link_type & LinkItemInterface::LINK_EXTERNAL)) {
    $this->context->addViolation($constraint->onlyInternalMessage, ['@uri' => $value->uri]);
  }
  elseif (!$url->isExternal() && !($link_type & LinkItemInterface::LINK_INTERNAL)) {
    $this->context->addViolation($constraint->onlyExternalMessage, ['@uri' => $value->uri]);
  }
}
catch (\InvalidArgumentException $e) {
  $this->context->addViolation($constraint->invalidMessage, ['@uri' => $value->uri]);
}

This suggestion also uses an early return, just to improve the code complexity. What do you think?

Besides that, the code looks good and since my suggestion is just a nitpick, I'll leave this issue as NR.

1. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraint.php
   @@ -14,6 +14,25 @@
   +  public $notFoundMessage = "The path '@uri' doesn't exist.";
   ...
   +  public $invalidParameterMessage = "The path '@uri' has an invalid parameter.";
   ...
   +  public $missingParameterMessage = "The path '@uri' is missing a required parameter.";
   

   We can add string property type hints here now.

2. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
   @@ -14,6 +14,25 @@
   +  public $invalidMessage = "The path '@uri' is invalid.";
   ...
   +  public $onlyInternalMessage = "The path '@uri' is external, but the link field only supports internal paths.";
   ...
   +  public $onlyExternalMessage = "The path '@uri' is internal, but the link field only supports external paths.";
   

   Here too

#32 seems reasonable to me

This looks very close to me

Hey @larowlan, here's a patch with your suggestions and #32, now I think it's good.

PS: The interdiff has a change on the LinkFieldTest file, this is a reroll since this file had changed recently on #3300957: Potentially speed up LinkFieldTest

Hey @joachim, I think I was too much in a hurry yesterday with my suggestion(sorry :( ), when I saw:

public function validate($value, Constraint $constraint) {
  if (isset($value)) {
    /** @var \Drupal\link\LinkItemInterface $link_item */
    $link_item = $value;

I just thought about making this piece of code better, I thought "ok, the $value will always be mixed and we're creating another variable just to type hint the interface, so why not validate the $value instance, this way we could remove the unnecessary $link_item variable and also type hint the $value itself", so I haven't thought about some stuff like "Under what circumstances is the $value NOT a LinkItemInterface?", just thought it would be a good idea to validate the instance type because IMHO is a good practice to validate the instance before using it(when we cannot change the method signature).

The new validation messages at least make sense to me. I agree that it's a good idea to pay extra attention to the error messages because they are critical to user experience.

Thanks @lauriii!

Retesting #34

Having a link field just for internal and using link "https://www.google.com/ generates The path 'https://www.google.com/' is external, but the link field only supports internal paths."

Having a link field just for external and using /node/1 I actually don't get an error but an HTML popup "Please enter URL" which isn't super clear

Inputting /node/2333 (node that doesn't exist) also does not trigger an error.

Can these be confirmed this is a desired behavior?

makes me think, 'Under what circumstances is the $value NOT a LinkItemInterface? When might be be something else? What would it mean in that case?'

The code seems to some extent expect that it is not a LinkItemInterface because

if (isset($value)) {

will return true even if LinkItem->isEmpty() returns TRUE. I think there's definitely room for improvement there, which is of course not necessarily part of this issue, and

+    if (!$value instanceof LinkItemInterface) {
+      return;
+    }

could break certain scenarios, so this isn't right I think. I filed a separate issue addressing this: #3387013: Check LinkItemInterface::isEmpty() before validating

Usability review

We discussed this issue at #3449637: Drupal Usability Meeting 2024-05-31. That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @rkoller, @simohell, @shaal, and @SKAUGHT.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

We agree that having more specific error messages is an improvement, and I am adding the Usability tag. The suggested messages are clear. We think it might help to come up with a shorter version of these two:

• "The path '@uri' is external, but the link field only supports internal paths."
• "The path '@uri' is internal, but the link field only supports external paths."

We do not have a firm recommendation, but it might help to break each one into two, short sentences. For example, the second might be "The path '@uri' is internal. This field only supports external paths." Can we use the field label instead of "This field"?

I am also adding the tag for an issue summary update:

• Under "Proposed resolution", please add each of the new messages, along with steps to generate each one. For example, I am not sure how to trigger the message, "The path '@link_path' has an invalid parameter."
• Under "User interface changes", add at least one pair of before/after screenshots. We need screenshots for all of the new messages

.

I also created a merge request based on the patch in #34. I edited some context lines in the patch so that it applies to the current 11.x branch.

Postponing on #3387013: Check LinkItemInterface::isEmpty() before validating which makes the same change to check field values are LinkItemInterfaces.

This is unblocked.

Usability Review

The usability team had another look at this issue at the #3566641: Drupal Usability Meeting 2026-01-16. The participants were @benjifisher, @pallavi singh3013, @rkoller, and @simohell. I am giving them contribution credit on this issue.

In the previous usability review (Comment #43), I asked for screenshots of each possible error message. I have changed my mind. One set of before/after screenshots would be nice, and the new messages should be listed in the Proposed resolution section, but we do not need more screenshots than that.

In Comment #43, I added the tag for an issue summary update. This issue still needs that update.

In this round of review, we added a Link field with the default settings. In particular, it allows both internal and external links. We noticed two problems.

Error messages should not assume links are internal.

When we entered "www.drupal.org" for the URL, using the feature branch for this issue, we got the error message,

Manually entered paths should start with one of the following characters: / ? #

It is possible that the site has a page /www.drupal.org, so adding the leading / is the correct fix. But it is also possible that the user intended to give an external URL and forgot the protocol https://. (It is also possible that the user intended a query parameter or fragment, so a leading ? or # is the correct fix.) The error message in the 11.x branch is more complete:

Enter a content title to select it, or enter an internal path starting with /, ? or #. External links must be a full URL including the protocol, such as https://example.com.

Ideally, the message would depend on the configuration of the field:

1. If only internal links are allowed, then give something like the first part of the 11.x error message.
2. If only external links are allowed, then give something like the second part of the 11.x error message.
3. If both internal and external links are allowed, then give something like the full 11.x error message.

The term "path" should not be used to refer to external links.

With the default settings, the Link field has two parts (subfields), labeled "URL" and "Link text". If the error message refers to "path", then it is not clear that the user should change the URL value. Also, if the user wants to enter an external link, then "path" is not the correct term to use.

We do not have a firm recommendation, but some possibilities are to refer to "internal link" and "external link" or to "path" and "URL", depending on the type of link. It might also make sense to change the label of the first part to "path", "URL", or "URL or path", depending on how the field is configured. (That might not be in scope for this issue.)

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because I think that we can use technical writing and reading comprehension skills to update the issue summary from #48 usability team review, and then remove the Needs issue summary update tag. Then if we feel comfortable after understanding the issue, go ahead and rebase or make the changes based on that review.

The Drupal Contribution Mentoring team is triaging issues for DrupalCon Chicago 2026, and we are reserving this issue for Mentored Contribution during the event. After Thursday 26th March 2026 + 1 DAY (13:00 UTC), this issue returns to being open to all. Thanks!

RE: #48

Error messages should not assume links are internal.

This was the focus of #2828556: Display meaningful error messages according to the link type, which was committed about a week before comment #48 was made. It was responsible for updating the error messages mentioned as being on the 11.x branch. Those error messages already change based on the internal/external settings.

I updated the MR to change the constraint classes to refer to the URL subfield instead of "path" and when referring to external URLs.

A pre-MR screenshot was added to the issue summary. I added a table that shows sample input to trigger the validation messages, along with the updated messages.

That last test run crashed.

Tested with all the same inputs provided and do get the new messages (not going to spam the screenshots) but will say this also seems to address a random php warning Message Warning: Undefined array key 1 in Drupal\link\Plugin\Field\FieldWidget\LinkWidget::getUriAsDisplayableString() (line 75 of /var/www/html/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php) I got when trying entity:non_existing_entity_type/yar

I did start a new pipleline and it's all green.

Think this is good to go.

This will need a separate MR for 11.x to do a deprecation of a constraint option/parameter. (It likely would've needed a separate 11.x MR anyway.)