Use a link field for custom menu link

On our branch maintainer call, we decided that since this is a hard-blocker to finalizing the menu links system, this is indeed a critical issue. Tagging.

Alter the base table of the entity so that we story the user input and/or the "system" path (path after resolving aliases) in the entity.

On the call we agreed that we store the user input itself.

Let's start simple, and store what we put in.

Should we distinct clearly between the external url and the path of the input?

I don't think this is actually postponed on anything?

State in HEAD is that you can already, from an API POV store the path in the url part, and store the route name separately, so the simplest imaginable way would be to
store it there and be done.

Another try. Let's not store route name and parameter to see how many failures we get. On the longrun we will switch to the link field here.

Another try. Let's not store route name and parameter to see how many failures we get. On the longrun we will switch to the link field here.

Removing out-dated child/postponed information from issue summary.

I think the proper plan would be following: use a link field. Adapted the issue title and IS for that.

Some progress

Working on it

Let's see how much is left.

+++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_menu_links.yml
@@ -22,15 +22,8 @@ process:
+  'link/uri': link_path

Just a short observation for now: are you sure this is correct? I would think that 'link_path' needs some processing in order to become a URI.

The link field has a title and a description on it (following #2413029: Change LinkItem schema to also store a description). The menu item link also has a title and a description on it. What is stored in these and how is translation handled? This looks confusing to me.

Add missing tag.

There is a bunch of discussion on #2235457: Use link field for shortcut entity on not using the link field's title and description. I guess that applies here also. However that RTBC patch adds the link field as translatable. Would that make sense here as well?

Rerolling the patch in #14 after #2235457: Use link field for shortcut entity got in.

#17: great point. amateescu said in #2235457-3: Use link field for shortcut entity: I'd prefer to not use the title property of the link field, mostly because I see it as a main property of the entity. — by that same reasoning, the menu link entity indeed should not use the link field's description property.
But, this is putting words in his mouth — I wonder what amateescu thinks? :)

And related: #2413029-10: Change LinkItem schema to also store a description — the validity/necessity of adding a description property to the link field is now questionable AFAICT.

In #19, I incorrectly stated that #2235457: Use link field for shortcut entity had the link field translatable, that is not true. So I guess fine here to not have it translatable either.

I didn't want to derail #2235457: Use link field for shortcut entity, which was finally ready and had some nice improvements for the link field, so I stayed away from the title debate, but my current thinking is exactly the opposite of what I wrote there almost a year ago :/

Some reasons for this change of heart:

• a link is by definition composed of a title, a url and optionally other attributes
• keeping the link and description fields separate comes with a small performance cost (more field api/typed data objects being instantiated and their definitions kept in memory), and menu items are quite sensitive in this regard (but shortcuts less so)
• based on the same principle of the latest menu link meta (use a link field everywhere), it would be nice if we could also use the UI from #2407913: Discuss/define the minimal UX for adding menu links to entities everywhere, not only for menu links
• it would also help contrib a bit, the menu_attributes module could turn into a generic link_attributes module

On the other hand, if we decide to use the title property of the link field, I'm afraid we'll have some more work to do in order to support specifying a field property (e.g. link__title) as the 'bundle' key in the entity type annotation, but maybe that's not too much work?

In the end, I'm not really decided on either approach so let's discuss a bit more :)

If you want to natively suggest field properties for entity keys, which should be easy enough (see ContentEntityBase::getEntityKey()), then I would suggest link.title, because that will just work with $entity_query->condition().

Also, was not aware that you changed your opinion on that topic (well, how could I ;)), so I silently supported the decision to keep title separate in the shortcut issue based on what i remembered from our old discussion there ;)

If you want to natively suggest field properties for entity keys, which should be easy enough (see ContentEntityBase::getEntityKey()), then I would suggest link.title, because that will just work with $entity_query->condition().

I was just about to post exactly this but I refreshed the page and saw your comment :)

If you want to natively suggest field properties for entity keys, which should be easy enough (see ContentEntityBase::getEntityKey()), then I would suggest link.title, because that will just work with $entity_query->condition().

Beside that it doesn't work for base fields, see #2415001: EntityQuery should be independent from schema for fields.

What I don't like is that once you add the title to the link field, you have potential limits on the way how you built your UI, which is bad. The data modelling should not affect the way how the UI is built and wise versa.

Working on it for now.

This will be green!

Unassign it for now.

This url <-> uri thing big DrupalWTF can we have some clarity please.

1. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -72,50 +75,15 @@ public function getTitle() {
      public function getUrl() {
   

   Shouldn't it be get uri?

2. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -198,13 +151,24 @@ public function getChangedTime() {
   +        $definition['url'] = $url_object->getUri();
   
   +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -198,6 +198,7 @@ public function extractFormValues(array &$form, FormStateInterface $form_state)
            $new_definition['url'] = $extracted->getUri();
   
   @@ -299,10 +274,8 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    $entity->link->uri = $new_definition['url'];
   

   so url is uri :S

3. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -328,22 +292,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
        // @todo Use a link field https://www.drupal.org/node/2302205.
   

   We can remove this now.

I'll attempt explaining this.

URI is an identifier, URL is a locator. Roughly: URIs identify some resource, URLs allow you to locate (obtain) some resource.

IOW: we can use URIs internally as long as we translate them to URLs when presented to a browser; a browser doesn't understand entity: URIs for example.

Initial review:

1. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -72,50 +75,15 @@ public function getTitle() {
      public function getUrl() {
   -    return $this->get('url')->value ?: NULL;
   +    return $this->get('link')->uri ?: NULL;
      }
   

   This no longer matches the behavior described by the docs:
   Returns the external URL if the menu link points to an external URL, otherwise NULL.

2. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -72,50 +75,15 @@ public function getTitle() {
   +    return \Drupal::pathValidator()->getUrlIfValidWithoutAccessCheck($this->link->uri) ?: Url::fromRoute('<front>');
   

   The path validator should only be used for user-path: URIs I think?

   EDIT: d'oh, user-path: didn't land yet, so this doesn't make sense yet.

3. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -188,7 +188,7 @@ public function extractFormValues(array &$form, FormStateInterface $form_state)
   -    $new_definition['url'] = NULL;
   +    $new_definition['url'] = $form_state->getValue('url');
   

   This looks wrong; this means that also for menu links that link by route will have url set in the definition.

4. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -219,33 +220,7 @@ public function extractFormValues(array &$form, FormStateInterface $form_state)
   +    $default_value = $this->getEntity()->link->uri;
   

   Wonderful simplification here, if it's correct :)

5. +++ b/core/modules/menu_link_content/src/Tests/LinksTest.php
   @@ -147,7 +147,7 @@ public function testCreateLink() {
   -    // Check the initial hierarchy.
   +    // Check the initial hierarchy
   

   Nit: Unintended change.

+++ b/core/modules/menu_ui/menu_ui.module
@@ -212,8 +212,7 @@ function menu_ui_node_prepare_form(NodeInterface $node, $operation, FormStateInt
+          ->condition('link__uri', 'node/' . $node->id())

@@ -224,8 +223,7 @@ function menu_ui_node_prepare_form(NodeInterface $node, $operation, FormStateInt
+          ->condition('link__uri', 'node/' . $node->id())

Just in case someone wonders: This is caused by #2391217: Support base fields with multiple columns in entity queries

And here is patch.

1. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -198,13 +144,24 @@ public function getChangedTime() {
   +    if ($url_object) {
   +      if ($url_object->isExternal()) {
   +        $definition['url'] = $url_object->getUri();
   +        $definition['options'] = $url_object->getOptions();
   +      }
   +      else {
   +        $definition['route_name'] = $url_object->getRouteName();
   +        $definition['route_parameters'] = $url_object->getRouteParameters();
   +        $definition['options'] = $url_object->getOptions();
   +      }
   +    }
   

   Nit: you could pull this out of these ifs and do it in both cases, unconditionally.

2. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -327,23 +284,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ))
   +    ;
   

   Nit: Interesting code formatting :)

3. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -219,33 +220,7 @@ public function extractFormValues(array &$form, FormStateInterface $form_state)
   +    $default_value = $this->getEntity()->link->uri;
   

   This causes a notice at /admin/structure/menu/manage/tools/add:
   Notice: Trying to get property of non-object in Drupal\menu_link_content\Form\MenuLinkContentForm->form() (line 223 of core/modules/menu_link_content/src/Form/MenuLinkContentForm.php).

4. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -299,10 +274,8 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    $entity->link->uri = $new_definition['url'];
   +    $entity->link->options = $new_definition['options'];
   

   This causes a warning upon saving:
   Warning: Creating default object from empty value in Drupal\menu_link_content\Form\MenuLinkContentForm->buildEntity() (line 277 of core/modules/menu_link_content/src/Form/MenuLinkContentForm.php

5. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -162,8 +162,8 @@ function menu_ui_node_save(EntityInterface $node) {
   +          // @todo Replace it with entity:// once possiblele
   

   entity:

This is somewhat above my head, but looks like an awesome simplification.

I'm a bit surprised at the number of times MenuLinkContentForm ends up calling $this->extractFormValues(), but that's just me looking at code I never saw before, this is not added here :-)

No way to use a link widget instead of some custom $form logic here ? I guess probably not if the patch doesn't do it, but it looks like it hasn't been mentioned in the issue so far, so, just asking :-)

+++ b/core/modules/menu_ui/menu_ui.module
@@ -212,8 +212,7 @@ function menu_ui_node_prepare_form(NodeInterface $node, $operation, FormStateInt
+          ->condition('link__uri', 'node/' . $node->id())
@@ -224,8 +223,7 @@ function menu_ui_node_prepare_form(NodeInterface $node, $operation, FormStateInt
+          ->condition('link__uri', 'node/' . $node->id())

Just in case someone wonders: This is caused by #2391217: Support base fields with multiple columns in entity queries

Yup - could we link to that issue in a @todo so that we can cleanup when that other issue is fixed ?

+++ b/core/modules/system/src/Tests/Menu/BreadcrumbTest.php
@@ -248,8 +248,7 @@ function testBreadCrumbs() {
+        'link__uri' => 'taxonomy/term/' . $term->id(),

Is that the same reason/bug here ? @todo as well ?

Fixed the remaining review points ... and added a follow up for the FIeld

No way to use a link widget instead of some custom $form logic here ? I guess probably not if the patch doesn't do it, but it looks like it hasn't been mentioned in the issue so far, so, just asking :-)

Well yeah, we thought that would be easy but it turned out it isn't at all. LinkWidget has additional problems on top of the existing LinkItem had.
Therefore we have an additional issue for that: #2416955: Convert MenuLinkContent to use a link widget

Is that the same reason/bug here ? @todo as well ?

Yeah it is, because ES::loadByProperties uses entity query.

Let's get it in.

I think you uploaded the README.txt instead of an interdiff. :)

Rerolling the patch in #48 for conflicts from #2364157: Replace most existing _url calls with Url objects.

@dawehner: I think you uploaded another file by mistake instead of the interdiff. Please upload it if you think it is necessary.

Interdiff of #45 and #48.

This can't be right.

Just a couple of small fixes. The patch looks ready to me.

does the change in #53
the !
mean there is missing test coverage?

+++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
@@ -64,10 +64,11 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
-            // We allow access, but only if the link is accessible as well.

+++ b/core/modules/menu_ui/menu_ui.module
@@ -162,8 +162,8 @@ function menu_ui_node_save(EntityInterface $node) {
-          'route_name' => 'entity.node.canonical',
-          'route_parameters' => array('node' => $node->id()),
+          // @todo Replace it with entity: once possible.
+          'link' => ['uri' => 'node/' . $node->id()],

which issue would this todo be?

guessing this needs change record updates before it is rtbc.

for #55 -> #2417359: Add test coverage for MenuLinkContentAccessControlHandler::accessCheck() when "We allow access, but only if the link is accessible as well."

for #56
#2411333: Create standard logic to handle a entity: URI scheme is in.
so #2417367: Use the new entity: URI scheme will be that @todo

I'll make a new patch that references that issue.

Here is the patch.

---
Also searched for which change records might need to be updated
"entity base table" returns no results https://www.drupal.org/list-changes/published?keywords_description=entit...

"entity table" https://www.drupal.org/list-changes/published?keywords_description=entit...
returns
Database schema of content entities is automatically generated based on entity type and field definitions https://www.drupal.org/node/2259243

this patch is changing code from previous menulinkNG issues so maybe there is something to change in
Menu links converted to plugins, including static, views, and content entity implementations https://www.drupal.org/node/2302069

still looking for change records.

making a new mega change record for issues related to the meta, and putting in details from here into that.
initial guess at title: Configurable link field, short cut, menu links store user entered paths as URI (not routes or paths)
https://www.drupal.org/node/2417421

also updated the issue summary.

I think the added issue
#2417367: Use entity: in MenuLinkContentAccessControlHandler instead of 'link' => ['uri' => 'node/' . $node->id()], will be that @todo - doesn't require any work. link is not attribute in create entity thats why.

@RavindraSingh
Thanks for looking at this.
You are correct, link is not in head right now.

this patch is adding [link] to the array. so that issue will make sense when this gets committed.

----

I read through the patch a couple times, the summary is up to date, the change record draft made, and follow ups created. RTBC from me.

+1 for RTBC.

16 files changed, 78 insertions, 232 deletions.

Mmm, that's a tasty diffstat! :)

Two questions when reading through. One: What's up with this?

+          // @todo Use entity: in the URI.
+          //   https://www.drupal.org/node/2417367
+          'link' => ['uri' => 'node/' . $node->id()],

We should be able to do that as of ~20 hours ago, no? However, reading #2417367: Use the new entity: URI scheme it looks like this was deliberately split off because it could be tricky to get tests passing. Fair enough. It's not different from what's in HEAD atm which is also not using entity: so I think this is ok.

Two: What's up with this?

+          // @todo Use link.uri once https://www.drupal.org/node/2391217 is in.
+          ->condition('link__uri', 'node/' . $node->id())

However, this is also covered with a @todo, and #2391217: Support base fields with multiple columns in entity queries seems to cover this case fine.

Everything else looks pretty straight-forward, so going to go ahead and get this in so we can continue to make progress.

Committed and pushed to 8.0.x. Thanks, all!!

found #2417809: link and shortcut have baseFieldDefinition settings that do not do anything: default_value max_length
came from code copied from shortcut

Just an FYI, this patch updated files in "core/modules/migrate_drupal/src/Tests/Table" which are automatically generated from a database dump but didn't update the database dump.

This has been fixed in #2410623: D6-D8 Custom Block Migration not visible in region

I have written the unit tests in #2417359: Add test coverage for MenuLinkContentAccessControlHandler::accessCheck() when "We allow access, but only if the link is accessible as well." as requested in #58. I would appreciate a review to see whether it covers the case(s) affected by this issues.