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.
Comment | File | Size | Author |
---|---|---|---|
#20 | 790770-20.patch | 2.25 KB | swentel |
#18 | 790770-18.patch | 2.3 KB | swentel |
#15 | 790770-15.patch | 2.3 KB | swentel |
#7 | 790770-fix-doc.patch | 2.52 KB | jhodgdon |
#6 | 790770-clarify-tests.patch | 2.44 KB | jhodgdon |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedRelatively simple patch - should mostly work OK.
Comment #2
yoroy CreditAttribution: yoroy commentedPatch 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.
Comment #3
webchickSounds good to me. Could we toss in a couple of quick tests for good measure?
Comment #4
naxoc CreditAttribution: naxoc commentedHere are some quick tests thrown in.
Comment #5
jhodgdonI 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...
Comment #6
jhodgdonHere'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.
Comment #7
jhodgdonOne more try. Fixed up the docblock on the test method as well.
Comment #8
ArtusamakIt's working great, i would just have set instead of this comment:
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).
Review done.
Comment #9
jhodgdon#7: 790770-fix-doc.patch queued for re-testing.
Comment #10
jhodgdonReally do you think that comment in the test needs more description? The test header says
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?
Comment #11
jhodgdon#7: 790770-fix-doc.patch queued for re-testing.
Comment #12
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedStill a valid issue in Drupal 7.0.
Comment #13
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #14
Bojhan CreditAttribution: Bojhan commentedSo 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
Comment #15
swentel CreditAttribution: swentel commentedOk, 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 :)
Comment #16
Bojhan CreditAttribution: Bojhan commentedComment #17
catchGarland 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.
Comment #18
swentel CreditAttribution: swentel commentedReroll, changed Garland to Bartik in the comment.
Comment #19
jhodgdonHow 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.
Comment #20
swentel CreditAttribution: swentel commentedI can live with that last option, new patch :)
Comment #21
jhodgdonLooks good to me. But I will not set it to RTBC since I wrote the original patch.
Comment #22
Bojhan CreditAttribution: Bojhan commentedComment #23
catchCommitted to 8.x, moving to 7.x for webchick.
Comment #24
jhodgdon#20: 790770-20.patch queued for re-testing.
Comment #25
webchickThis seems like a very reasonable thing to do.
Committed and pushed to 7.x. Thanks!