This bug is reproducible most simply if you set Seven as your main site theme and then go to example.com/whatever, but it is a more general problem. We should fix it - the shortcut '+' icon looks odd on these pages, and I can't imagine much of a legitimate reason to bookmark a page you can't see (plus, I don't even think it works correctly).

This is in some ways similar to #635814: Some pages should not display the "Add to shortcuts" link but that is a much more complicated issue, whereas this is a simple obvious fix. We should keep them separate so that this bug doesn't get lost.

Files: 
CommentFileSizeAuthor
#20 790770-20.patch2.25 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 36,082 pass(es).
[ View ]
#18 790770-18.patch2.3 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 33,118 pass(es).
[ View ]
#15 790770-15.patch2.3 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 32,943 pass(es).
[ View ]
#7 790770-fix-doc.patch2.52 KBjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 26,863 pass(es).
[ View ]
#6 790770-clarify-tests.patch2.44 KBjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 20,977 pass(es).
[ View ]
#4 790770-4.patch1.93 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 20,946 pass(es).
[ View ]
#1 shortcut-link-790770-1.patch998 bytesDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 20,147 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new998 bytes
PASSED: [[SimpleTest]]: [MySQL] 20,147 pass(es).
[ View ]

Relatively simple patch - should mostly work OK.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

Sounds good to me. Could we toss in a couple of quick tests for good measure?

Status:Needs work» Needs review
StatusFileSize
new1.93 KB
PASSED: [[SimpleTest]]: [MySQL] 20,946 pass(es).
[ View ]

Here are some quick tests thrown in.

I think I would be more comfortable with this test being a valid test if it also tested a page that *should* have a shortcut (using the same testing sequence), and verified that the shortcut was there.

EDIT/ADDED: I will do this...

StatusFileSize
new2.44 KB
PASSED: [[SimpleTest]]: [MySQL] 20,977 pass(es).
[ View ]

Here's a new patch with 2 lines added to the test to verify the methodology. Hope that's OK. This exact way of testing that the shortcut link is there is not used elsewhere in the test file, so for me, adding the two lines (plus comment) made the test clearer.

StatusFileSize
new2.52 KB
PASSED: [[SimpleTest]]: [MySQL] 26,863 pass(es).
[ View ]

One more try. Fixed up the docblock on the test method as well.

It's working great, i would just have set instead of this comment:

// The user does not have access to this path.

Something like that to be a little bit more clear about the notion of "admin user", i was wondering why this assertion was right according to the fact that you were logged in as "admin" (but actually you're just shortcuts admin).
// The user can not access to this path because he doesn't have "Administer modules" permission. (403 HTTP code returned)

Review done.

#7: 790770-fix-doc.patch queued for re-testing.

Really do you think that comment in the test needs more description? The test header says

+  /**
+   * Tests that the add shortcut link is not displayed for 404/403 errors.
+   *
+   * Tests that the "Add to shortcuts" link is not displayed on a page not
+   * found or a page the user does not have access to.
+   */

and that line of comment is just meant to say that this is the page the logged-in user doesn't have access to, which is what we are trying to test?

#7: 790770-fix-doc.patch queued for re-testing.

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Still a valid issue in Drupal 7.0.

I am not even sure this is favorable, if we redesign the location of the icon. I don't really want users to be confused by *magic* of things appearing and disappearing.

So sun informed me, that the right way to approach this is to fix it and backport it. But then go and remove it again from core, for D8 :D

StatusFileSize
new2.3 KB
PASSED: [[SimpleTest]]: [MySQL] 32,943 pass(es).
[ View ]

Ok, just a reroll so it applies cleanly with git. I'm ok with the original docblock. I don't really think that such long inline comments aren't needed, especially if we're going to remove this again in D8 - someone needs to explain that to me however :)

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Garland isn't the default theme in either D7 or D8. And after #911054: Remove Garland from Core is in it won't be in core, so the comment in the test can't be right.

Status:Needs work» Needs review
StatusFileSize
new2.3 KB
PASSED: [[SimpleTest]]: [MySQL] 33,118 pass(es).
[ View ]

Reroll, changed Garland to Bartik in the comment.

How about instead of saying either Bartik or Garland specifically as the default theme, we just say:

The default non-administrative theme does not display shortcut links.

That way if we change the default theme again, the comment will still be valid?

Or maybe just:

Change to a theme that displays shortcuts.

StatusFileSize
new2.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,082 pass(es).
[ View ]

I can live with that last option, new patch :)

Issue tags:-Needs tests

Looks good to me. But I will not set it to RTBC since I wrote the original patch.

Status:Needs review» Reviewed & tested by the community

Version:8.x-dev» 7.x-dev

Committed to 8.x, moving to 7.x for webchick.

#20: 790770-20.patch queued for re-testing.

Status:Reviewed & tested by the community» Fixed

This seems like a very reasonable thing to do.

Committed and pushed to 7.x. Thanks!

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

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