On 21 Jan 2007, at 23:53, Karoly Negyesi wrote:
Try this: give administer comments permission to anonymous (no other admin perms) and log out . Observe the comments link in the navigation block on the home page in the Navigation block for Drupal 5. The new patch will not even consider admin/content/ comment for display. To do so, it would need to access check every single visible item in the menu table. As the new system does access checking on run time via callbacks, this will lead to slowdowns. We could attack this with various caching strategies but that's not the solution.
The simple fact is that hook_menu need and hook_router should not use the same permission system. hook_menu should only do permissions, after http://drupal.org/node/73874#comment-116054 this patch is done it will be just a JOIN. This way, in a sense, hook_menu indeed can access check every single menu item. We will give the permissions a numeric ID and the JOIn can happen over menu_links, permissions_roles, user_roles -- a three table JOIN over numeric keys. Likely to be reasonably fast.
Are you saying that hook_menu, rather than hook_router, should do the access control? This sounds a little bit scary. It sounds as if securing things at the router level would be less prone to security errors. Or are you saying that both hook_router and hook_menu need to duplicate access control (in a different way)?
There are other fun problems with visible links and router items mingled into one: if foo is not defnied, foo/bar is not visible but foo/bar/this is visible and again foo/bar/this/that is not visible then how should we sort these paths? For inheritance purposes, the order I listed them is a must, but visibility purposes foo/bar/this actually appears on the first level. This is another sure sign that menu and router structures are very much not the same.
How so? *confused*
I still propose to get the patch in as it stands. Before the navigation block was added back it was actually hook_router and it was pretty much without problems. Accept that the navigation block I added back yesterday is just a development tool and not the Real McCoy yet -- eventually the quirks above will be smoothed out. It's sheer impossible to do all changes at once.
I'm a little bit nervous about committing a patch that has many unknown edge-cases. What makes it difficult to extend this patch? We gave it a several reviews and the code looks good -- it should be a good foundation to build on -- whether it is actually committed or not. -- Dries Buytaert :: http://www.buytaert.net/