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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Status: Active » Needs review
FileSize
998 bytes

Relatively simple patch - should mostly work OK.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

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?

naxoc’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

Here are some quick tests thrown in.

jhodgdon’s picture

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...

jhodgdon’s picture

FileSize
2.44 KB

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.

jhodgdon’s picture

FileSize
2.52 KB

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

Artusamak’s picture

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.

jhodgdon’s picture

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

jhodgdon’s picture

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?

jhodgdon’s picture

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

Tor Arne Thune’s picture

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

Still a valid issue in Drupal 7.0.

Bojhan’s picture

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.

Bojhan’s picture

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

swentel’s picture

FileSize
2.3 KB

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 :)

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.3 KB

Reroll, changed Garland to Bartik in the comment.

jhodgdon’s picture

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.

swentel’s picture

FileSize
2.25 KB

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

jhodgdon’s picture

Issue tags: -Needs tests

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

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

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

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

jhodgdon’s picture

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

webchick’s picture

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.