Posted by David_Rothstein on May 5, 2010 at 6:27pm
11 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | shortcut.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7 |
Issue Summary
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.
Comments
#1
Relatively simple patch - should mostly work OK.
#2
Patch works:
before: http://skitch.com/yoroy/ddm8u/page-not-found-d7-before
after: http://skitch.com/yoroy/ddm8a/page-not-found-d7-after
Similar for 'access denied' pages.
#3
Sounds good to me. Could we toss in a couple of quick tests for good measure?
#4
Here are some quick tests thrown in.
#5
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...
#6
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.
#7
One more try. Fixed up the docblock on the test method as well.
#8
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.
#9
#7: 790770-fix-doc.patch queued for re-testing.
#10
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?
#11
#7: 790770-fix-doc.patch queued for re-testing.
#12
Still a valid issue in Drupal 7.0.
#13
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.
#14
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
#15
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 :)
#16
#17
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.
#18
Reroll, changed Garland to Bartik in the comment.
#19
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.
#20
I can live with that last option, new patch :)
#21
Looks good to me. But I will not set it to RTBC since I wrote the original patch.
#22
#23
Committed to 8.x, moving to 7.x for webchick.
#24
#20: 790770-20.patch queued for re-testing.
#25
This seems like a very reasonable thing to do.
Committed and pushed to 7.x. Thanks!
#26
Automatically closed -- issue fixed for 2 weeks with no activity.