Since the content overview page at /admin/content
is not labeled as such and the phrase "content overview" appears nowhere in the user interface, I think the "access content overview" requires clarification. (I had to read the source code to make sure I knew what it was talking about.) I propose making it explicit with a description like the following: "Access the content overview page at admin/node."
Comment | File | Size | Author |
---|---|---|---|
#65 | user-1365234-65-d7.patch | 619 bytes | Kevin Morse |
#63 | user-1365234-63-d7.patch | 711 bytes | Kevin Morse |
#53 | user_1.png | 13.34 KB | xjm |
#53 | weird_user_who_can_administer_permissions_but_not_see_the_content_overview.png | 10.46 KB | xjm |
#52 | user-1365234-52.patch | 890 bytes | xjm |
Comments
Comment #1
TravisCarden CreditAttribution: TravisCarden commentedHere's a patch.
Comment #2
bassthiam CreditAttribution: bassthiam commentedtested on a fresh drupal 7.12
Patch worked fine And permission's description is displayed well.
Comment #3
amonteroGreat idea, Travis.
Fresh cloned 7.x-dev and applied OK.
Fixing url path and linking. Everything looks good.
Somebody retest, please.
I would be thankful if someone drops by #1308564: Basic token support in fields' help texts and helps me move that forward ;)
Comment #4
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks good, thanks.
This would have to go into D8 first. Tagging Novice since rerolling the patch should be straight forward.
Comment #5
amonteroIndeed, it is :)
Comment #6
amonteroComment #7
TravisCarden CreditAttribution: TravisCarden commentedTo conform to coding standards, "if the line declaring an array spans longer than 80 characters (often the case with form and menu declarations), each element should be broken into its own line, and indented one level". Here's an updated patch.
Comment #8
Bathie CreditAttribution: Bathie commentedHello! patch works good. Tested in Drupal 7.12
Comment #9
dastagg CreditAttribution: dastagg commentedTested on 8.x and it looks fine.
Comment #10
catchWorks for me. Committed/pushed to 8.x.
Not sure about backport but will leave that up to David to decide.
Comment #11
TravisCarden CreditAttribution: TravisCarden commentedHere's a patch.
Comment #12
tim.plunkettLooks good to me.
Comment #13
tstoecklerWhy not link directly to the page, instead of referencing the path explicitly, i.e.:
"Access the administrative content overview"
?
Comment #14
amonteroI think tstoeckler's comment hits the nail in the head.
It applies to original issue purpose and also better follows the other items in the permissions page (see for example "View the administration theme" description). However, I'm not sure if 'administrative' may be appropiate, since it's just content, not configuration.
Rerolling patch with "Access the Content overview page." description.
Also capitalizing as read on http://drupal.org/node/604342#capitalization
Related issues:
#30984: Usability: Provide descriptions for permissions
#620446: Rewrite permission titles and descriptions
Comment #15
amonteroComment #16
TravisCarden CreditAttribution: TravisCarden commentedI agree; that's better.
Comment #17
tstoecklerNow the title and the description are the same, except for capitalization (which is extra-weird; if the Content should be captitalized, which I think it shouldn't, it should be in the title as well). That's why I was a little bit more verbose in my example, to avoid exactly that.
Comment #18
amonteroI admit that title and desc are way too similar, but I also think that 'administrative' may convey a sense of a configuration-related page and may be misleading. I'm not satisfied with a help text so similar, but I preferred to avoid 'administrative'. Truth is, I also doubted about it, but I preferred to pick the lesser of two evils. Maybe it's just me who relates administrative too much with [sys]admin and is easily mislead :) However, can't it be rephrased in a way that clarifies that no admin rights are there?
About the capitalization, I just looked at existing descriptions and sticked to the guidelines linked. I really think it is the most appropiate.
Comment #19
michael.otolorin CreditAttribution: michael.otolorin commentedComment #20
yoroy CreditAttribution: yoroy commentedWhat about:
Access the content listing page
See [link]all existing content[/link] in the system
Comment #21
tstoeckler#20 looks pretty good. I'm not sold on the "in the system" part, though. As a non-technical user, what the heck is "the system"?! I haven't ever done any Drupal usability tests, though, so maybe that is a well-known term, and I just don't know about it. :-)
Anyway, thanks for stepping in this issue and un-stalling it with such a great proposal.
Comment #22
TravisCarden CreditAttribution: TravisCarden commentedDo we really need a separate description? What if we just hyperlink the words "content overview page" in the label? I opened this issue because it was impossible to know what that referred to without cracking the code. Just hyperlinking it to what it refers to solves the problem without introducing semantic issues.
Comment #23
yoroy CreditAttribution: yoroy commentedThank you. Agreed "in the system" is unnecessary. I was trying to convey that this is not a front-end view but that may not be needed.
Links in the label would be a first, I don't think we should be introducing that pattern. I'm guessing that labels are used as links to individual admin pages (like "blocks" is a link on the "structure" page) so that would get us in trouble. But, I don't know how the code works.
---
Acces the content listing page
See all existing content.
---
Acces the content listing
Get an overview of all existing content.
Maybe this last version is a bit stronger overall?
Comment #24
TravisCarden CreditAttribution: TravisCarden commentedJust to be clear, what I was proposing was simply this:
Access the content overview page
No description.
Comment #25
yoroy CreditAttribution: yoroy commentedAnd you're right, I see this is already used (see the text format permissions) so lets do that. I'd go for listing instead of overview since we have quite a few primary tabs called 'List':
Access the content listing page.
Comment #26
TravisCarden CreditAttribution: TravisCarden commentedOh! And we're not checking for access on the hyperlink. Here's a patch per #24 with an access check. @yoroy: If everyone prefers your wording, we can change it. I had already rolled my patch, and I'm "just that lazy" today. ;-)
Comment #27
TravisCarden CreditAttribution: TravisCarden commentedComment #28
tstoecklerThe user_access() check should be outside of the t() call so that instead of using a placeholder, we are putting a completely different string in t() depending on the user_access() check. I know that that is more verbose and might be considered less elegant, but that is the only way to make this properly translatable.
I don't really have an opinion on the specific wording.
Comment #29
TravisCarden CreditAttribution: TravisCarden commentedCan do!
Comment #30
amonteroReview: The patch applies cleanly and works correctly in regards to permissions. Good call.
Comments:
- I agree with yoroy concern in #23 about that linking permission *title* would be a first. I prefer "Get an overview of all existing content" from there, but as permission help text, not in the title. #25 wording is also fine.
- I think we should stick with Drupal's standards about proper capitalization (see #14), if we use the page name.
Comment #31
TravisCarden CreditAttribution: TravisCarden commented@amontero: Oh, good call on the capitalization. I overlooked that. I'll re-roll a patch as soon as there's consensus on the details.
Comment #32
tstoecklerI won't chime in on the wording/capitalization/..., but the user_access() check looks better now. I just think the wrapping is a bit strange now. Even if it is more verbose I would personally prefer a simple if/else construct. If you want to go with the ternary, I would suggest the following to make it (IMO) more readable:
(Some people prefer the '?' and the ':' at the end of the line instead of the beginning; I don't really care about that. I think that makes is more readable than the current patch, either way.)
Sorry for being so picky.
Leaving at needs review for the rest of the discussion.
Comment #33
tim.plunkettThat would be a change in coding standards, which should be another issue.
Comment #34
chanderbhushan CreditAttribution: chanderbhushan commentedfor permission you can use hook_perm() . It will be work fine . and also you can set permission for content access through user_access .
Comment #35
helenc CreditAttribution: helenc commentedChanging status to 'needs work' because a new patch is required. Referencing comment #31
Comment #36
jp.stacey CreditAttribution: jp.stacey commentedAlong with capitalization, we should also use l() rather than having a bare tag, and changing the token from e.g. @url to !link so the HTML from l() doesn't get escaped.
Comment #37
TravisCarden CreditAttribution: TravisCarden commented@jp.stacey: Actually, this is the pattern observed across core, because if you use
l()
, the link text isn't translated with the rest of the string. See, for example, this excerpt from filter module'shook_permission()
implementation:Here's an updated patch.
Comment #38
amonteroEither current code is not following recommended practice in http://api.drupal.org/api/drupal/includes!common.inc/function/l/7 or it's needed to update docs in l() function with TravisCarden's comments some way.
Comment #39
tstoecklerRe #38: Which current code are you talking about?
Comment #40
amonteroThe one quoted in #37. Seems to me that it does not complies with l() function docs:
Comment #41
tstoecklerAhh, O.K., I see what you mean. We should update that documentation to be more clear. The code in #37 is correct. The reason is translatability. If you use l() in the way that e.g. #26 does, which is what you propose, I think, translators will the the string "content overview", and have to translate it, without knowing that this actually appears as part of the permission description.
Comment #42
amonteroReroll accounting for comments #30 & #32 to set the ball rolling again.
However, I support yoroy's comment against linking the permission title instead of the description. It would be a first in this page and who knows how it can affect if someone is using permission titles from code someway. So I kept the texts above mentioned, but linked in the permission description.
Keeping Content page name capitalization and checking access before linking using ?: construct, but line breaking at each clause to improve readability.
Now it looks like this:
Access the Content overview page
Get an overview of all existing content
Also, as tstoeckler pointed in #41, I've opened #1837840: Improvement to l() function docs.
Comment #44
amontero#42: node-content-overview-perm-1365234-42.patch queued for re-testing.
Comment #45
yoroy CreditAttribution: yoroy commentedI think we're almost there, but what information value does the word 'existing' add?
Comment #46
TravisCarden CreditAttribution: TravisCarden commentedComment #47
Aravind Bharathy CreditAttribution: Aravind Bharathy commented#42: node-content-overview-perm-1365234-42.patch queued for re-testing.
Comment #48
bartmcphersonPatch still tests fine. Agree that the word "existing" can/should be removed.
Comment #49
bartmcphersonGenerated patch for sake of seeing "existing" removed from description.
Comment #50
tstoecklerYup, this is finally good to go.
Comment #51
xjm#49: 1365234_49.patch queued for re-testing.
Comment #52
xjmI added periods to the permission description, because all the other ones end in periods. I didn't provide an interdiff, because I just added two periods. I am keeping the issue RTBC for the same reason. :)
Comment #53
xjmScreenshots:
User 1:
Weird user who can administer permissions but not use the content overview page:
Comment #54
xjm#52: user-1365234-52.patch queued for re-testing.
Comment #55
alexpottMulti-line ternary operators are against Drupal's coding standards...
Also I question the need to code for the use-case "Weird user who can administer permissions but not use the content overview page". This is will introduce (afaics) the first use of
user_access()
inhook_permission()
... and this feels mightly circular if you ask me :)Additionally if the user were to click the link and get access denied... all they are going to do is give themselves the permission :)
A similar situation is ignored
user_permission()
...Someone could have 'administer permissions' and not 'administer users' :)
Comment #56
xjmI also found this weird. If we link it without the check, only this aforementioned weird user, who is probably just someone who has not set up their own permissions correctly, will ever see the message. See above; the check was introduced in #26.
Also, it looks like @tstoeckler recommended the multi-line ternary in #32, and @tim.plunkett pointed out it was against coding standards, and @TravisCarden provided a justification for the change in #37.
Edit: Getting rid of the access check solves both problems. ;)
Comment #57
David_Rothstein CreditAttribution: David_Rothstein commentedThere is at least one place in core that does a user_access() check in hook_permission() already: filter_permission().
However, the permission combination needed to trigger it here does seem a lot less likely than that one.
I don't see any problem with adding it really, but we could certainly live without it also...
Comment #58
tstoecklerI didn't actually recommend the ternary in #32 but I can certainly live with it. Although I'm not the biggest fan of it myself I have yet to see proof that it is actually against coding standards. Let's just get this in, plz?!
Comment #59
xjmThere's exactly two references to ternaries in our handbook, one of which is about configuring NetBeans, and the other of which is this cryptic item on a deprecated page under "Programming pitfalls to avoid": The dichotomy of the ternary operator".
I know there have been discussions on-issue about the use of ternaries in the past, but looks like none of them made it into the coding standards docs.
Comment #60
alexpottCommitted aaf4810 and pushed to 8.x. Thanks!
Comment #61
amonteroWorth backporting to 7.x?
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commentedSeems reasonable to me, as long as we're just adding a string.
Comment #63
Kevin Morse CreditAttribution: Kevin Morse commentedShould do the trick...
Comment #64
amonteroChanging the current 'title' field (even just casing) would break current translated strings.
I think David was meaning to add only the 'description' field and leave current title as is as a minor annoyance.
Comment #65
Kevin Morse CreditAttribution: Kevin Morse commentedLets try this then.
Comment #66
amonteroWorks as expected in both cases.
Thanks.
Comment #67
amonteroUpdating.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/38cf2ce