Link field validation constraints don't give enough detail

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.

Expanded the IS.

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 to go to RTBC.

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

> 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

I don't actually think this is a good idea.

Reading this code:

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

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?'

It makes the code confusing and adds to cognitive load.

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: 🐛 Check of $value not covering LinkItem cases in link contraint validators Needs review

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-05-31 Needs work . 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.

> Can we use the field label instead of "This field"?

Done, and rebased.

> 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."

I understand the desire to have shorter, simpler sentences but removing the 'but' between these two clauses removes meaning. Without it, it's less clear what the connection is between these two statements. It's like saying 'My cat eats cat food. This field only supports external paths' -- the reaction is -- 'so??'

Still needs IS update and screenshots. Tagging as Novice for those two tasks.