<nolink> and <none> are two routes provided by core that can be used to create empty links, without urls. This can be done through the menu system by creating a menu item with a link route:<nolink>.

However, because these urls are empty, Drupal currently considers these link to be going to the front page.
This means two things:

  1. The links are generated with "data-drupal-link-system-path=<front>"
  2. The links are generated with class="is-active" when on the homepage.

What is the expected behaviour? Nolinks should not contain either data-drupal-link-system-path or is-active on any page.

Setting up menu link

Incorrect attributes shown

CommentFileSizeAuthor
#22 Screen Shot 2018-05-10 at 10.33.28.png339.72 KBLeon Kessler
#22 Screen Shot 2018-05-10 at 10.32.36.png289.45 KBLeon Kessler
#19 2838351-interdiff-12-19.txt3.14 KBLeon Kessler
#19 links_with_no_path_get-2838351-19-TEST-ONLY.patch1.98 KBLeon Kessler
#19 links_with_no_path_get-2838351-19.patch2.95 KBLeon Kessler
#12 interdiff.txt889 bytesLeon Kessler
#12 links_with_no_path_get-2838351-12.patch1.83 KBLeon Kessler
#9 links_with_no_path_get-2838351-9-FAIL.patch945 bytesLeon Kessler
#2 links_with_no_path_get-2838351-2.patch1.79 KBLeon Kessler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Leon Kessler created an issue. See original summary.

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented
Status: Active » Needs review
FileSize
links_with_no_path_get-2838351-2.patch1.79 KB

Patch which adds a simple check that the link isn't <nolink> when generating the data-drupal-link-system-path. This also fixes the active class issue.

Added set_active_class in the test. This fails with LinkGenerator.php in it's previous state, and passes when the update is applied.

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented
Related issues: +#1543750: Allow menu items without path
Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented
Issue summary: View changes
Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler at Mirum Agency commented
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: +Needs tests

the generated markup need tests

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler at Mirum Agency commented

@andypost the existing test for markup
$this->assertSame('<span>Test</span>', (string) $result);
is still correct...
By adding $url->setOption('set_active_class', TRUE); to the test, we are ensuring that this does not affect the output.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

I it correct then why it does not fail?
the issue supposed to be a bug then better to have some fail.patch to show that bug exists and then patch that fixes

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler at Mirum Agency commented
FileSize
links_with_no_path_get-2838351-9-FAIL.patch945 bytes

Ah okay sure, here's the patch that will fail (this contains the updated test, but without the update to LinkGenerator.php

Status: Needs review » Needs work

The last submitted patch, 9: links_with_no_path_get-2838351-9-FAIL.patch, failed testing.

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented
Status: Needs work » Needs review
Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler at Mirum Agency commented
Status: Needs review » Needs work
FileSize
links_with_no_path_get-2838351-12.patch1.83 KB
interdiff.txt889 bytes

Found another case where the is-active class is set incorrectly. When using <nolink> (which can happen if you set a menu item to link to a fragment #).

I've uploaded a new patch an interdiff, but I think this probably needs a bit more though (there are perhaps other types of links where this issue happens). Also the tests should now be expanded to cover these cases.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

FiNeX’s picture

FiNeX CreditAttribution: FiNeX as a volunteer commented

Hi, I'm also experiencing this bug using latest Drupal 8.5.1. Are the patches safe enough to be applied? Thanks!

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented

Hi @FiNeX
Yes, it may well need a re-roll, but should be safe (we had the patch running on a production site with no issues).

I should have sometime to take a look at addressing issues I posted in my previous comment soon.

Eyal Shalev’s picture

Eyal Shalev
he/him
CreditAttribution: Eyal Shalev as a volunteer commented
Status: Needs work » Needs review

The patch in #12 works on the latest branch, and it has been over a year since submitted - so I'm moving this issue to needs review.

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented
FileSize
links_with_no_path_get-2838351-19.patch2.95 KB
links_with_no_path_get-2838351-19-TEST-ONLY.patch1.98 KB
2838351-interdiff-12-19.txt3.14 KB

I have gone over the comments that I raised back in #12.

<nolink> and <none> are the only routes I can find which are effected by this issue (in core).

I have added an additional test for <none> routes.

And I've also modified LinkGenerator::generate so that rather than checking the route name, we simply check if the url is empty. To me this makes more sense, as it should cover other possible similar empty routes.

Status: Needs review » Needs work

The last submitted patch, 19: links_with_no_path_get-2838351-19-TEST-ONLY.patch, failed testing. View results

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Tests passed, setting back to needs review.

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler as a volunteer commented
Issue summary: View changes
FileSize
Screen Shot 2018-05-10 at 10.32.36.png289.45 KB
Screen Shot 2018-05-10 at 10.33.28.png339.72 KB

Updating issue summary, adding screengrabs.

Eyal Shalev’s picture

Eyal Shalev
he/him
CreditAttribution: Eyal Shalev as a volunteer commented

Leon, Wouldn't a link to the front-page have an empty url?

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler as a volunteer commented

That's already covered in the preceding conditional:

+        if ($url->getRouteName() === '<front>') {
+          $system_path = '<front>';
+        }

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

LaravZ’s picture

LaravZ CreditAttribution: LaravZ commented

The patch was applied cleanly to my existing 8.5.6 installation and a new 8.7.x installation, and the data-drupal-link-system-path en class="is-active" elements were removed. The system path now no longer defaults to but to , which seems to me to be more desirable. Do you reckon it would be desirable to mention above the code that it now defaults to ?

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler as a volunteer commented

@LaravZ, in terms of code commenting, I didn't think it was necessary to add anything here. The 

if (!empty($system_path)) {

is self-explanatory (and I think it's always preferable for code that documents itself).

However, Eyal's version (on https://www.drupal.org/project/drupal/issues/2963523) is more explicit, and as such includes a comment:

+      // <nolink> and <none> cannot be active, and as such are excluded.
+      if ($url->isRouted() && !in_array($url->getRouteName(), ['<nolink>', '<none>']) && !isset($variables['options']['attributes']['data-drupal-link-system-path'])) {

I would say my approach is simpler, and would cover other possible routes that are empty. But happy to go with Eyal's approach if that's what everyone favours.

LaravZ’s picture

LaravZ CreditAttribution: LaravZ commented

Nah, I like simple approaches as long as they suffice. And I suppose the code alteration can be traced back to this issue, so I am personally fine with not adding any additional comments.

LaravZ’s picture

LaravZ CreditAttribution: LaravZ commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record 
+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -133,8 +133,15 @@ public function generate($text, Url $url) {
+        if (!empty($system_path)) {
+          $variables['options']['attributes']['data-drupal-link-system-path'] = $system_path;
+        }

Before the data-drupal-link-system-path would always be set to something. No it is not. I think this is correct because there is no system path so that's good. But I think we need to create a change record to document the change. There's a link to add a change record above the related issues list.

LaravZ’s picture

LaravZ CreditAttribution: LaravZ commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Alright, I've added a change record: https://www.drupal.org/node/3000819

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

@LaravZ thanks for that. The change record is to tell users about the effects of the change not to rebuild the patch. So the important information here is that links with the route nolink and none will no longer have a data-drupal-link-system-path attribute. The nitty gritty of the patch is not so important.

LaravZ’s picture

LaravZ CreditAttribution: LaravZ commented

Alright, I've replaced the code in the before and after to regular text in order to properly explain the changes. Thanks, @alexpott!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

@LaravZ I've edited the change record to only focus on the important information. So people know about the change that might affect their site and not anything extraneous.

LaravZ’s picture

LaravZ CreditAttribution: LaravZ commented

Makes sense, do we need to do anything else before we can reset the status to Reviewed & tested by the community?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

@LaravZ I don't think so. As a committer it's best that I don't rtbc patches myself.

LaravZ’s picture

LaravZ CreditAttribution: LaravZ commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 8e67805 and pushed to 8.7.x. Thanks!

I've only committed this to 8.7.x because of the changes to markup which are not API per se but might in very rare cases affect a real site (or maybe a workaround put in place to overcome this issue). Only committing to 8.7.x gives people time to discover we've fixed this and adapt accordingly.

  • alexpott committed 8e67805 on 8.7.x 
    Issue #2838351 by Leon Kessler, LaravZ, Eyal Shalev, andypost: Links...
MrPaulDriver’s picture

MrPaulDriver CreditAttribution: MrPaulDriver commented

Will this be back ported to 8.6 ?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

@MrPaulDriver see #38

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

phelix’s picture

phelix CreditAttribution: phelix commented

I am running Drupal 9 and using <front>#anchorName is adding this data element and class on every link on the menu. Is there a way to fix this?