Implement autocomplete UI for the link widget

Ok, #2417793: Allow entity: URIs to be entered in link fields is in, I think this is good to go again.

Also, fixing some metadata, since this is now the new critical issue for implementing the UX.

It's unclear to me whether #1959806: Provide a generic 'entity_autocomplete' Form API element is a hard or soft blocker for this one. Maybe you folks can discuss tomorrow?

We (yesct, dawehner) are working on providing a minimal implementation of that based upon #1959806: Provide a generic 'entity_autocomplete' Form API element

This part of the current issue summary:

If the input has a slash "/" in it, no longer attempt to autocomplete; instead store what the user typed.

is missing a essential part from my proposal: The input *has* to start with a slash "/" in order to be considered a path, because entity titles can contain slashes.

Well, no, because "https://drupal.org/" but fair enough.

@webchick, I was talking about paths as in internal paths, not external URLs :)

uploading WIP

Yayyyy!

Just a note for the peanut gallery, the size of the patch from #8 is so big because it also includes the patch from #1959806: Provide a generic 'entity_autocomplete' Form API element :)

Meh.

Reroll.

Here is a reviewable bit.

This could really fix a couple of the failures.

.

A bit of work on some tests in the meantime.

Here is original patch without #1959806: Provide a generic 'entity_autocomplete' Form API element for review.

git merge to the rescue.

Here is rebased original patch.

Doing a UI review atm, here are some screenshots. (Note that yes, the fieldset looks wonky; that's being cleaned up in #2416987: Fix UI regression in the menu link form). Overall this looks pretty thorough:

When you first get to the page it looks like this:

There's an autocomplete spinner but nothing in the help text to explain what the spinner is going to autocomplete. Don't think we can let this in without adjusting that, since we want it to ideally be the 90% interaction.

In terms of the "simplest thing that could possibly work," let's go with "Enter the title of a piece of content, an internal...." (i.e. keep the rest as-is; we can always further refine it in normal follow-up issues.)

As you start typing characters you get an autocomplete of node titles:

The CSS styling looks a bit weird here, since there's no border? Not a commit-blocker, but maybe something to check out to see if there's a typo or something causing styles not to be applied.

Once you select one you get a typical Entity Reference value:

The mock shows also exposing the path here but I think that this is fine. I was mostly spitballing anyway, and arguably exposing the path would encourage people to type paths instead, which we tend to want to discourage them from doing.

Prefixing a path with "/" also works as expected:

There's also validation if you try and type crap in here, with instructions on how to correct it:

The error message could use some tweaking, since most often when they get this it'll be in the service of trying to select content. Maybe something like:

"Content with the title '%title' was not found. If you meant to link to a path, note that paths must start with /, #, or ?." (note the judicious use of Oxford comma! ;))

Side-note: It's _very_ rare to link to a # or a ? compared to a /. I actually think we shouldn't bother exposing those in the error message. They're quite advanced concepts that only people fluent in HTML would know (remember this screen is often for content authors). I could see us setting up a handbook page somewhere that talks about all the crazy things you can type in here and linking to it in a follow-up.

--

Overall, this is looking really good! Excited how fast this came together.

Re: webchick in #34:

The CSS styling looks a bit weird here, since there's no border? Not a commit-blocker, but maybe something to check out to see if there's a typo or something causing styles not to be applied.

I noticed this earlier and posted a patch at #2417705: Autocomplete suggestions visual regression after modal and jQuery UI update

Just FTR there's a module doing this on D6/D7 but in the context of CKEditor only. Would potentially be good to look at it for inspiration and/or for a follow-up issue to add autocomplete for links added via CKEditor.

I'd like to mention that I'm using such a module in D7. We had lot of usability issues as it often lists duplicate articles titles and taxonomy terms with same title. Users have selected wrong links and have not understood why two/3/4 of same title have been listed.

How do you like to solve this UX?

That was the point of showing the path in the drop-down. However, doing that is going to be in a follow-up, if we decide to do it.

I like this feature a lot and it is a huge usability enhancement to Drupal. We only need to make sure that not only the title is shown and I hope we do not miss to get this in, too. Critical is the proper prio for sure :-)

We also need this in CKEditor... this is where my users using this auto complete a lot.

Straight reroll on top of #1959806-51: Provide a generic 'entity_autocomplete' Form API element (which is RTBC!).

Now fixing the test failures.

(The do-not-test part, which is the actual patch in this issue, is *identical* to that in #44.)

#48: fully agreed! #2292159: EditorLinkDialog should validate URLs, and autocomplete like the Link widget already exists for that.

Without a reroll against the recent entity_autocomplete patch, but with proper fixes.

The massive number of failures in #49 stems from the fact that #44 (and hence also #49) called EntityAutocomplete::extractEntityIdFormAutocompletionResult(), which doesn't actually exist, at least not in the latest 'entity_autocomplete' Form API element. EntityAutocomplete::extractEntityIdFromAutocompleteInput() is what we want.

Also fixed the 4 failures in #43/#44 (which had now actually become 5 failures, probably due to other patches having landed, most likely #2418613: Fix #0 bug in toUriString() method in Url class, clarify toString() vs toUriString(), which updated how we deal with (empty) fragments).

1. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
   @@ -24,6 +24,8 @@ class LinkTypeConstraint extends Constraint implements ConstraintValidatorInterf
   +  public $messageInvalidStart = 'Manually entered paths should start with /, # or ?.';
   
   @@ -77,6 +79,13 @@ public function validate($value, Constraint $constraint) {
   +      // Figure out the user entered value after user-path. This might contain a
   +      // fragment so we cannot use parse_url($string, PHP_URL_PATH), so we strip
   +      // out the 10 chars of user-path:.
   +      if (parse_url($link_item->uri, PHP_URL_SCHEME) === 'user-path' && !in_array(substr($link_item->uri, 10)[0], ['#', '/', '?', '<'], TRUE)) {
   +        $this->context->addViolation($this->messageInvalidStart);
   +      }
   

   Having this on the LinkTypeConstraint makes no sense. The leading slash is a pure widget-level decision. LinkItem does not store 'user-path:' URIs with a leading slash. In fact, user-path:/node/add does not even resolve to the expected URL, but user-path:node/add does.

   Hence this doesn't belong in the constraint, but at the widget. Once we have #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route, it'd be a different story.

2. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -577,4 +571,24 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
   +  protected function generateEntityReferenceLabel($value) {
   

   This should only return the entity reference label if the user has access to the entity.

3. +++ b/core/modules/menu_ui/src/Tests/MenuTest.php
   @@ -86,10 +86,11 @@ function testMenu() {
   +//    $this->doMenuTests();
   +//    $this->doTestMenuBlock();
   ...
   +//    $this->addCustomMenuCRUD();
   

   Debug leftover, but easy fix, of course.

4. +++ b/core/modules/shortcut/shortcut.module
   @@ -298,7 +298,7 @@ function shortcut_preprocess_page(&$variables) {
   -    $link = Url::fromRouteMatch(\Drupal::routeMatch())->getInternalPath();
   +    $link = Url::fromRoute('<current>')->toString();
   

   Nice clean-up :)

5. +++ b/core/modules/shortcut/src/Tests/ShortcutTestBase.php
   @@ -66,7 +66,7 @@ protected function setUp() {
   +          'uri' => 'user-path:/node/add',
   
   @@ -76,7 +76,7 @@ protected function setUp() {
   +          'uri' => 'user-path:/admin/content',
   
   +++ b/core/profiles/standard/standard.install
   @@ -58,7 +58,7 @@ function standard_install() {
   +    'link' => array('uri' => 'user-path:/node/add'),
   
   @@ -66,7 +66,7 @@ function standard_install() {
   +    'link' => array('uri' => 'user-path:/admin/content'),
   

   For the same reasons as my remark about LinkTypeConstraint, this is wrong.

   Since you pinged me while I was posting a patch, I reviewed your patch, and thought this was a more elegant solution than the one I had. But this is wrong. So rerolling my patch with my original solution.

+++ b/core/modules/shortcut/shortcut.module
@@ -298,7 +298,7 @@ function shortcut_preprocess_page(&$variables) {
-    $link = Url::fromRouteMatch(\Drupal::routeMatch())->getInternalPath();
+    $link = Url::fromRoute('<current>')->toString();
     $route_match = \Drupal::routeMatch();

Actually, is this even worth it? AFAICT Url::fromRoute('<current>') is more expensive. And since we need $route_match anyway (see the next line of code after the change), we don't actually gain anything but a minor, arguable code legibility improvement.
Perhaps this hunk snuck in from [#2418229]? It makes sense there, but is kind of out of scope here.

Furthermore, this causes the link to contain a leading slash, and for a shortcut to be created with that leading slash: user-path:/<whatever the path is> is what gets stored. This is the same problem as the above.

The only reason user-path: URIs with leading slashes are working for you is that when they are processed by Url::fromUri(), it eventually hits PathValidator::getUrl(), which then simulates a request, and does this to do that:

    $path = ltrim($path, '/');
    $request = Request::create('/' . $path);

I.e. the leading slash is trimmed away, and hence still resolves to the expected route. But it's definitely not how user-path URIs are designed to work today. Again, we have #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route for that.

Fixed my patch as per #54.5. And did some docs fixes. Hopefully testbot will pick up this patch, because it didn't pick up #52 and #53…

EDIT: for clarity, the interdiff is against my latest patch, in #53, not dawehner's in #54.

- $this->drupalPostForm('admin/config/user-interface/shortcut/link/' . $shortcut->id(), array('title[0][value]' => $new_link_name, 'link[0][uri]' => $shortcut->link->uri), t('Save'));
+ $this->drupalPostForm('admin/config/user-interface/shortcut/link/' . $shortcut->id(), array('title[0][value]' => $new_link_name, 'link[0][uri]' => str_replace('user-path:', '/', $shortcut->link->uri)), t('Save'));

Sorry but you are wrong. The test ensured that you can simply change the title of an existing shortcut. It should be possible for the user without changing the URL at the same time (see my standard.install change)

Time for an actual review!

Attached patch fixes all points raised, except the <front> and <none> hunks.

Next: addressing #58 and the other points that I raised.

1. +++ b/core/lib/Drupal/Core/Path/PathValidator.php
   @@ -109,6 +109,9 @@ protected function getUrl($path, $access_check) {
        if ($parsed_url['path'] == '<front>') {
          return new Url('<front>', [], $options);
        }
   +    elseif ($parsed_url['path'] == '<none>') {
   +      return new Url('<none>', [], $options);
   +    }
   
   +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -44,15 +45,39 @@ public static function defaultSettings() {
   +    $uri = str_replace('<none>', '', $uri);
   +    $uri = str_replace('<front>', '/', $uri);
   

   I think these were added temporarily, to get the tests to pass?

   AFAICT the goal was to not support <front>, but instead /, which is much more logical than <front>.

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -44,15 +45,39 @@ public static function defaultSettings() {
   +      $entity = \Drupal::entityManager()->getStorage($entity_type)->load($entity_id);
   

   These will trigger unhandled exceptions when using entity:non_existing_entity_type/1.

3. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -65,18 +90,33 @@ protected static function getUriAsDisplayableString($uri) {
   +        if (parse_url($string, PHP_URL_PATH) === NULL) {
   +          $string = '<none>' . $string;
   +        }
   +        if (parse_url($string, PHP_URL_PATH) === '/') {
   +          $string = '<front>' . ltrim($string, '/');
   +        }
   

   This is not quite right… it violates the semantics of user-path:

   1. The URI user-path: points to the front page.
   2. Hence the URI user-path:#foo points to #foo on the front page.
   3. Hence user-path: is not designed to be able to link to just fragments, i.e. relative to whatever the current page is, i.e. it doesn't support the <none> route.

   We have an issue to fix this: #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route. The changes made here introduce weird, inconsistent semantics. Let's do it right, by doing that other issue.

4. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -131,18 +180,20 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      // @todo This should be a setting?
   

   I thought that the conclusion from all prior discussion was:

   1. we want the user to be able to select any entity type
   2. but in this first pass, we just hardcode it to nodes

   Filed a follow-up for this: . If the above 2 points are wrong, then please update that follow-up.

5. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -131,18 +180,20 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
        if (!$this->supportsExternalLinks()) {
   -      $element['uri']['#field_prefix'] = \Drupal::url('<front>', array(), array('absolute' => TRUE));
   +      $element['uri']['#field_prefix'] = rtrim(\Drupal::url('<front>', array(), array('absolute' => TRUE)), '/');
        }
   

   With entity autocomplete, this makes far less sense. You'll end up with http://drupalsite.com […], where […] is the autocomplete. So far so good. If you type a path, you're fine. But once you've used the entity autocomplete, it looks like this: http://drupalsite.com [Some article (1)].

   That makes far less sense.

   Therefore I doubt the prefix is worth keeping.

6. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -98,9 +98,11 @@ function testURLValidation() {
          'entity:user/1',
   

   This is the only test coverage we have for testing the link widget's 'entity:' URI support. We should add additional test cases now that we have entity reference autocomplete support.

7. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -150,6 +155,12 @@ protected function assertValidEntries($field_name, array $valid_entries) {
   +      // Special case entity URLs.
   +      if (parse_url($value, PHP_URL_SCHEME) === 'entity') {
   +        list($entity_type, $entity_id) = explode('/', parse_url($value, PHP_URL_PATH));
   +        $entity = \Drupal::entityManager()->getStorage($entity_type)->load($entity_id);
   +        $value = $entity->label() . ' (' . $entity_id . ')';
   +      }
   

   This reimplements a lot of the widget logic in a test assertion function. That's not acceptable.

8. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -168,7 +179,20 @@ protected function assertInvalidEntries($field_name, array $invalid_entries) {
   +      // There are two types of invalid entries: Either the path does not exist/
   +      // the user does not have access to it, OR the slash at the beginning is
   +      // missing.
   +      // If a schema is specified there is nothing to see, its kinda valid.
   +      if (parse_url($invalid_value, PHP_URL_SCHEME)) {
   +        $this->assertText(t("The path '@link_path' is either invalid or you do not have access to it.", array('@link_path' => $invalid_value)));
   +      }
   +      elseif (($path = parse_url($invalid_value, PHP_URL_PATH)) && !in_array($path[0], ['?', '#', '/'])) {
   +        $this->assertText(t('Manually entered paths should start with /, # or ?'));
   +      }
   +      else {
   +        $this->assertText(t("The path '@link_path' is either invalid or you do not have access to it.", array('@link_path' => $invalid_value)));
   +      }
   

   This talks about 2 types, but does 3 checks, hence there are actually 3 types of invalid entries.

   Furthermore, the second if is very wrong, since entity label (entity ID) is now valid input. This is another example of the assertion function implementing too much logic.

The test ensured that you can simply change the title of an existing shortcut. It should be possible for the user without changing the URL at the same time

If that's what it was testing, then we should just omit uri[0][value] from the values being POSTed. Did that in this reroll.

My change was correct, because the widget expects data in a different form than it is stored. The user enters /node/add, which is stored as user-path:node/add. The patches before mine stripped user-path:, and hence were posting node/add. This is wrong.
The only reason the tests passed, is because ShortcutTestBase was changed to have wrong links saved as well (note that those are created without validation, which is why that was at all possible). Therefore, what was stored was user-path:/FOO, hence stripping just user-path: resulted in /FOO, which was indeed valid input for the widget. But note how this only worked because the data in the DB was wrong!
Once the data is stored correctly, i.e. user-path:FOO, stripping user-path: results in FOO, which is invalid input.

So we have only two valid solutions:

1. str_replace('user-path:', '/', $shortcut->link->uri): user-path:FOO -> /FOO, which is accepted by the widget
2. $shortcut->link->uri: user-path:FOO -> user-path:FOO, which is accepted by the widget

Well, those two are valid, or we can simply not post the link field's value, which means the test runner will reuse the current value from the form. Which is what you say the test intended to test.

(see my standard.install change)

Those changes were wrong for the same reasons as above. They stored user-path:/FOO, which is invalid.

… and here's the patch I forgot in #61.

Still working on addressing the <front> and <none> stuff in my #59 review.

In the mean time, this reroll makes sure that any input that begins with /, # or ? does not trigger the autocomplete!

In the mean time, this reroll makes sure that any input that begins with /, # or ? does not trigger the autocomplete!

Meh, we discussed that multiple times that this issue is about a minimal implementation.

This addresses the <front> and <none> hunks I reviewed in #54.

In #54.3, I said:

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -65,18 +90,33 @@ protected static function getUriAsDisplayableString($uri) {
+        if (parse_url($string, PHP_URL_PATH) === NULL) {
+          $string = '<none>' . $string;
+        }
+        if (parse_url($string, PHP_URL_PATH) === '/') {
+          $string = '<front>' . ltrim($string, '/');
+        }

This is not quite right… it violates the semantics of user-path:

1. The URI user-path: points to the front page.
2. Hence the URI user-path:#foo points to #foo on the front page.
3. Hence user-path: is not designed to be able to link to just fragments, i.e. relative to whatever the current page is, i.e. it doesn't support the <none> route.

We have an issue to fix this: #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route. The changes made here introduce weird, inconsistent semantics. Let's do it right, by doing that other issue.

As is to be expected, there is no way to make the validation rules introduced in this patch work without also doing #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route.

The validation rules introduced in this issue say that a user-path: URI must begin with either /, # or ?. The first implies the <front> route. The latter two both imply the <none> route (i.e. URIs that render to the URLs <a href="#foo>… and <a href="?foo=bar">… respectively). But since the current user-path: URI semantics cannot express the equivalent of the <none> route, we cannot actually make this validation rule work in the current patch!

Next: getting #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route to RTBC and committed, since that's effectively a blocker for this issue. Updated the IS and issue title to reflect the 2 blockers for this issue.

Some minor clean-up and docs updates. Plus a fix for the new test failure in #65.

Discussed with Alex, he didn't mean to remove the "[PP-2]". Restoring it.

#1959806: Provide a generic 'entity_autocomplete' Form API element is the first blocker, and is now RTBC! :)

#2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route is the second, now up for review! :)

Mere minutes later, #1959806: Provide a generic 'entity_autocomplete' Form API element got committed! :)

Rerolled #67's do-not-test patch against HEAD.

.

I'm not sure whether its a good idea to not allow the usage of <none> and <front> anymore.
They are for example still used in other places, like block visibility, so I could imagine that people expect it to work the same here.

I agree with #74, those special cases are quite useful and easy to remember for users. Would it be too much work to keep supporting them here?

#74 + #75: That would indeed mean a certain level of disconnect. It would not be much work.

But:

1. we're already making the leading slash required in the link widget, versus no leading slash in block visibility, path alias forms, site information form
2. by still allowing <front> (<none> has never actually been supported as a valid path… try it, you'll get a validation error), instead of getting a clean break with the past, a clearly different mental model, we end up with a mix of both — I think that's the worst of both worlds
3. the places where you can enter "old style path input" is strictly limited to site administrators/site builders, whereas the "new style path input" (i.e. the changes made here to LinkWidget) affect content authors.

I think the harm mentioned in #74 is therefore strictly limited to site administrators, which is better than to harm content authors' mental model, which the proposed solution in #74 implies, IMHO.

If we feel strongly about this: can't we just update those other forms in analogous ways; not changing what they store, but changing what they present to the user?

we're already making the leading slash required in the link widget, versus no leading slash in block visibility, path alias forms, site information form

Yes. For manually typed paths. Which will be a ~10% case, versus adding a link in your navigation menu to the home page, which is basically a 100% case.

by still allowing <front> [...], instead of getting a clean break with the past, a clearly different mental model, we end up with a mix of both — I think that's the worst of both worlds

It's not as though "the past" was done in a vacuum. It was done because non-technical site builders and especially content authors do not always know how URLs work. "/" is intuitive to you and me, but someone like my brother would never be able to figure that out. If it were documented, he would copy/paste it fine, but he would not understand why that meant "front page." Versus <front> is a plain-English word that's easy to understand and used in several UIs in core.

OK, #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route is in. Unpostponing this bad-boy.

Can I humbly request that since the idea to move from <front> to / or whatever is going to require some discussion and also be a fair bit of work (in order to do consistently across all core UIs, which is what we would want to do), we postpone it to another non-critical issue and simply make this critical one match the existing behaviour?

Versus <front> is a plain-English word that's easy to understand […]

Did you know that <front> cannot be translated? Therefore it could indeed be argued it's easier to understand for English Drupal sites. But not for any translated site.

From HEAD:

• English: The path for this menu link. This can be an internal Drupal path such as node/add or an external URL such as http://drupal.org. Enter to link to the front page.
• Dutch: Het pad voor deze menulink. Dit kan een intern pad zijn zoals node/add of een externe URL zoals http://drupal.org. Voer in voor een link naar de voorpagina.
• Hungarian: A menüponthoz tartozó útvonal. Ez lehet belső Drupal útvonal, mint a node/add vagy külső, mint a http://drupal.org. A címlapra hivatkozáshoz a használható.
• Simplified Chinese: 此菜单链接的路径。可以是 Drupal 的内部路径，如 node/add，或者外部 URL，如 http://drupal.org。输入 可以链接到首页。

Yes, there's also the node/add example, but that's a literal example, whereas <front> is a symbolic link, with a meaningful name. But there's only meaning in English. With a leading slash instead, we're putting all languages on equal footing.

But if we really feel that it's very important to keep <front>, I can rework the patch to add that translation layer.

Straight reroll, fixed a whole bunch of merge conflicts, was able to remove a bunch of overlapping hunks because things became simpler in this patch thanks to #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route.

Then had to add some fixes because of #2349071: EntityStorageException when trying to save a link over the maximum depth (which added a new test to update) and #2216271: Regression: Shortcut links access is not checked. The latter uncovered a hilarious bug: <none> is inaccessible by even UID 1, so a jumplink does not show up in the list of shortcuts… because <none> doesn't specify requirements. So the very route needing the least access checking of all, because it doesn't correspond to an actual controller … failed access checking :D

I'm not necessarily saying we should definitely *not* change it, I'm saying that whether to change it or not needs separate, dedicated discussion with pros/cons listed and weighed against one another. Your points are good ones for that disucssion, but they're going to get lost/missed in a critical issue that is supposed to be just about allowing the link field to autocomplete node titles.

Its just sad how this issue went from: let's build a minimal implementation, get this in and refine later. Things like fixing the auto completion, IMHO would have belonged into its own refinement issue ... But so is work in the issue queue. I agree that none just worked in menu_link_config

#81: Alright. This merely implements what AFAIK was discussed and agreed upon before, but it's fair to expand that a bit to maintain "usability backwards compatibility" of course. Opened #2421941: Determine whether not only "/" but also "<front>" should be valid input for the Link widget.

#82: I thought this was the minimal implementation; I understood that the lack of support for entity types other than nodes was the "minimal" part. Perhaps that was wrong, in which case: apologies. I merely worked to drive this to completion ASAP as per what has been discussed in NJ and the calls leading up to NJ.

Rerolling to add back support for <front> as valid input.

Done. Includes test coverage.

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -170,18 +198,22 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
-      $element['uri']['#description'] = $this->t('This can be an internal path such as %add-node or an external URL such as %url. Enter %front to link to the front page.', array('%front' => '<front>', '%add-node' => 'node/add', '%url' => 'http://example.com'));
+      $element['uri']['#description'] = $this->t('This can be an internal Drupal path such as %add-node or an external URL such as %drupal. Enter %front to link to the front page.', array('%front' => '/', '%add-node' => '/node/add', '%drupal' => 'http://drupal.org'));

If <front> is a valid link, i think this string can stay as is?

Note: user facing strings that use an example of an external url should no longer use drupal.org per #2418209: Replace user facing strings that use drupal.org as example of an external url.

I made sure <front> is allowed, but not that it is the recommended approach.

webchick: what do you think?

... I think I very explicitly asked at least twice to *not* change anything about that until we are done at the other issue. :)

The reason being is because unless this change is done everywhere we currently use <front>, it introduces a UI inconsistency which then need a a follow up to ensure it's "complete." The other issue will allow us to take a more holistic look at this and see if the pros of changing it warrant the benefits of the impact in beta.

I don't want this change, which needs discussion on its own, crammed in with a change we *have* to commit. So yes, please remove it.

"Not change anything" is crystal clear, thanks, will do. That also means I will change it so that if a user *did* enter /, that upon editing it, he will see <front>. Hence all old behavior is then preserved.

P.S.: I'm sorry, I thought you were only referring to input. But it of course totally makes sense to keep UI strings unchanged also, otherwise we still have inconsistencies wrt the rest of core. Apologies.

Done.

Cleaned up and significantly expanded the number of test cases for LinkFieldTest's valid internal link entries.

I kept the changes of <front> to / in test code, to keep the usage of <front> centralized in the LinkFieldTest only, which makes future removal simpler, and to use the non-legacy input in tests that aren't explicitly testing the link widget. (As I said, LinkFieldTest now *is* explicitly testing <front>.)

The latest patch looks awesome to me! Here's a quick review:

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -42,83 +44,99 @@ public static function defaultSettings() {
   +        if ($entity && $entity->access('view')) {
   +          $displayable_string = $entity->label() . ' (' . $entity_id . ')';
   +        }
   

   This should have a @todo that references #2419923: Port SA-CONTRIB-2013-096 to D8 because not showing any input at all is not correct. For example, ER in D7 contrib shows a "- Restricted access -" string.

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -42,83 +44,99 @@ public static function defaultSettings() {
   +   * This method is the inverse of ::getUriAsDisplayableString().
   ...
   +   * @see getUriAsDisplayableString()
   

   Shouldn't these have static:: in front? I'm not sure, just asking :)

3. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -170,18 +206,22 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      // @todo This should be a setting?
   

   Can be removed?

4. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -93,27 +93,59 @@ function testURLValidation() {
   +      $node->label() . ' (1)' => $node->label() . ' (1)',
   

   How about adding another case here:

   'entity:node/1' => $node->label() . ' (1)',

   This would test that if the user entered entity:node/1 manually, the code that transforms it to "entity label + id" works, right?

5. +++ b/core/modules/menu_ui/src/Tests/MenuLanguageTest.php
   @@ -65,7 +65,7 @@ function testMenuLanguage() {
   -    $link_path = '<front>';
   +    $link_path = '/';
   
   +++ b/core/modules/menu_ui/src/Tests/MenuTest.php
   @@ -266,7 +266,7 @@ function doMenuTests() {
   -    $this->drupalPostForm(NULL, array('link[0][uri]' => '<front>', 'title[0][value]' => $link_title), t('Save'));
   +    $this->drupalPostForm(NULL, array('link[0][uri]' => '/', 'title[0][value]' => $link_title), t('Save'));
   
   @@ -454,7 +454,7 @@ function doMenuTests() {
   -    $item8 = $this->addMenuLink('', '<front>', $menu_name);
   +    $item8 = $this->addMenuLink('', '/', $menu_name);
   
   @@ -494,20 +494,19 @@ function testMenuQueryAndFragment() {
   -    // Use <front>#fragment and ensure that saving it does not loose its
   -    // content.
   +    // Use <front>#fragment and ensure that saving it does not loose its content.
   
   @@ -564,7 +563,7 @@ function testUnpublishedNodeMenuItem() {
   -    $this->addMenuLink('', '<front>', $custom_menu->id());
   +    $this->addMenuLink('', '/', $custom_menu->id());
   
   @@ -606,7 +605,7 @@ public function testBlockContextualLinks() {
   -  function addMenuLink($parent = '', $path = '<front>', $menu_name = 'tools', $expanded = FALSE, $weight = '0') {
   +  function addMenuLink($parent = '', $path = '/', $menu_name = 'tools', $expanded = FALSE, $weight = '0') {
   
   @@ -666,7 +665,7 @@ function checkInvalidParentMenuLinks() {
   -        'link[0][uri]' => '<front>',
   +        'link[0][uri]' => '/',
   
   +++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
   @@ -41,18 +41,18 @@ public function testShortcutLinkAdd() {
   -      '<front>',
   ...
   +      '/',
   

   Bunch of unnecessary changes :)

Thanks for the review!

1. Done.
2. Not sure either, but other places do this, so: done!
3. Well, it needs to be replaced with a @todo to an issue that is basically a follow-up of #1959806: Provide a generic 'entity_autocomplete' Form API element, to add support for selecting the entity type. Could you create that follow-up? I'll link to that in the next reroll then.
4. Great catch, added.
5. I don't think they're unnecessary. From #89:
   

   I kept the changes of <front> to / in test code, to keep the usage of <front> centralized in the LinkFieldTest only, which makes future removal simpler, and to use the non-legacy input in tests that aren't explicitly testing the link widget. (As I said, LinkFieldTest now *is* explicitly testing <front>.)

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -42,83 +44,100 @@ public static function defaultSettings() {
+      // - '<front>' -> '/'
+      // - '<front>#foo' -> '/#foo'
+      if (strpos($string, '<front>') === 0) {
+        $string = '/' . substr($string, strlen('<front>'));
       }

Do we have to ensure that <none> also works here?

In general this patch looks really ready beside of that, IMHO.

#92: no, <none> was never even supported in the UI, so I don't see why we need to add support for that now? Just type #jumplink or ?query=string#and_jumplink — both of those are supported and tested.

#93: great :)

Alright, cool. Maybe it was just menu_link_config which had it.

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -170,18 +207,22 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
-      $element['uri']['#description'] = $this->t('This can be an internal path such as %add-node or an external URL such as %url. Enter %front to link to the front page.', array('%front' => '<front>', '%add-node' => 'node/add', '%url' => 'http://example.com'));
+      $element['uri']['#description'] = $this->t('This can be an internal Drupal path such as %add-node or an external URL such as %drupal. Enter %front to link to the front page.', array('%front' => '<front>', '%add-node' => '/node/add', '%drupal' => 'http://drupal.org'));

This description was updated in #2418209: Replace user facing strings that use drupal.org as example of an external url. to remove any mentions of 'Drupal'. I think the original description can stay as is?

While manually testing this I also noticed path aliases are not longer valid links unless they are prefixed with a slash, eg /about is valid but about is not. This behavior is not described in the issue summary and I did not really expect this change from an issue named 'Implement autocomplete UI for the link widget'. Is this the intended behavior? If so, will it break existing menu items and should it not require a change notice?

#97: oops, then I resolved that merge conflict incorrectly, awesome catch! :)

#98: all paths must be entered with a leading slash, as the IS already indicates. It'd be highly consistent to enter non-alias paths with a leading slash and path aliases without.
The leading slash *only* is a UI thing, this issue/patch doesn't change what is stored. (That was changed in #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route and other issues, see #2407505: [meta] Finalize the menu links (and other user-entered paths) system.)

Fixing #97 and the test failure of #91. I can confirm that reversing the #91 interdiff makes the tests pass again. Investigating.

• Actually, I can't reproduce #91 after all. I was able to, just once, but not anymore. Very strange. Let's see what testbot says.
• Fixed #97.
• Also addressed #90.3 / #91.3 (fixing/improving a @todo — I created the issue in @amateeescu's place).

Since I only changed comments, back to RTBC.

Copied the wrong URL 3 times: once here, twice in the patch. Gah.

@Wim Leers thanks for your clarification

+++ b/core/modules/menu_ui/src/Tests/MenuTest.php
@@ -494,20 +494,19 @@ function testMenuQueryAndFragment() {
-    // Use <front>#fragment and ensure that saving it does not loose its
-    // content.
+    // Use <front>#fragment and ensure that saving it does not loose its content.

Nit that could be fixed on commit? s/loose/lose

Test fails from #91, #100 and #101 are all testbot problems, the patch actually works fine!

Fixing #102 and hoping we'll get a better one this time :)

I finally understood what the @todo mentioned in #90.3 / #91.3 was about :/

I initially saw it as a "target_type should be a 'entity_autocomplete' selection setting" but what it actually means is "target_type should be a widget setting, not hardcoded to 'node'". @dawehner, can you confirm that?

This also means that #2423093: Allow multiple target entity types in the 'entity_autocomplete' Form API element is actually about adding the possibility to select the target type and selection settings in the link widget, not about supporting multiple target types in the 'entity_autocomplete' form element.

I initially saw it as a "target_type should be a 'entity_autocomplete' selection setting" but what it actually means is "target_type should be a widget setting, not hardcoded to 'node'". @dawehner, can you confirm that?

Yeah its about not limiting this particular widget by default. It would be pretty helpful for contrib IMHO.

The autocomplete blacklist does not appear to work. I create a node with the title "#twitter likes a #" and then link to this. The autocomplete works just fine. Also I'm not sure about the concept of the blacklist anyway since this is a totally valid node title.

The autocomplete blacklist works if you *start* typing with /, # or ? :)

Edit: and for node titles that start with one of those three characters, I think the idea was to support them enclosed in qoutes (i.e. start typing with "), just like strings that contain commas.

Exactly what #109 says. I tried the #108 scenario, and I can't reproduce it.

(Note that if your node was titled "#yarhar fiddle deedee", then typing "#", "#y", "#ya" or even "#yarhar", it doesn't autocomplete. But typing "#yarhar" does autocomplete, and then the text field will contain "#yarhar fiddle deedee (1)". That's a leading hash. That's fine. It'll store the entity URI. Because it's not just a fragment anymore. That's still a consistent UX.)

Tentatively moving back to RTBC, since AFAICT that was just a misunderstanding.

even "#yarhar", it doesn't autocomplete. But typing "#yarhar" does autocomplete

That's a contradiction. I assume the second one was meant to say "yarhar"?

the text field will contain "#yarhar fiddle deedee (1)". That's a leading hash. That's fine. It'll store the entity URI.

Yes. Because of the following:

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -42,83 +44,101 @@ public static function defaultSettings() {
+    // Detect entity autocomplete string, map to 'entity:' URI.
+    $entity_id = EntityAutocomplete::extractEntityIdFromAutocompleteInput($string);
+    if ($entity_id !== NULL) {
+      // @todo Support entity types other than 'node'. Will be fixed in
+      //    https://www.drupal.org/node/2423093.
+      $uri = 'entity:node/' . $entity_id;
+    }
+    // Detect a schemeless string, map to 'user-path:' URI.
+    elseif (!empty($string) && parse_url($string, PHP_URL_SCHEME) === NULL) {
+      // @todo '<front>' is valid input for BC reasons, may be removed by
+      //   https://www.drupal.org/node/2421941
+      // - '<front>' -> '/'
+      // - '<front>#foo' -> '/#foo'
+      if (strpos($string, '<front>') === 0) {
+        $string = '/' . substr($string, strlen('<front>'));
       }
+      $uri = 'user-path:' . $string;
     }

Note that we check if an entity ID can be extracted first, so that takes priority.

Also I'm not sure about the concept of the blacklist anyway since this is a totally valid node title.

I think it's okay to turn off autocompletion if the first character is "/", "?", or "#", since those are much more likely intended to be paths, query strings, and fragments, than they are to be the first character of a node title. And so long as you can still autocomplete the node title by just starting with some other character (e.g., "twitter" instead of "#twitter"), I think that edge case is sufficiently handled. But maybe that's a usability question that could benefit from broader testing, in which case I recommend committing it to get it in front of people, and then followups opened as needed.

I tested this with two nodes, one more or less the same title as Alex's ("#twitter likes the # symbol") as well as "/dev/null: my home away from home." and a "coming soon" node called "Fish" with a URL alias of "fish." This is to simulate the use case of a path that changes entity types.

First, I went to create a link to the home page. (Also found out that I can't delete the default "Home" link, nor even change its title, which is a super annoying regression from Drupal 7, but not caused by this patch.) Linking to either <front> or / worked, so we seem to be good there.

Next, I went to create a link to the twitter post. As promised, typing in # kills the autocomplete. However, typing "# shows the autocomplete, with that post first in the list. (Curiously, also "/dev/null: my home away from home" and "fish" which I don't understand.) Entering "twitter" (without the leading hash) also works. So I think this is fine, and expected.

Similarly, /de == no autocomplete; "/de == autocomplete, all options in the list. Basically, it seems like the " character causes a TILT in the autocomplete field and it just starts returning the entire result set in alphabetical order. I am guessing this is a problem with the autocomplete implementation itself though, rather than something caused by this patch specifically, but it'd be good to just make sure.

Tried typing the letter d, since that's only in the "/dev/null" post, and as expected it shows in the drop-down.

Created a menu link "Fish" node via entering the path /fish. Worked fine. Then I edited the admin/content view's path to be "/fish." My "Fish" menu item still goes to the node. I delete the node and now /fish is a 404. Hm. Also, the menu link is gone. Why did that happen?

Note: It's a very annoying inconsistency that in menu link I type /fish, but in Views UI I type foo for the exact same thing. We don't need to fix it in this patch, but we do need a follow-up to make all UIs in which you enter a path match with this one, IMO. Probably major.

1. Look into that weird behaviour with linking to a pathed node, the node getting deleted, and the menu link getting deleted as well. I thought the whole intent of these various changes was so that wouldn't happen.
2. As mentioned back in #34, the help text on the field still reads This can be an internal path such as /node/add or an external URL such as http://example.com. Enter <front> to link to the front page. But it needs to lead with the primary interaction here, which is entering (part of) the title of a piece of content.

   Suggested change:

   Start typing the title of a piece of content to select it. You can also enter an internal path such as /node/add, or an external URL such as http://example.com. Enter <front> to link to the front page.

   (This doesn't have to be perfect; we can easily adjust it later in a normal/minor follow-up issue, even after release.)

3. As idebr pointed out in #98, this still needs a change record, as it affects site builders.
4. Follow-up for autocomplete and the " character, unless that's caused by this patch.
5. Follow-up for path UI consistently entering leading "/" (Views, Path, Block, others?)

Looking very close!

Just to remind you, this issue is just about implementing the autocompletion, everything else around it, is out of scope of this issue.
Afaik this is the way to move things along, small steps, not big steps which solves all the problems.

Look into that weird behaviour with linking to a pathed node, the node getting deleted, and the menu link getting deleted as well. I thought the whole intent of these various changes was so that wouldn't happen.

Well, we have all kind of code in menu_ui.module which ensures that in case you remove a node, the corresponding menu link is removed as well.
Sadly we don't distinct between a manually setup menu link and a menu link created on the node form itself, for which the behaviour is the expected one.
If we would have managed to get #2315773: Create a menu link field type/widget/formatter in, this would have been a trivial issue though, because we would have been able to distinct between the two cases really easily.

Btw. its a little bit odd for example in the case of views, because in that case we don't support fragments / query strings anyway, because you don't enter
a link but rather a path, not a URI, really its a path. You can't create a view pointing to foo?bar=baz, but rather just to foo. Maybe it would help to
make that distinction clear, so that people would be able to understand what they exactly enter in links.

For #112.5: #2423913: Leading slash in link fields and views has different UX

Just to remind you, this issue is just about implementing the autocompletion, everything else around it, is out of scope of this issue.

AFAIK this issue isn't blocked on anything that's out of scope. There's the change record, but that's required prior to commit of all patches that make changes from D7, critical or otherwise (that's done now, so removing the tag). And there's the help text change, but that's because this patch makes changes to the way in which the field works, so changing the field description is required to pass the docs gate (once again required of all patches, critical or otherwise). I would've just made the help text change myself, except I wasn't sure if some of the other weirdness I found when testing was due to this patch or not. If it's not, great, let's get this in. (Though my response to the second half of #113 is we should only invoke the behaviour if the link is to an entity:node/X-style link, but again not for this issue.)

The spin-off in #115 looks good. It might end up "won't fix" but we should at least discuss which is the least bad UX impact we can have.

#112:

1. Since this issue only touches the LinkWidget, that couldn't possibly be caused here. As dawehner also already indicated.
2. Oh, I missed that! Fixed :)
3. I already replied to @idebr's #98 comment in #99. AFAIK no change record is actually necessary because this issue doesn't change APIs, it only changes the UI. We don't write change records for UI changes, only for API changes?
4. Follow-up filed: #2424083: Unexpected behavior in generic "entity_autocomplete" Form API element when using quotes
5. effulgentsia filed that in #115.

#115: you may have set a new record for the longest issue title :D :D

core/misc/autocomplete.js
  184:12  error  options is defined but never used  no-unused-vars

We have an eslint fail.

Addressing #118. Since it is really minor, I think it is good to go to RTBC immediately.

Awesome

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 203ead6 and pushed to 8.0.x. Thanks!

This area has flown back to "above my head" heights :-), a couple side-remarks / questions below :

1. +++ b/core/misc/autocomplete.js
   @@ -174,6 +179,11 @@
   +        var blacklist = $autocomplete.attr('data-autocomplete-first-character-blacklist');
   +        $.extend(autocomplete.options, {
   +          firstCharacterBlacklist: (blacklist) ? blacklist : ''
   +        });
   

   Looks like this will have unintended effects if there are several autocompletes on the page (i.e $autocomplete has more than one DOM element) ? Shouldn't we iterate on each element in $autocomplete individually ?

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -126,6 +146,15 @@ protected static function getUserEnteredStringAsUri($string) {
   +    // If getUserEnteredStringAsUri() mapped the entered value is mapped to a
   

   Parse error :-)

3. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -170,18 +208,23 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
        // If the field is configured to support internal links, it cannot use the
        // 'url' form element and we have to do the validation ourselves.
        if ($this->supportsInternalLinks()) {
   -      $element['uri']['#type'] = 'textfield';
   +      $element['uri']['#type'] = 'entity_autocomplete';
   

   I might get this wrong, but if the field supports internal links, the widget provides a UI that only lets you pick entities ? I can't link to internal paths that are not entities ?

   Also, if we "have to do the validation ourselves", shouldn't we set the 'entity_reference' element's #validate_reference to FALSE ?

1. Unless I made a mistake, this actually only applies to this particular autocomplete instance.
2. Oops :( Filed #2424927: Docs typo in LinkWidget to fix that, with a patch.
3. Yes, you can, just type something like /node/add just as the description says.

   And we only store valid URIs, but we don't validate whether an entity URI points to an actual entity, because that would make the Url class dependent on the entity system. So we actually *do* want to keep #validate_reference's default value of TRUE.

re 1.
I should have posted more context, but $autocomplete is :
var $autocomplete = $(context).find('input.form-autocomplete').once('autocomplete');,
so it contains all autocomplete elements in the context for the current Drupal.behaviors.attach() call.
Then the code builds one single autocomplete.options for all of them, and attaches it to all the autocompletes in $autocomplete. Thus, unless I'm missing something, the data-autocomplete-first-character-blacklist set in one autocomplete spills over to the all of the others ?

@YesCT Thanks :-)

The autocomplete not showing results by relevance. So I have a menu with link title "About" and there are other menu items also which contain the keyword "about". Number of links having about keyword is more than 10. So when I search for About I don't see the page "About" in the autocomplete list. I am attaching a screenshot for the same.

Proposed solutions:
Ideally we could have a link which can work as load more. So in autocomplete if there are more options which can be shown than we show a link to load more options.
Another possible solution is to add relevance with Link title starting with search keywords appearing first and than the links which have that keyword in between the title somewhere.

Same situation as #129 in a large project. The autocomplete is limited, but doesn't include the result the editors are looking for and the results are not ordered by relevance. So this is an issue on large projects.

I'm looking for a follow-up issue. If I shouldn't find one and link it here, me or the next who runs into this, should create a follow-up issue for things like:

• order results by relevancy
• making the autocomplete limit configurable
• being able to exclude certain content types, that are irrelevant (#2427803: Expose target type, selection handler and selection settings in LinkWidget ?)
• ...?