Incorporate node access arbitrator into node.module

If you want to rename to hook_node_user_grants just do it. Modules need a serious upgrade anyways. I would be extremely wary of a current() call without a reset, what's the problem with array_shift?

These are just nitpicks. Extremely nice work.

I set back to review because if i set to needs work , noone will review and my nitpicks are just that.

this patch makes complete sense, and the code looks reasonable. in order to test it, we really need a compatible node_access module.

@merlinofchaos - is the forum_access in contrib compatible with this patch? i know that dries and others really want private forums and foru moderators in core. perhaps you can update that one (if needed) and attach here or in separate issue. it just might get into core too.

I gave this a good code review and good test. It holds up well. I think the benefits here are substantial. Furthermore, the risk is quite small. We are not changing at all how Drupal calculates access to nodes. We are just centralizing the writing to the node_access table so that modules can work together properly. This is consistent with rest of core. We do not have tables which any old module writes into directly.

My patch removes some fuzz, and renames the 'reset' feature to 'rebuild' since the final product is a perfectly recalculated node_access table, not an empty one. I also optimized this function for sites that don't use node_access. Those sites don't have to iterate over all nodes. Nice for the update function especially. I'd for for Earl or someone else to look at my changes in this rebuild function, so I set this top Review for now ... My next post includes a new node.install file.

We will work more on forum_access module once this patch gets in.

as promised

i have converted og in my localhost to this new framework and all is looking good.

here is a new patch which fixes a small bug in the write_grants function. it also takes care not to show the node_access rebuild settings for sites that already have a clean node_access table (the default). earl has reviewed these changes.

the .install files posted here are no longer needed. please ignore them. node_access modules will perform any needed updates themselves. this is required to do the migration without data loss. those modules did the original writing of grants to the DB.

Committed to HEAD.

I'd like to see documentation, a progress bar for regranting, and automatically doing the regranting in the future.

Actually, we need some work on the menu item done. It should not mention 'node' in the title and the categorization should be verified.

Docs look good, I have a much better understanding of the that whole mess now :-)

Can't the menu callback be simply node_access_rebuild_page, instead of explicityl calling drupal_get_form()?

I think the comma before the 'or' in the changed string is unnecessary since there are only two alternatives. I think that string could in general be made more brief and concise, for example "updated to newer code" could be "upgraded."

Reviewed. Previous concerns have been suitably addressed.
The patch still applies with a small offset.

subscribe

IMO, this is an ugly problem with a ugly solution.

The patch seems relatively straightforward and good to me. This is a weird page to have, but we have it and it should work.

Dries, any further review?

http://drupal.org/node/84562 marked as a duplicate of this.

I managed to test this. +1

Managed to test this as well, have some comments.

There is a very minor coding style issue in the patch, there is a space before function node_access_rebuild_page_submit() { that needs to be removed. This patch, since the rest has gone in should be committed.

However, going back to the original patches and not the latest fix which should go in. Does the node access menu item really have to be a menu item? The way it is coded now this query will be run on pretty much every page hit. Wouldn't it better to move it out of the menu and instead have it be an additional option on the node settings page? This doesn't strike me as a feature that needs its own menu item as you should only have to run it if something has really gone wrong. Burry it somewhere with lots of warning text to never ever run this unless you have good reason to. This way it a) doesn't have a performance hit unless the user is on a specific page (menu is slow enough as it is), b) users might not get in trouble by playing around with menu items.

Also I wonder if this needs to work like search-index and be able to run in mutiple cron runs. Anyone got any data on how many nodes/node_access modules you can have before a rebuilding of the node_access table can't happen in one run?

The other thing is that in general the node access stuff can very easilly get you in all sorts of trouble, all you need is one poorly coded module that does the wrong thing. To combat this there needs to be more documentation, both for developers so they what the Right Way is, and for end users so when they download a node_access enabled module they some documentation to get them out of trouble.

i'm fine with moving this elsewhere. don't know where though ... the whole point of this feature is to fix the "something bad happenned to my site " problem. you disable the offending module and rebuild and then the site is in a consistent state again.

This patch moves the node rebuilding button from the menu to the node settings page, and adds a confirmation step to make sure the user really wants to do this.

tested and all works well. code looks good.

this reroll fixes a typo and removes inconsistent bold descriptive text for the rebuild button. was bold, and is now normal.

Code looks good but can't help but think this is one of the ugliest (but necessary?) solutions I've seen in a while. :/ I'll commit it later on.

Dries, could you elaborate more on what is ugly? Is it the fact that we need to rebuild the node_access table, or..?

Reading the help text, it is not clear what the consequences are. Will I loose data? Will I have to fix up the individual nodes after I wiped the permissions? What can I loose? It would be great if the documentation was a little bit more 'action oriented' or 'practical' even (focus on what is expected from the user after taking this action).

It also looks weird to do a drupal_goto() in a validate function.

We should avoid using 'node' in user output -- maybe this is a valid exception. It is debatable. The fact is that things like 'node access table' are not user-friendly. My mother would wonder what a 'table' is in this context?

This looks really good work to me.

The only 'ugly' part as far as I can see is the 'rebuild' button. It would be better if we could make a 'hook_module_disable' that fires whenever a module is noted as disabled. The node module could then use that hook to trigger the rebuild.

iirc there is already a hook_uninstall just for this purpose?

Updated the help text to be more extensive.

This:

 if (db_result(db_query('SELECT COUNT(*) FROM {node_access}')) > 1 || count(module_implements('node_grant'))) {

became:

 if (db_result(db_query('SELECT COUNT(*) FROM {node_access}')) == 1) {

This does not look good to my eyes.

My bad, forgot to undo the debug if(). Fixed that and changed the comment to accurately describe what the check does.

A lot better. Committed to CVS HEAD. Thanks.

Are these API changes undocumented on converting to 5.x because they're still in flux?

I keep hearing that Node Arbitrator Module has been incorporated into Version 5.1 (which I have installed), but where is it? How do I get to it? How do I use it? I can't find any documentation on this. Please help. I need it to iron out an issue between OG and TAC. Thanks.

The code is in node.module. There really isn't a UI for it. There is a 'rebuild permissions' option somewhere if you have a node accewss module enabled. thats supposed to be used in rare cases when you think your tables might be out of whack.