Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kevinquillen’s picture

Version: 6.x-1.4 » 6.x-2.0-beta1
rickupdegrove’s picture

I am also looking for this feature.

Dave Reid’s picture

Version: 6.x-2.0-beta1 » 7.x-1.x-dev
Category: support » feature
Status: Active » Postponed (maintainer needs more info)

I'm wondering what the use case is. Pretty much we only alter existing UIs that already require the 'administer menus' permission either on the node form or the menu settings.

kevinquillen’s picture

Separation. For some users, it makes sense that they can add menu items and administrate menus (the links and structure) without seeing the extended attribute forms. For example, having an administrator, and simply having a content manager role. The content manager can still create menu items, or extraneous menu items (via Menu administration) but not necessarily need to see/edit menu attributes.

Maybe I am misunderstanding- but don't you need 'administer menus' to get to the menu links page to add links?

Dave Reid’s picture

Anyone with the 'administer menus' can do *anything* with menus and menu links, so I still don't see why we need to have a separate permission for menu attributes.

kevinquillen’s picture

I guess what I am saying is, if you don't have 'administer menus' permission, you can't add menu links from the admin. This is often necessary for navigation links that are not a node (usually ones that go off-site, or aliases of internal links for SEO). This is a great module for a themer, but confusing for end-user (a client) who sees the additional fields but won't use them.

Example:

UID 1 / developer / site administrators - have administer menus permission, can add/change menu attributes for theming or SEO
lower roles / content editors - have administer menus permission, but does not need to see/use anything about menu attributes

'administer menus' permission is important for both roles to create additional links from the menu administration.

mErilainen’s picture

+1
Some things are only useful for administrators and confuse the end-user. I've been working on a project to simplify Drupal administration, and it's a lot of work where granularity of permissions is very important.

kevinquillen’s picture

FileSize
2.87 KB

Here is a patch I made to accomplish this. Might need to clear your cache, since hook_menu was changed.

It adds a 'administer menu attributes' permission, and with it, it will add attributes to forms that have menus attached.

kevinquillen’s picture

FileSize
2.87 KB

Noticed a typo in the hook_perm in previous patch. Resubmitting

amateescu’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
4.07 KB
4.44 KB

I agree with #7 because I was also confronted with this issue, clients asking about some options that they'll never have to use. I had to alter some forms to hide those advanced options so I can surely see why this would be a good feature.

Patches attached for 6.x-1.x (see http://drupal.org/node/1241828#comment-4833998) and 7.x-2.x.

Dave Reid’s picture

Status: Needs review » Needs work

This is adding access to the wrong places.

function _menu_attributes_form_alter() should be using the following: rather than having user_access() called in all of its callers:
$form['options']['attributes']['#access'] = user_access('administer menu attributes');

menu_attributes_form_menu_configure_alter() should just return rather than wrap its stuff in user_access() - less indentation makes the code easier to read and there's no reason for that function to continue if the user does not have the permission.

Also, we need to consider an upgrade path adding the permission to any role with the 'administer menu' permission since this would result in a loss of UI.

amateescu’s picture

Status: Needs work » Needs review
FileSize
2.25 KB

Nice! It's great to learn best practices like this :)

New patch for 7.x. If you are ok with it, I'll roll one for 6.x as well.

Status: Needs review » Needs work

The last submitted patch, 1077426-menu_attributes_permission-12.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
amateescu’s picture

Status: Needs review » Patch (to be ported)

Commited #12 without the 'Implements hook_update_N()' doxygen to 7.x-1.x.

http://drupalcode.org/project/menu_attributes.git/commit/4dbf8c2

amateescu’s picture

Version: 7.x-1.x-dev » 6.x-2.x-dev
kevinquillen’s picture

For some reason I haven't been getting notified of updates on this thread.

I am a little confused by the last few posts- is this feature now in the newest version of Menu Attributes? Or still just the dev?

kevinquillen’s picture

Nevermind- I see now. I misread one post and saw the status change in the other.

Thanks for the progress here all.

cweagans’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Needs review
FileSize
1.24 KB

Not quite. The related #access property is being set like three times in the alter function, but it all gets overridden at the end when the function checks for any visible children.

The attached patch fixes it in 7.x.

Status: Needs review » Needs work

The last submitted patch, make_permissions_work.patch, failed testing.

cweagans’s picture

Ahem. With correct PHP syntax this time.

amateescu’s picture

Status: Needs work » Needs review

Is it me or this is the same patch? :)

Status: Needs review » Needs work

The last submitted patch, 1077426_21-make_permissions_work.patch, failed testing.

phenaproxima’s picture

After patching with #21, add a } on line 228 to fix the syntax error.

cweagans’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

One more new patch. This one should be good.

Status: Needs review » Needs work

The last submitted patch, 1077426_25-make_permissions_work.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

New patch. Added permission to unit test.

dalin’s picture

Status: Needs review » Reviewed & tested by the community

Looks great & works great.

amateescu’s picture

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

Look good to me as well :) Committed to 7.x-1.x, thanks!

kevinquillen’s picture

Is there an update that applies the new permission to roles that have administer menu already? Seems like the last thing to include with a message notifying the admin to adjust their permissions for the new change (if some roles need administer menu attribtues removed).

cweagans’s picture

There's no update, and I don't think it makes a lot of sense to add one. The permission was already there....it just didn't do anything.

Stolzenhain’s picture

Did anybody work on patching this for Drupal 6?
I was a bit confused on all the version changes on this issue.

  • amateescu committed 4dbf8c2 on 8.x-1.x
    Issue #1077426 by Kevin Quillen, amateescu and Dave Reid: Added...
  • amateescu committed fdd462e on 8.x-1.x authored by cweagans
    Issue #1077426 by cweagans: Followup: Fix permission checks.
    
joelpittet’s picture

Issue summary: View changes
Status: Patch (to be ported) » Closed (outdated)

Closing to triage the issue queue, likely won't be patching 6.x at this point.