Shortcuts are added with no link title (hence unusable)

For some reason parameter name is empty here:
$name = $request->query->get('name');

Maybe we should add a test here?

Yeah, probably, just wan't sure if this was the 'right' way.

This is actually a critical bug.

What is actually happening is that when you click the "add to shortcuts" contextual link, a link item is added to your Shortcuts, but it is added with an empty link title. This makes the shortcut completely unusable -- the link "shows" in the Shortcuts bar but as it has no link title, there is nothing to click.

Therefore this is a critical or at least major issue, because the normal UI for adding shortcuts (via contextual links) is totally broken.

I just ran into this today on a new install of the latest DEV code. It is still an issue. I saw it a few days ago too, but it wasn't present a week or two ago, so I think it is a recent regression.

The patch in #3 does fix the issue, when tested manually. However, I have marked this "needs work" because it definitely needs a test (and how in the world did there not manage to be a test for this in the testing suite already? It seems like pretty basic behavior of the Shortcut module?).

I also came across this issue and had the same fix. Attached is the fix + a test.

I also fixed an indent issue.

The test doesn't really test the behavior fully. It's certainly better than nothing, and it does fail without the patch, but it doesn't really test the fundamental behavior from a user perspective.

I would like to see a functional test that clicks on that "add shortcut" link, then goes to a different page and verifies that the shortcut link is actually displayed. Shouldn't be too hard... and it would prevent other regressions from breaking the real expected beahvior.

Working on this at DrupalSouth code sprint…

I think this is is, the first test I've written for Drupal 8, so may not be best practises etc…

Setting to NR for the testbot, not sure about the patch in #13 since it doesn't actually include the fix.

Thanks for the test in #13! The patch didn't apply, so the test didn't run...

It doesn't look quite right to me though:

- Is that really the link title of the link that is clicked to add a shortcut? When I look at the HTML source of the Structure admin page, it looks like the content inside the A link is:

<span class="icon"></span><span class="text">Add to <em class="placeholder">Default</em> shortcuts</span>

Which, sadly, is unlikely to be very easy to click using clickLink().

Which means probably what we'd need to do is an xpath query. Looking at what clickLink() does:

    $url_before = $this->getUrl();
    $urls = $this->xpath('//a[normalize-space()=:label]', array(':label' => $label));
    ... (calculate url_target using $this->getAbsoluteUrl() )...
        $this->drupalGet($url_target);

So probably what we could do is an xpath... The link is in:

<div class="add-shortcut"><a ...

So maybe it would be an xpath on 'div.add-shortcut a' (I am not up on xpath syntax but that is what it would be for a CSS selector, maybe someone can turn that into a working test?)

- I think the test would be more convincing if after adding the shortcut, you did a drupalGet to go to a different page, such as maybe admin/config, and make sure the shortcut is still visible there.

Working on this..

Here we go. Adding both PASS and FAIL patches with and without the Fix respectively.
interdiff(7-18) is same for both PASS and FAIL.

Made the following changes:

• Included the fix to the actual issue(the one from patch#3)
• Implemented the feedback from#16 while adding the tests.
• As mentioned earlier, it is difficult to use clickLink() here. So added a xpath query and simulated clickLink().
• Added a test to check if the shortcut exists(added correctly) after moving to a different page(admin/config).

Sorry, there was one white space in the earlier patches(#18). Adding the latest ones removing the white space.

forgot to include 'Drupal\Component\Utility\String' Re-rolling again from #19.
Ran the tests locally this time..and passed. Lets see what the test bot has to say.

Well, the "Fail" test didn't fail. There must be something wrong with the test, because what it is supposed to be testing fails when you do it manually.

jhoddon, Yes it is probably because of the following which checks for the Structure link. But there will be another Structure link which is part of Admin links(Manage toolbar section):
$this->assertLink('Structure', 0, 'Added Shortcut found on page');

I will add a specific test to check the presence of the Structure (added shortcut) in the shortcuts toolbar.

Yes, I just came to the same conclusion about why the test is passing.

An even easier solution would be to make the shortcut from a different page that is not shown in the Manage menu, such as the Cron settings page or something like that.

Yes, I was thinking about the same as well. But, If we can write a test which is foolproof irrespective of other links on the page that would be great.
In case that's not possible, I'll go with the alternative.

On the other hand, a very clear test that doesn't require a lot of work to read the code and understand is also good. If you have to have a lot of logic to detect that the correct link is there, the test code becomes harder to understand.

jhodgdon, fair enough :)

Made the following changes to the test:

• Navigating(drupalGet) to Cron settings page and adding it to shortcuts(using clickLink() replica).
• Moving to a different page, admin/structure which doesn't have any links to Cron page by default.
• Checking if there is a link to Cron page(which is added to shortcuts).

Setting to Needs Review as the FAIL patch was meant to FAIL.

Excellent work!
- I looked at the test results and agree the FAIL patch failed on the expected line, so it reproduces the bug.
- The PASS test passed.
- I had previously manually tested the code patch and it worked fine.
- The code in the test and patch seems clean to me and is well-commented.

Let's get this in!

Couple minor things that can be fixed up before commit.

1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
   @@ -65,6 +67,33 @@ public function testShortcutLinkAdd() {
   +    theme_enable(array('seven'));
   

   theme_enable() is deprecated.

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
   @@ -65,6 +67,33 @@ public function testShortcutLinkAdd() {
   +    //Test if the shortcut is added correctly after going to a different page.
   

   Missing a space after the //

Cottser, thanks for the comments, I will make the changes..

Here we go..made the changes suggested in #34
interdiff is same for both PASS and FAIL patches.

Looks right. Assuming the testbot still agrees on the FAIL/PASS, this should be back to RTBC. Thanks Cottser for noticing those issues!

C'mon testbot!

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
@@ -65,6 +67,33 @@ public function testShortcutLinkAdd() {
+    // Replicating clickLink() method as we can't use
+    // the default method to click the add to shortcuts link.
+    $url_before = $this->getUrl();
+    $label = 'Add to Default shortcuts';
+    $urls = $this->xpath("//div[contains(concat(' ', normalize-space(@class), ' '), ' add-shortcut ')]//a");
+
+    if (isset($urls[0])) {
+      $url_target = $this->getAbsoluteUrl($urls[0]['href']);
+    }
+
+    $this->assertTrue(isset($urls[0]), String::format('Clicked link %label (@url_target) from @url_before', array('%label' => $label, '@url_target' => $url_target, '@url_before' => $url_before)), 'Browser');
+
+    if (isset($url_target)) {
+      $this->drupalGet($url_target);
+    }

Woah, what? :) Do we actually need to replicate clickLink() in its entirety? We already asserted two lines above this that the link exists. So all the conditionals to check if it exists or not seem like wasted processing. We also don't need to assert that a link was clicked; if $this->drupalGet($url_target); succeeds, then the link was clicked.

In either case, though, we should definitely add a comment there explaining why we do not clickLink and be done. #16 was a difficult easter egg to find.

Thanks @chakrapani! In addition to @webchick's feedback, here are a couple minor things I noticed.

1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
   @@ -65,6 +67,33 @@ public function testShortcutLinkAdd() {
   +    // Test that the 'add to Default shortcuts' works as expected.
   

   Some weird capitalization here.

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
   @@ -65,6 +67,33 @@ public function testShortcutLinkAdd() {
   +    theme_enable(array('seven'));
   

   This function is deprecated. https://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/t...

   Is there a reason we're still using it here?

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
   @@ -65,6 +67,33 @@ public function testShortcutLinkAdd() {
   +    // Replicating clickLink() method as we can't use
   +    // the default method to click the add to shortcuts link.
   

   This comment is also wrapping too early. :)

4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
   @@ -65,6 +67,33 @@ public function testShortcutLinkAdd() {
   +    //Test if the shortcut is added correctly after going to a different page.
   

   Missing space between the // and "Test" here.

I think #42 is reviewing an older version of the patch, at least 2 and 4 have been fixed as far as I can tell - see my review in #34 (@xjm taught me well!)

Whoops, yep, @Cottser is correct -- #2 and 4 from my comment have already been fixed. :) Sorry for the noise!

It is not difficult to use clickLink() here - the XPath normalize-space() function strips tags and whitespace so we can just search for the text inside the link.

The attached patch takes a slightly simpler approach, using clickLink(), and improving testShortcutQuickLink() to actually test the link functionality (previously it only checked that the "Remove" link appeared on the page).

edit: patch has wrong filename because xjm posted while I was preparing it :)

This changed one important thing about the previous test: it would be more convincing to go to a different page in order to verify that the shortcut link was actually created, and then after removing it, again go that different page to verify it is gone.

Also, since you created a new test here, uploading a test-only patch so we can verify that it actually fails (and in the right places) without the patch would be helpful.

It would be more convincing to go to a different page in order to verify that the shortcut link was actually created, and then after removing it, again go that different page to verify it is gone.

I am not sure I see the benefit in this, I think testing that the shortcut appears or disappears from the page immediately is better, as this is the expected behaviour - for example, there could be a caching issue that means it doesn't appear until the second page load. However, the attached patches now test both the immediate page and a different one.

Looks good. Thanks for indulging me. The reason I prefer to verify on a separate page is that there could be a link in the current page with the same name, and assertLink('Cron') is not very specific. Previous test versions above suffered from a similar problem, because they added the link on the Structure page, and assertLink('Structure') exists due to the Tools menu.

So. I think this is ready to go. The tests on the FAIL patch failed in the right places (in addition to some unrelated testbot problems that we can just ignore), and the PASS patch passed. Also, the patch itself had been manually tested previously. Thanks!

@jhodgdon: The new tests mitigate against that scenario somewhat by explicitly using assertNoLink to test the link is gone immediately after removing it, but I guess having both tests can't hurt.

Ok, awesome. The test is much easier to follow now. Let's get this sucker in!

Committed and pushed to 8.x. Thanks!