node_access_rebuild in a batch

I updated my good friend batch_test.module to implement some dumb node access hooks.
Demonstrates node_access_rebuild_batch() in :
- hook_enable
- hook_disable
- hook_update_n

True, but not because of this patch :-)
The 'Rebuild permissions' button on this form is broken, does not redirect to admin/content/node-settings/rebuild as intended, even without the patch applied.

Probably a followup bug from the fapi3 commit. I suggest you report it in a new issue.

Forgot to set this to 'needs review', BTW.

Updated :

- I finally decided it was OK to let node_access_rebuild_batch() actually _set_ the batch, instead of returning the $batch array. You just call node_access_rebuild_batch() instead of batch_set(node_access_rebuild_batch()) previously.

- I my initial post I forgot to say a word about the TODO that was left in the code, about a slightly convoluted workaround used for a race-condition on module disable (basically, the module is still 'active' when node_access_rebuild is called in its _disable hook, but is already 'disabled' when the batch gets actually processed on the following http request, possibly resulting in {node_access} table being left completely empty).

This new patch
1) comes with a much cleaner approach, introducing a new module_disabling() funtion in module.inc,
2) frees contrib developpers of the burden of implementing their own my_module_disabling() function so that their module does not declare grants while being disabled (see node_access_example on api.d.o, or forum access for instance).

updated batch_test.module, with new / simpler hook_disable, hook_node_access_records (see the commented out bits)

chx pointed on IRC that I should clarify about the batch_test.module above :
it's not in the patch , its just there to let people test with an actual module, since I guess there are not many D6-ready node access modules out there yet.

@moshe : I shoudl probably comment that better, chx also misunderstood this part on IRC

Actually it's not 5 nodes per request, its 'process all nodes, by groups of 5 - after 1 second execution time, report progress and process the rest in the next request'. So if you have 100 nodes and processing all of them takes less than 1 sec, everything is done in one request. Just like how 'multipart update functions' worked so far.

The '1 sec execution time' is hardcoded in the batch processing engine. I took that value straight out of the old update.php code.
I considered having this as a parameter you can set when defining a batch, and then thought it was probably not such a good idea.

Why not - but you have to keep in mind that this also sets how often the progress bar gets updated and the user gets feedback.

This is not really related to this specific node_access_rebuild thread, though.
I suggest discussion about this happen in a separate thread.

BTW - there is possibly an edge-case glitch when node access modules get activated on drupal install time (say, in an install profile).
I'm trying to grab merlinofchaos on IRC about that, as I think this is also a problem with current (non-batch) node_access_rebuild : basically, you have no nodes yet, and {node_access} comes out empty, _without_ the default 'grant view perm to all' record.

following some discussion with merlin on IRC, i've been working on a slightly different angle on this, where node access modules do not directly call node_acces_rebuild(_batch), but simply set a 'needs_rebuild' flag. This should provide a better control of when exactly rebuild processes are triggered.

Unfortunately, I'm under an insane deadline for a, well, paying project, and am not moving as fast as I'd like on this. New patch in a few days, I hope.

David, Thanks for the hint - alas, the D6 implementation already went in under the name 'batch', and the API functions are named batch_set, batch_process... I proposed that name when starting the work, and no-one objected or came up with a better term during the several weeks it took to get the thing right and committed ;-)

We _might_ want to rename the whole thing, but until then, I still think that keeping the 'batch' term in the issue title makes it clearer what it is actually about. But, OK 'batchify' was probably too barbaric...

OK, getting back at this, with the approach I mentioned in #16, suggested by chatting with Earl and Moshe on, IRC (i _think_ it was them, it's been a few weeks now...) :

- When node_access table needs to be rebuilt, a flag is set using node_access_needs_rebuild(TRUE);
This is done in module.inc's module_enable / module_disable, that check if node access modules are involved.
As opposed to D5, no special hook_enable/disable or my_module_disabling() dance is needed from n_a modules themselves.
- When the 'needs rebuild' flag is set, a message is displayed on all '/admin' pages, inviting the user to visit 'admin/content/node-settings/rebuild' (confirm form) - just like in dww's update.module patch.
This avoids unexpected long batch operations when enabling / disabling modules.

- No API change for node_access_rebuild() which works in 'dual' mode, using a $batch_mode param (defaults to FALSE).
- Modules can use node_access_rebuild($batch_mode = TRUE) in hook_update_n. The batch rebuild will be appended after the last update. They can also simply use node_access_needs_rebuild(TRUE); to set the flag and let the admin trigger the actual rebuild (we might *recommend* one of the two, but both should work).

The last thing that's missing AFAICT is about install profiles. Install profiles that enable n_a modules at install time should be able to rebuild the table at the end of installation - esp. if they created some nodes as well ? With the current patch, they can either :
- set the 'needs rebuild' flag and let the user finish the work (not that 'clean', probably...)
- use node_access_rebuild() ('old' mode, no batch), which should be more than enough if they did not define 100's of nodes.

If we really want batch mode for profiles as well, additional work the installer is needed to let profiles have an opportunity to batch some stuff if they need to. I do think this can be tackled in a subsequent patch, though.

The thread was already in 'needs review' state. Well, it actually does :-)

Attached is an *example* node_access module, to let reviewers actually test the patch. The module also defines an update function to test node_access_rebuild() in updates.

Note : running the patch probably requires the batch fixes in http://drupal.org/node/149593.

My sample na_example module in the previous post had some cruft left from previous attempts. Here it is again, cleaned up.
I kept and commented out the parts that na modules had to implement in D5 to work properly, and that are not needed anymore with the 'batch na_rebuild' D6 approach.

Improved code comments as per moshe's suggestions.
Should also perform a tad faster by reducing the number of iterations (process by packs of 20 nodes instead of 5 by 5 previously - batch processing pauses after 1 sec anyway...)

A small correction / rewording to what I was saying about install profiles at the end of comment #20 :
- with current patch, install profiles that enable n_a modules will let the user see the 'node access needs rebuilding' message after install completes.
- to avoid that, they can call node_access_rebuild() ('old' mode, no batch) themselves in their custom install steps. That's ok if they do not define 100's of nodes.
- if we want install profiles to be able to use the new spiffy (and safe) batch mode, additional work on the installer is needed to let profiles have an opportunity to batch some stuff if they need to. I do think this can be tackled in a subsequent patch, though.

Rerolled after node.module split, plus added comments requested by Dries.

Basically, the node_access_needs_rebuild() flag is a user friendly way to let the admin perform the setting changes he intends to do without going through a long rebuild on each step. Or to avoid 'enable an n_a module and disable another one means 2 full rebuilds'.

And the need for both batch and non-batch mode is about the trickiness of providing a drop in 'batch' replacement for an API function. In short, batching is kosher while in a FAPI submission workflow, but not in cron, for instance (you need an actual browser to perform the redirects).

A grep through all of current D5 contrib repository brought up only 3 problematic instances of node_access_rebuild() calls :
hook_node_type('insert') in nodeaccess.module
hook_taxonomy('delete') in taxonomy_access.module
hook_user('update' / 'insert') in taxonomy_access.module
For this type of context, the callstack might be all sorts of things : form submission, cron, direct call to user_save, install profile... There is AFAICT no way we can decide if we are in a safe context for batching, thus the need for a fallback non-batch mode.
The fact that 'non-batch' is the default mode also means no actual API change.

Thx for the reroll.

About non-JS mode, this is not specific to this patch, but is related to the batch engine.
See http://drupal.org/node/168870#comment-291546. In short : it does work when JS is disabled - but *not* if you just disabled JS in order to test it, because the has_js session cookie is still here.

If you want to test non-JS batch execution, disable JS, close your browser, open it, and test. Maybe we want to improve that, but it's something to discuss on http://drupal.org/node/168870.

Cool. Marking as 'needs work' until I update the "D5->D6 migration' handbook page, and example_n_a.module

Update D5 -> D6 handbook page :
http://drupal.org/node/114774#node_access_enable
http://drupal.org/node/114774#node_access_rebuild_batch

Updated node_access_example.module in http://cvs.drupal.org/viewvc.py/drupal/contributions/docs/developer/exam...