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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden’s picture

Assigned: TravisCarden » Unassigned
Status: Active » Needs review
FileSize
845 bytes

Here's a patch.

bassthiam’s picture

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

amontero’s picture

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

Niklas Fiekas’s picture

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.

amontero’s picture

amontero’s picture

Status: Needs work » Needs review
TravisCarden’s picture

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.

Bathie’s picture

Hello! patch works good. Tested in Drupal 7.12

dastagg’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +tcdrupal2012
FileSize
158.83 KB
153.78 KB

Tested on 8.x and it looks fine.

catch’s picture

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.

TravisCarden’s picture

Status: Patch (to be ported) » Needs review
FileSize
559 bytes

Here's a patch.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

tstoeckler’s picture

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

amontero’s picture

Issue tags: +Usability
FileSize
946 bytes

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

amontero’s picture

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

I agree; that's better.

tstoeckler’s picture

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.

amontero’s picture

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.

michael.otolorin’s picture

Assigned: Unassigned » michael.otolorin
yoroy’s picture

What about:

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

tstoeckler’s picture

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

TravisCarden’s picture

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.

yoroy’s picture

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?

TravisCarden’s picture

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

Access the content overview page

No description.

yoroy’s picture

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.

TravisCarden’s picture

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

TravisCarden’s picture

Status: Needs work » Needs review
tstoeckler’s picture

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.

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
788 bytes

Can do!

amontero’s picture

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.

TravisCarden’s picture

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

tstoeckler’s picture

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:

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

tim.plunkett’s picture

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

chanderbhushan’s picture

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

helenc’s picture

Status: Needs review » Needs work

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

jp.stacey’s picture

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.

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
788 bytes

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

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

amontero’s picture

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.

tstoeckler’s picture

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

amontero’s picture

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.

tstoeckler’s picture

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.

amontero’s picture

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.

amontero’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +Novice, +Needs backport to D7, +tcdrupal2012
yoroy’s picture

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

TravisCarden’s picture

  • 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.
Aravind Bharathy’s picture

bartmcpherson’s picture

Issue tags: +SprintWeekend2013

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

bartmcpherson’s picture

FileSize
888 bytes

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

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yup, this is finally good to go.

xjm’s picture

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

xjm’s picture

Issue tags: +Quick fix
FileSize
890 bytes

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

xjm’s picture

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

xjm’s picture

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

alexpott’s picture

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

xjm’s picture

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

David_Rothstein’s picture

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

tstoeckler’s picture

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

xjm’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed aaf4810 and pushed to 8.x. Thanks!

amontero’s picture

Status: Fixed » Patch (to be ported)

Worth backporting to 7.x?

David_Rothstein’s picture

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

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

Kevin Morse’s picture

Status: Patch (to be ported) » Needs review
FileSize
711 bytes

Should do the trick...

amontero’s picture

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.

Kevin Morse’s picture

Status: Needs work » Needs review
FileSize
619 bytes

Lets try this then.

amontero’s picture

Status: Needs review » Reviewed & tested by the community

Works as expected in both cases.
Thanks.

amontero’s picture

Assigned: michael.otolorin » Unassigned

Updating.

David_Rothstein’s picture

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

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