LinkWidget does not populate Route name and parameters

It works testing manually and the 2 fails are fixed. Let's take a look at the results again...

#5 looks good but this test should fail. Locally this test works.

The test is correct... we're editing and the bug appears on "view". I was trying with an entity user and the bug appears too.

Added test + patch.

this might actually be critical.

--

+++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -165,8 +165,11 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   public function isEmpty() {
-    $value = $this->get('url')->getValue();
-    return $value === NULL || $value === '';
+    if ($this->isExternal()) {
+      $value = $this->get('url')->getValue();
+      return $value === NULL || $value === '';
+    }
+    return FALSE;

why are all internal links non-empty?

Why should $value['url'] be populated at all if you have found a route?

regarding #12, it passes without that if.

@pwolanin in the comment I see that we "Reset the URL value to contain only the path." I was checking and when adding for example "node/add" as a link, we receive "/node/add" so we remove the first "/".

The test reproduce the bug and the bug is fixed by the patch so looks good. :)

Note: This kinda does similiar things as #2235457: Use link field for shortcut entity is doing, but this is already. Just wanting to raise awareness.

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -212,6 +212,8 @@ public function massageFormValues(array $values, array $form, FormStateInterface
+          $value['route_name'] =  $url->getRouteName();
+          $value['route_parameters'] = $url->getRouteParameters();

Can we please get rid of the $url->toArray() call? It doesn't buy as that much and is rather confusing ... this used to work for us, but it changed its internal behaviour.

+++ b/core/modules/link/src/Tests/LinkFieldTest.php
@@ -158,6 +160,10 @@ protected function assertValidEntries($field_name, array $valid_entries) {
+      // Verify that we can load the entity for an internal URL.
+      if (!Url::fromRoute($value)->isExternal()) {
+        $this->drupalGet('entity_test/' . $id);
+      }

This is testing our test data. We should be loading the entity and seeing that the field's value is a value route with parameters for internal paths. Also, afaics, $value here is always a path not a route.

#17 @dawehner At least we need "options". removed toArray() method.

#18 @alexpott I think maybe it is enough checking if the link exists when we go to the node

Thanks for the revision.

In the latest -dev core build it is not possible to save nodes with internal links via the UI.
This patch fixes it.

#18 @alexpott I agree with @marthinal at #19. Any reason for testing the route?

I still question whether this should be removed or changed when there is a route:

           $value['url'] = substr($url->toString(), strlen(\Drupal::request()->getBasePath() . '/'));

In general you shouldn't be storing the rendered URL

#22 needs to be answered before we can commit this.

@pwolanin I have done tests locally and I think you are right. We don't need to store the URL when there is a route, then I have removed that line.

This issue seems to be closely related to another issue: #2300161: Link field URL textfield is populated with a fully generated URL in entity edit form if internal URL is used as a field value

1. First of all I agree with everyone who expressed an opinion that URL should not be used for internal paths. It's worth noting that url value will still be stored in field storage unless it's set to NULL.

2. It's also worth mentioning that none of the patches in this issue address the problem with internal path not being properly populated in the widget when entity is edited. As it's stated in #2300161: Link field URL textfield is populated with a fully generated URL in entity edit form if internal URL is used as a field value:

Link field URL textfield is populated with a fully generated URL in node (or any other entity) edit form if internal URL is used as a link field URL value.

In order to avoid that, URL value for the link field needs to be processed and:

1. output as-is if URL is external;
2. output as un-aliased internal path if URL points to internal path;
3. output as-is if URL points to a special internal path like <front>, <none> or <current>.

3. Current tests do not test for validity of special internal paths (e.g. <front>). I added <front> to the list of valid internal paths to the tests.

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -39,14 +40,42 @@ public static function defaultSettings() {
   +        // Check if link points to a special route name that conforms to the
   +        // pattern of "<[route]>" which is applied to commonly used routes
   +        // like <front>, <none> and <current>. In that case display the route
   +        // name in the widget as URL value.
   +        if (preg_match('/^<[^>]+>$/', $items[$delta]->route_name)) {
   +          $default_url_value = $items[$delta]->route_name;
   +        }
   

   It is sad that we have to support it ...

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -39,14 +40,42 @@ public static function defaultSettings() {
   +          $default_url_value = substr($url->toString(), strlen(\Drupal::request()->getBasePath() . '/'));
   +          // Get un-aliased URL.
   +          $default_url_value = \Drupal::service('path.alias_manager')->getPathByAlias($default_url_value);
   

   Afaik you can achieve the same thing when you set $options['alias'] = TRUE;

@dawehner, thanks for the tip. Implemented.

Do we want to re-resolve the route at render time or otherwise work harder to retain the user input?

i.e. maybe postpone for this discussion: #2346189: Denormalizing paths into route names/parameters is brittle / broken

Given that we talk here about a bug-fix only issue I would argue that this should be worked on.
It is a problem which exists out there already.

link module is not usable at all for internal links without this patch, so regardless of what's the outcome of discussion in #2346189: Denormalizing paths into route names/parameters is brittle / broken this bug needs to be fixed.

Yeah I have to agree, we want to add it know, and if just to have that tiny little bit more of test coverage we have to be aware of in the future.

The solution based on #2407505: [meta] Finalize the menu links (and other user-entered paths) system that we should process the internal path at render time, and not try to store a route.

At that given point in time, this is a duplicate of #2406749: Use a link field for custom menu link