Download & Extend
Taxonomy Access Control » Issues

Update hook_menu() for D7

Posted by xjm on March 18, 2011 at 5:36pm
Project:Taxonomy Access Control
Version:7.x-1.x-dev
Component:Code
Category:task
Priority:normal
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

Update hook_menu() to appropriate D7 paths, including "operations" for the modules page.

Login or register to post comments

Comments

#1

Posted by David_Rothstein on March 21, 2011 at 4:38am

This is easy to do codewise, but the question is what the paths should be...

Since the D6 paths are under admin/user, the most straightforward conversion to D7 would probably move them to admin/config/people. But I'm not sure that makes sense.

These core issues could be relevant, but it doesn't look like anyone thought about node access modules there unless I missed it while skimming:
#546956: [meta-issue] Overhaul of Information Architecture
#627080: [meta-issue] Additional categories admin/config

I spent a little while looking at this page to see what other node access modules are doing for D7:
http://drupal.org/project/modules?filters=tid%3A13434%20drupal_core%3A10...

And the answer is not much, so far:

  1. The most relevant one is obviously tac lite - looks like they just did a straight find+replace changing admin/user to admin/people, but that puts the admin page as a tab on the People page, which doesn't seem like a good idea to me.
  2. The Nodeaccess module's configuration page lives under admin/config/system in D7, which might make some sense.

So possible suggestions here might be:

  1. admin/structure (gets it next to Taxonomy)
  2. admin/config/system
  3. ???

#2

Posted by David_Rothstein on March 21, 2011 at 4:50am

I guess the logic behind admin/people is that the module's admin screens are very role-based, and that's where the main Roles page already is (and the Permissions page is related too, of course).

So if all content access modules in D7 generally congregated underneath a single "Access control" tab in the People area (or something like that) it could actually make sense. But it won't look good if each module tries to provide a tab on that page all by itself.

#3

Posted by xjm on March 21, 2011 at 2:22pm

Putting it on a sub-tab at admin/people/permissions might be a good idea. That's where I always went to look for access control configuration when I first started using Drupal. That way it would be of a level with Roles (although I find the current placement of Roles terribly confusing; I think it should be up a level next to List and Permissions rather than under the latter).

A single access control tab at admin/people/permissions would help address concerns like in #536640: Add 'package' to the .info file, and would also be a good place to provide a link to rebuild permissions. (It took me forever to find that one in D7; for awhile I just gave up on it and rebuilt permissions using drush.) Of course, people usually don't install that many access control modules. Still, it would be good for all node and field access configuration to be in a central place on or near the Permissions page.

I imagine experienced users will probably ignore the menu system and look for Configure on the modules page, but that doesn't resolve what that would actually link to.

Eventually I want to additionally provide configuration on individual role, term, and vocabulary pages. That should take care of anyone who looks for it near Taxonomy.
#364058: Allow configuration of TAC permissions on term/vocab create/edit
#146671: Alternative view: Roles by category

I'll try to get some more feedback on this.

#4

Posted by xjm on March 21, 2011 at 2:29pm

admin/config would also be a good place to have an access control category, but #627080: [meta-issue] Additional categories admin/config hasn't been touched in over a year. Hmmm.

#5

Posted by xjm on March 21, 2011 at 2:33pm

Oh, we should also make sure to accommodate an additional settings form in the placement, e.g. for #925166: Allow exclusion of certain vocabularies on configuration page.

#6

Posted by xjm on March 21, 2011 at 7:52pm
Status:active» fixed

#7

Posted by xjm on March 21, 2011 at 7:54pm
Status:fixed» active

Whoops, wrong issue.

I've moved all the module's config paths to admin/taxonomy_access for now so that they're at least functional.

#8

Posted by xjm on March 21, 2011 at 8:19pm

I added a constant for the module's main configuration path so that we can change it easily in the future if needed:
http://drupalcode.org/project/taxonomy_access.git/commit/f8cc4da

#9

Posted by David_Rothstein on March 23, 2011 at 1:53am

(although I find the current placement of Roles terribly confusing; I think it should be up a level next to List and Permissions rather than under the latter)

If that were an issue for D8 core, I think I'd probably support it :)

admin/config would also be a good place to have an access control category, but #627080: [meta-issue] Additional categories admin/config hasn't been touched in over a year. Hmmm.

So I think that if all the contrib modules agreed on the category, it would be possible to settle on one without touching core. Each module would have to define that top level category (admin/config/access or whatever) in its own hook_menu(), but as long as everyone agreed and defined it the same way I believe that would actually work and not break anything, right? Same goes for a tab on the permissions page.

I added a constant for the module's main configuration path so that we can change it easily in the future if needed

Hm, personally I'm not sure if I'd keep that in the long term - I don't think it's so great for code readability. Just my opinion. It's pretty useful for now though, if you want to experiment with going back and forth between a couple URLs :)

#10

Posted by xjm on March 23, 2011 at 5:53pm

Re: the roles menu item location: #884086: Roles are quite hidden at the moment

I'm actually not sure that several modules declaring the same menu path world work... I'll try writing a couple test modules and see what happens.

#11

Posted by David_Rothstein on March 29, 2011 at 3:44am

I was just about to say you were right.... because I realized that array_merge_recursive() can really mess this kind of thing up (it would turn strings into arrays and such). And that is indeed how most Drupal hooks wind up get invoked. Is that what you were thinking?

But then I looked and I think it would be OK after all... http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_router... uses array_merge() when invoking hook_menu(). I'm not sure if that's by design or by accident, but I think it does mean that if several modules define the same menu path, they should be able to merge together correctly.

#12

Posted by xjm on March 30, 2011 at 11:12am

Aye, I was able to declare the path in two modules and nothing blew up, at least. I didn't try declaring the same path differently between the two modules, though, because I couldn't get a simple menu "page" working--apparently system_admin_menu_block_page() only works for system.module menus in D7. So each module would additionally have to declare its own callback for admin/people/permissions/node-access or admin/config/node-access or whichever? Unless there is a solution I'm missing.

#13

Posted by David_Rothstein on March 30, 2011 at 12:51pm

Hm, in what way did system_admin_menu_block_page() not work?

Since it lives in modules/system/system.admin.inc, there's some key you have to set in hook_menu() to tell Drupal that your menu item's include file is relative to a different module's directory (a.k.a. not relative to your own module, which it assumes otherwise). I know I've been tripped up by that before.

Other than that, I would think it should work.

#14

Posted by xjm on June 23, 2011 at 11:47am

I never did figure out how to get system_admin_menu_block_page() to work for a custom module... the menu page just came up blank.

For now I'm going to put the module config at admin/config/system like nodeaccess does, since that's at least easy to find. We can revisit the pan-module issue once the module's working, for sure.

#15

Posted by xjm on June 23, 2011 at 12:58pm
Status:active» needs work

http://drupalcode.org/project/taxonomy_access.git/commit/e04c99935e74788...

Marking "needs work" for a better path choice.

#16

Posted by xjm on June 26, 2011 at 11:37am

See: http://drupal.org/node/549094 (Bojhan pointed these docs out on IRC)

People

People is only the listing of all users. Modules could for example expose other data here, such as to which event users are related to. No modules should add their configuration here, only listings related to the object people (such as events).

Configuration

This section is for all configuration, any module which exposes configuration should be placed here. One has to carefully pick the right category for his modules configuration to live in.

[snip]

People

This category should hold all modules that would fit the old "User Management".

Based on this, I believe the appropriate path for any node access module is admin/config/people/modulename. To get all access control config in one place, it would be a good idea to open issues against other modules suggesting they use this pattern.

#17

Posted by xjm on June 26, 2011 at 11:38am
Status:needs work» fixed

http://drupalcode.org/project/taxonomy_access.git/commit/668cfb4

#18

Posted by xjm on June 26, 2011 at 6:40pm

The following projects should probably use admin/config/people rather than their current paths:
Nodeaccess
TAC Lite
Path Access
Flexi Access
Comment Access

I opened issues with each of these projects suggesting changing the path.

#19

Posted by System Message on July 10, 2011 at 6:42pm
Status:fixed» closed (fixed)

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