Hi, The current menu patch does work well. You see a navigation block and you can click links, the menu expands, you can visit pages and they load. All is well. 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. So, what will happen eventually is that you are displayed a tree of links that you have permission to see. When you click on an item you might end up with a 403. The 403 comes from hook_router which will be able to use access callback/arguments for access checking because we need to do this only once per page so it wil not be slow. Note that in Drupal 5 no access check is performed on custom items, so for example if you put a node into the menu that the user can't access then the user will see the link but clicking on it will give her a 403. Even if the user has no access content permission, this will happen with Drupal 5. The new menu system will at least check access content, if not node_access. But this makes the split absolutely ineviatable. 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. 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. Regards NK
Gerhard tells me that this mail is not understandable at all. How true. I will not explain everything in it for now. Menu documentation is written for the parts that are complete but this mail talked about future, yet undocumented work. So what I wanted to say is only this: the menu patch works well for pages now, but the navigation block will have problems in certain edge cases and you should be aware that despite I added it back, it's only a tool so that Drupal development does not come to a standstill while I work on further menu parts. Even more than usual, using it on a live site of any sort must not happen. Also, I announced that I because I have two problems that can not be overcome via other means, hook_menu will be split into hook_menu and hook_router. Thanks for your attention, NK
Actually, Karoly, I understood what you wrote and it sounds like a good solution to the problem. Well done! I still vote for hook_dispatch instead of hook_router. ;-)
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/
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)?
Yes. We need to do two access control, let's say an "access control light" for hook_menu and that's permissions -- as I have outlined this is more than what Drupal 5 does for custom menu items. And an "access control full" and that's callbacks for hook_router.
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*
The sorting for hook_router is based on number_parts (which in the above example for foo/bar/this is three) but hook_menu needs depth (which in the above example for foo/bar/this is one) -- and you can only sort on one or the other.
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.
It is a good foundation to build on it. The router functionality as implemented can and will stay unchanged aside from the small fact that its hook will be renamed. Please note that this is exactly what I planned for this phase. You asked for a navigation block so that Drupal development remains easy and I spent more than a full day doing one. It works quite nicely until you start giving acccess to a role to some item without giving access to its parents or defining a tree of paths with visible and invisible items intermingled -- core does not have such a one. There are no unknowns -- I know of every problem and have a plan to fix them. The reason I can't do the right thing for the link functionality is that there are at least two other patches (Steven's url arguments array patch and the normalize permissions) that I depend on and one of them (the norm.perm. one) is nowhere near finished. Can we hold this patch and wait for the others to finalize? Of course that's a possibility which I really would not like because currently the menu patch works and as features will be committed it will be quite a task and a waste of time to reroll and reroll a 200K+ patch to keep up with HEAD. Regards, NK
On 23 Jan 2007, at 09:49, Karoly Negyesi wrote:
It is a good foundation to build on it. The router functionality as implemented can and will stay unchanged aside from the small fact that its hook will be renamed. Please note that this is exactly what I planned for this phase. You asked for a navigation block so that Drupal development remains easy and I spent more than a full day doing one. It works quite nicely until you start giving acccess to a role to some item without giving access to its parents or defining a tree of paths with visible and invisible items intermingled -- core does not have such a one. There are no unknowns -- I know of every problem and have a plan to fix them.
I'm willing to commit your patch. It's a bit risky but a little bit of courage and trust never hurts -- it's often the path to real innovation. ;) @all: this mean that the trunk of CVS (formerly known as 'HEAD') will be somewhat broken for a while.
The reason I can't do the right thing for the link functionality is that there are at least two other patches (Steven's url arguments array patch and the normalize permissions) that I depend on and one of them (the norm.perm. one) is nowhere near finished.
Should we get these patches in first? It might reduce our downtime ...? Just thinking up loud. -- Dries Buytaert :: http://www.buytaert.net/
On Wednesday 24 January 2007 01:40, Dries Buytaert wrote:
@all: this mean that the trunk of CVS (formerly known as 'HEAD') will be somewhat broken for a while.
How broken is it supposed to be? I wanted to go in and squash a couple of E_ALL notices... but I get 404 wherever I go. Can't do anything. I tried to make a clean install of head, but I can't: "Your MySQL Server is too old. Drupal requires at least MySQL 4.1.0. (Currently using MySQL database 4.0.26)" Augustin. -- http://www.wechange.org/ Because we and the world need to change. http://www.reuniting.info/ Intimate Relationships, peace and harmony in the couple.
Augustin (Beginner) wrote:
On Wednesday 24 January 2007 01:40, Dries Buytaert wrote:
@all: this mean that the trunk of CVS (formerly known as 'HEAD') will be somewhat broken for a while.
How broken is it supposed to be?
I wanted to go in and squash a couple of E_ALL notices... but I get 404 wherever I go. Can't do anything.
great. no more notices then :) you need to rebuild your menu. add a call to menu_rebuild() in index.php just for one page view and then remove. there are probably other ways.
I tried to make a clean install of head, but I can't:
"Your MySQL Server is too old. Drupal requires at least MySQL 4.1.0. (Currently using MySQL database 4.0.26)"
seems pretty clear to me ... the minimum mysql has changed. you can upgrade or never use future versions of drupal. your choice.
You have pissed me off with: ( ) Not contributing ( X ) Going totally offtopic ( X ) Not reading my mails ( ) Not reading docs To repent you need to ( ) Bang your head with a hammer ( X ) Review patches ( ) Write handbook pages Kind regards, Karoly Negyesi
On Wednesday 24 January 2007 01:40, Dries Buytaert wrote:
@all: this mean that the trunk of CVS (formerly known as 'HEAD') will be somewhat broken for a while.
How broken is it supposed to be?
I wanted to go in and squash a couple of E_ALL notices... but I get 404 wherever I go. Can't do anything.
I tried to make a clean install of head, but I can't:
"Your MySQL Server is too old. Drupal requires at least MySQL 4.1.0. (Currently using MySQL database 4.0.26)"
Augustin.
-- http://www.wechange.org/ Because we and the world need to change.
http://www.reuniting.info/ Intimate Relationships, peace and harmony in the couple.
----- End Original Message -----
On 1/25/07 1:02 PM, Karoly Negyesi wrote:
To repent you need to
( ) Bang your head with a hammer
Just for the record, he does sometimes pick this one. You've been warned. -- James Walker :: http://walkah.net/ :: xmpp:walkah@walkah.net
participants (6)
-
Augustin (Beginner) -
Chris Johnson -
Dries Buytaert -
James Walker -
Karoly Negyesi -
Moshe Weitzman