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

Files: 
CommentFileSizeAuthor
#65 user-1365234-65-d7.patch619 bytesKevin Morse
PASSED: [[SimpleTest]]: [MySQL] 40,355 pass(es).
[ View ]
#63 user-1365234-63-d7.patch711 bytesKevin Morse
PASSED: [[SimpleTest]]: [MySQL] 40,333 pass(es).
[ View ]
#53 user_1.png13.34 KBxjm
#53 weird_user_who_can_administer_permissions_but_not_see_the_content_overview.png10.46 KBxjm
#52 user-1365234-52.patch890 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 54,060 pass(es).
[ View ]
#49 1365234_49.patch888 bytesbartmcpherson
PASSED: [[SimpleTest]]: [MySQL] 53,410 pass(es).
[ View ]
#42 node-content-overview-perm-1365234-42.patch1.23 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 50,730 pass(es).
[ View ]
#37 node-content-overview-perm-1365234-37.patch788 bytesTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 42,298 pass(es).
[ View ]
#29 node-content-overview-perm-1365234-29.patch788 bytesTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 40,376 pass(es).
[ View ]
#26 node-content-overview-perm-1365234-26.patch790 bytesTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 40,334 pass(es).
[ View ]
#14 1365234-permission-desc-link.patch946 bytesamontero
PASSED: [[SimpleTest]]: [MySQL] 37,261 pass(es).
[ View ]
#11 node-add-perm-description-1365234-10.patch559 bytesTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 39,138 pass(es).
[ View ]
#9 1365234-before.png153.78 KBdastagg
#9 1365234-after.png158.83 KBdastagg
#7 node-add-perm-description-1365234-7.patch976 bytesTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 35,399 pass(es).
[ View ]
#5 0001-Issue-1365234-by-TravisCarden-amontero-Add-descripti.D8.patch930 bytesamontero
PASSED: [[SimpleTest]]: [MySQL] 35,384 pass(es).
[ View ]
#3 0001-Issue-1365234-by-TravisCarden-amontero-Add-descripti.patch905 bytesamontero
PASSED: [[SimpleTest]]: [MySQL] 38,749 pass(es).
[ View ]
#1 drupal-1365234-1.patch845 bytesTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 37,293 pass(es).
[ View ]

Comments

Assigned:TravisCarden» Unassigned
Status:Active» Needs review
StatusFileSize
new845 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,293 pass(es).
[ View ]

Here's a patch.

tested on a fresh drupal 7.12
Patch worked fine And permission's description is displayed well.

StatusFileSize
new905 bytes
PASSED: [[SimpleTest]]: [MySQL] 38,749 pass(es).
[ View ]

Great 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 ;)

Version:7.x-dev» 8.x-dev
Component:node system» node.module
Status:Needs review» Needs work
Issue tags:+Novice, +needs backport to D7

Looks good, thanks.

This would have to go into D8 first. Tagging Novice since rerolling the patch should be straight forward.

StatusFileSize
new930 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,384 pass(es).
[ View ]

Indeed, it is :)

Status:Needs work» Needs review

StatusFileSize
new976 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,399 pass(es).
[ View ]

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

Hello! patch works good. Tested in Drupal 7.12

Status:Needs review» Reviewed & tested by the community
Issue tags:+tcdrupal2012
StatusFileSize
new158.83 KB
new153.78 KB

Tested on 8.x and it looks fine.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Works for me. Committed/pushed to 8.x.

Not sure about backport but will leave that up to David to decide.

Status:Patch (to be ported)» Needs review
StatusFileSize
new559 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,138 pass(es).
[ View ]

Here's a patch.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Why not link directly to the page, instead of referencing the path explicitly, i.e.:
"Access the administrative content overview"
?

Issue tags:+Usability
StatusFileSize
new946 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,261 pass(es).
[ View ]

I 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

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs review

I agree; that's better.

Status:Needs review» Needs work

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

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

Assigned:Unassigned» michael.otolorin

What about:

Access the content listing page
See [link]all existing content[/link] in the system

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

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

Thank 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?

Just to be clear, what I was proposing was simply this:

Access the content overview page

No description.

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

StatusFileSize
new790 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,334 pass(es).
[ View ]

Oh! 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. ;-)

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new788 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,376 pass(es).
[ View ]

Can do!

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

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

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

<?php
...
'#title' => user_acess(...)
  ?
t(...)
  :
t(...);
?>

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

That would be a change in coding standards, which should be another issue.

for permission you can use hook_perm() . It will be work fine . and also you can set permission for content access through user_access .

Status:Needs review» Needs work

Changing status to 'needs work' because a new patch is required. Referencing comment #31

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

Status:Needs work» Needs review
StatusFileSize
new788 bytes
PASSED: [[SimpleTest]]: [MySQL] 42,298 pass(es).
[ View ]

@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's hook_permission() implementation:

<?php
  $perms
['administer filters'] = array(
   
'title' => t('Administer text formats and filters'),
   
'description' => t('Define how text is handled by combining filters into <a href="@url">text formats</a>.', array(
     
'@url' => url('admin/config/content/formats'),
    )),
   
'restrict access' => TRUE,
  );
?>

Here's an updated patch.

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

Re #38: Which current code are you talking about?

The one quoted in #37. Seems to me that it does not complies with l() function docs:

This function correctly handles aliased paths and adds an 'active' class attribute to links that point to the current page (for theming), so all internal links output by modules should be generated by this function if possible.

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

StatusFileSize
new1.23 KB
PASSED: [[SimpleTest]]: [MySQL] 50,730 pass(es).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-Usability, -Novice, -needs backport to D7, -tcdrupal2012

The last submitted patch, node-content-overview-perm-1365234-42.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Usability, +Novice, +needs backport to D7, +tcdrupal2012

I think we're almost there, but what information value does the word 'existing' add?

  • Re #45: I agree that the word "existing" adds no information value and should be removed. In fact, what information does the entire description provide that the label doesn't by itself? As @yoroy observed (#25), this would not be the first instance of a hyperlink in a permission label: text formats are hyperlinked. We could probably do the same thing and drop the description altogether. If anyone needs to use permission labels for something else, they can always strip_tags(). If that's not good enough, then text format permissions are already a problem.
  • Re #32 & #42: I've never seen that approach to formatting a ternary operator. Is it done anywhere else in core? As @tim.plunkett observed (#33), that seems to contradict our coding standards.

Issue tags:+#SprintWeekend

Patch still tests fine. Agree that the word "existing" can/should be removed.

StatusFileSize
new888 bytes
PASSED: [[SimpleTest]]: [MySQL] 53,410 pass(es).
[ View ]

Generated patch for sake of seeing "existing" removed from description.

Status:Needs review» Reviewed & tested by the community

Yup, this is finally good to go.

#49: 1365234_49.patch queued for re-testing.

Issue tags:+Quick fix
StatusFileSize
new890 bytes
PASSED: [[SimpleTest]]: [MySQL] 54,060 pass(es).
[ View ]

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

Screenshots:

User 1:
user_1.png

Weird user who can administer permissions but not use the content overview page:
weird_user_who_can_administer_permissions_but_not_see_the_content_overview.png

#52: user-1365234-52.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

Multi-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() in hook_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()...

    'cancel account' => array(
      'title' => t('Cancel own user account'),
      'description' => t('Note: content may be kept, unpublished, deleted or transferred to the %anonymous-name user depending on the configured <a href="@user-settings-url">user settings</a>.', array('%anonymous-name' => config('user.settings')->get('anonymous'), '@user-settings-url' => url('admin/config/people/accounts'))),
    ),

Someone could have 'administer permissions' and not 'administer users' :)

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() in hook_permission()... and this feels mightly circular if you ask me :)

I 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. ;)

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

Status:Needs work» Reviewed & tested by the community

I 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?!

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

Status:Reviewed & tested by the community» Fixed

Committed aaf4810 and pushed to 8.x. Thanks!

Status:Fixed» Patch (to be ported)

Worth backporting to 7.x?

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

Seems reasonable to me, as long as we're just adding a string.

Status:Patch (to be ported)» Needs review
StatusFileSize
new711 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,333 pass(es).
[ View ]

Should do the trick...

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new619 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,355 pass(es).
[ View ]

Lets try this then.

Status:Needs review» Reviewed & tested by the community

Works as expected in both cases.
Thanks.

Assigned:michael.otolorin» Unassigned

Updating.

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.23 release notes

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