Display meaningful error messages according to the link type

I was able to reproduce the bug in Safari and the patch fixes it as described.

Note Opera and Chrome won't let you get this far… you get a browser-generated "Please enter a URL" popup if you don't use an http(s):// prefix, even if begins "www." (can be disabled with formnovalidate attribute)

I'm wondering if we could be making it easier for users - e.g. a default http:// placeholder (though that could cause confusion with https://) or maybe simply a better error message than:

The path 'www.example.com' is invalid.

Something like:

URLs should begin http:// or https://

(Any Help text the site builder chooses to add is displayed separately to the "This must be an external URL such as..." message.)

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -133,13 +134,16 @@ public static function validateUriElement($element, FormStateInterface $form_sta
+    if ($element['#link_type'] !== LinkItem::LINK_EXTERNAL && parse_url($uri, PHP_URL_SCHEME) === 'internal') {

Here you disabled this validation for external links, so it will display a different error message.

+++ b/core/modules/link/src/Tests/LinkFieldTest.php
@@ -163,6 +163,8 @@ public function testURLValidation() {
+    // Missing protocol schema.

The test fails because it is using the wrong error message, as you had disabled the external links form validation. It will work if you use $validation_error_2, the string from constraints validation. Moreover, this test is already covered by the invalid URL 'no-leading-slash' => $validation_error_2,.

AFAIK, Drupal 8 only supports HTML5 browsers. @wturrell probably you are using an older version of Safari, as version 9.0 supports url field validation: https://html5test.com/compare/browser/safari-9.0.html#row-form.url

I understand that we need need to display meaningful messages to users, so I'm changing the issue as to display messages according to each field type.

I'm adding a patch for tests with the same strings as the current version, so it should pass the tests.

There is the same tests patch with the corresponding strings as in the new code, this should pass only in the tests together with the new code.

This patch displays error messages based on the link type, as the original issue was about misleading users when the messages do not correspond to the same type.

@mac_weber - actually Safari 10.0.3 (El Capitan), but according to caniuse.com there's only partial form validation support due to a webkit bug.

I'm guessing browser form validation will always be disabled in the drivers Simpletest and Mink use? Can't find much written about it. It obviously makes manual testing a little slower (editing DOM each time to add novalidate attribute) - apparently there used to be a setting in chrome://flags/, but removed (presumably as no longer an experiment).

Anyway where I'm going with this is we could open a separate issue about improving the wording (#11/#12) but it may be a waste of time if it's a message increasingly few people will ever see, given it'll usually be intercepted by the browser. (i.e. I've no desire to hold this issue up.)

I opened a follow-up issue for the HTML5 message: #2861316: Display meaningful HTML5 validation error message

@wturrell you can get rid of the input type url in the file LinkWidget.php in the function formElement() by changing '#type' => 'url', to '#type' => 'textfield',

@gambry The constraint validation message is $message = "The path '@uri' is invalid.", which could lead to another similar issue being opened to get a more meaningful message to end users. I think we better fix and improve all 3 link type messages in this issue.

I see the test failed on testURLValidation while it is passing on my local machine. I will mark it to re-test.

Fixed the patch to avoid the "uninitialized string offset" error.

I had forgotten to rebase.

Added one more string from the Menu Link module.

I'll work on this

I have fixed the seven points explained in the comment #28.

Thank you for the review and the patch fixing CS.

When submitting /test/path on a LINK_EXTERNAL I get 'The path '/test/path' is invalid.', so only LINK_EXTERNAL internal values matching !in_array($element['#value'][0], ['/', '?', '#'], TRUE) will get the new meaningful error message.

It will trigger the new message for external links in case of missing protocol, such as example.com. I thought about a different logic to also trigger this message in case of trying to use an internal link, but it would require other changes in the test file, which I think we better spend time after fully converting it to BrowserTestBase. In fact it just got converted between comments #24-#25, but it still needs to get rid of deprecated methods there and I think there is one part that needs a rewrite.

Then, I think we are good to commit this issue.

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -133,13 +133,34 @@ public static function validateUriElement($element, FormStateInterface $form_sta
   -    // If getUserEnteredStringAsUri() mapped the entered value to a 'internal:'
   -    // URI , ensure the raw value begins with '/', '?' or '#'.
   -    // @todo '<front>' is valid input for BC reasons, may be removed by
   -    //   https://www.drupal.org/node/2421941
   -    if (parse_url($uri, PHP_URL_SCHEME) === 'internal' && !in_array($element['#value'][0], ['/', '?', '#'], TRUE) && substr($element['#value'], 0, 7) !== '<front>') {
   ...
   +    // Check if it is an internal link or an external link missing protocol.
   +    if (parse_url($uri, PHP_URL_SCHEME) === 'internal') {
   +
   +      // Ensure internal links raw value begins with with '/', '?' or '#'.
   +      if (!in_array($element['#value'][0], ['/', '?', '#'], TRUE)) {
   +
   +        // @todo '<front>' is valid input for BC reasons, may be removed by
   +        //   https://www.drupal.org/node/2421941
   +        if (substr($element['#value'], 0, 7) !== '<front>') {
   

   Changing this here makes a reviewer have to consider whether this change is part of the bugfix. As far as I can tell it's not. Also whether or not this makes the code more understandable is highly debatable. I don't think this change belongs here.

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -133,13 +133,34 @@ public static function validateUriElement($element, FormStateInterface $form_sta
   +              $error_message = 'External links must be a full URL and include its protocol, such as %link_example.';
   ...
   +              $error_message = 'Use a title of a piece of content to select it, or enter an internal path starting with /, ? or #.';
   ...
   +              $error_message = 'Use a title of a piece of content to select it, or enter an internal path starting with /, ? or #. External links must be a full URL and include its protocol, such as %link_example.';
   ...
   +          $form_state->setError($element, t($error_message, ['%link_example' => 'http://example.com']));
   

   This is not how t() works. We can't make the change like this because the translation extractor won't find the new strings. We need to put the t() inside the switch.

@alexpott 1. According to coding standards line length this should be split, not?

2. Yet now we should remove all t() from tests - not in this issue - there is a similar code in the tests already in core:

+++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
@@ -216,7 +255,7 @@ protected function assertInvalidEntries($field_name, array $invalid_entries) {
-      $this->assertText(t($error_message, ['@link_path' => $invalid_value]));
+      $this->assertText(t($error_message, ['@link_path' => $invalid_value, '@link_example' => 'http://example.com']));

Putting this back to needs review. #34 provided some reasoning as a reply to the comments in #33. I would like to know what still needs doing for this issue to move it forward.

#34.1 While there might be a case for this, coding standards/documentation cleanup shouldn't be in the same issue as a bug fix. That just makes all of our lives harder. In this case - that means that this change needs to be reverted to the docs
#34.2 Yes there is already code like this in a test. But as far as I can see, the code that @alexpott quoted in #33 is not in a test.

So this doesn't have to change in the tests, there it's ok, it does have to change in the other, non-test code.

Back to needs work, let's make the docs the same again and let's see if we can keep the if-statement the same as well and let's change the t from #33.2 as well.

d10 version of #30

I converted the patch in #50 to an MR, undid some of the code style changes, addressed the feedback in #33, and updated the IS.

Hiding patches.

Can we add screenshots of the new messages compared to the old please.

Added screenshots

Added a branch and MR based on Drupal core 11.2.x

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Appears to be 2 MRs now not sure what's up for review.

@smustgrave
I added a second Merge Request MR13480 based on 11.2.x . The other MR is based on 11.x. Sorry if this was incorrect. I created the branch as I needed a patch for my 11.2.x based site. Please let me know the proper workflow. Thanks.

@philltran most people generate a patch file for the version and upload it to the issue rather than opening a new MR. There's nothing wrong with creating a new branch for that purpose, but as you've seen creating an MR from that branch results in confusion.

I'm going to hide your 11.2 branch in the issue UI so that it's clear which MR needs to be reviewed. This won't make your branch go away permanently. There's a link to show hidden branches below the branch list. So you can maintain it and keep it up to date if necessary.

Yes no worries @philltran, so reviewing the changes and not 100% sure 'content title' is correct. What if I'm linking to a media or something else is that still "content"

...not 100% sure 'content title' is correct. What if I'm linking to a media or something else is that still "content"

I don't mind changing it. I don't love how it's worded anyway. But how do we say tell people to enter the title of a node when we aren't supposed to say "node" in the UI?

Yes, I normally do post patch files. All the patches were just hidden.... so I did not want to be that guy that added a new one. LOL.

How about instead of 'content title' , we say 'title of referenced item'. Is that to many words?

Or what about 'entity title' vs content

I think 'entity title' works well.

Or what about 'entity title' vs content

Except you can only reference nodes.

All the patches were just hidden...

@philltran When we create MRs on issues that have been around since the old patch days we hide the old patch files. I assume this is done mostly for the sake of de-cluttering the issue summary. And there are probably other benefits like making sure people are focusing on the right thing when working or reviewing. But if you have a new patch that was made for the sake of helping people who need a stable fix for something, then that's relevant content and you shouldn't worry about doing it.

@dcam Thanks for the words of wisdom.

Then maybe content works? Not sold on it but can’t think of anything new

I can't think of anything better and this is a net improvement.

One question about the wording on the MR.

I made the suggested change.

Update LGTM

Committed/pushed to 11.x, thanks!