batch API callbacks cannot live in .inc files

I'm tempted to postpone this to D7. I've skimmed the code, and I can't spot significant code savings from this feature so I'm not entirely convinced this is (a) useful or (b) cruft. Feel free to prove me wrong though. :)

@Dries: There are no code savings for Drupal core because batch API is not really used there (except for update.php). However without this patch all contrib developers are forced to put batch api stuff into their main .module file, what leads to
a) inconsistent code distribution, because batch API callbacks that would functionality-wise belong to .inc files must be moved to .module for the only reason, that they wont work inside the .inc files
b) conflicts with the idea to split off 'unused code' for 'normal' page loads to reduce the memory usage and to speed up Drupal
c) a vague point ;) Its nowhere documented AFAIK that batch api functions cant live inside .inc files. As described in http://drupal.org/node/168870 the callback is simply not invoked without any notice or error message. It took me quite some time to figure out why it silently fails ... This patch introduces an additional property to batch, but it does not break the existing API.

@yched: Whats the $set_changed good for! I assume you want to include the file only once, right? But actually as you are using include_once() there is no need to handle this manually. include_once() is very fast if the file was included already. And you current use of $set_changes is faulty, because you set it initially TRUE and check it before inclusion, but it is actually never set FALSE (unless I overlooked that ;)).

Great work yched. Patch applies cleanly, code looks good and most important it works as expected. I'm inclined to set RTBC, but I think we need at least one independent review (and of course a decision of a core committer). However its a small, but very valuable addition.

Anyone?

I really wonder nobody else tumbled over this, but I guess thats mainly because only a very few modules are using batches at all and nobody has started to port these to Drupal 6 yet. However I still hope we can get this in, so that I can remove the wrapper function to manually include the .inc file from my module(s) ...

Yes, as said, the $set_changed stuff does not look correct.

OK, make this use with the locale batches, and it actually makes locale module a bit more performant. We have at least two batches which need their functions in locale.module because we have no such functionality introduced by this patch.

Yes, make use of this, so we can justify adding it. That way, we save code and we show gain.

needs to be re-rolled (1 out of 3 hunks FAILED in includes/batch.inc)

Also I dont see why stuff that I would usually expect in modules/locale/*.inc should be moved to (or already resides in) the 'global' includes/locale.inc. I agree with yched that it would be better to have this in locale.admin.inc or stg.

@profix898: that is because locale.inc was added more then six years ago, when the something.admin.inc naming convention was apparently not in place :) At that time, this include file did not contain the rarely included parts of Drupal localization, but locale module was still literally years ahead of other modules with moving rarely used stuff out of the module itself. I myself have asked for a move of the locale.inc to the locale module more then a year ago: http://drupal.org/node/73507

I hate to say, but still does not apply or actually fails completely now ;)

[thilo@profix drupal6-cvs]$ patch -p0 < batch_file_2.patch
patching file includes/batch.inc
patch: **** malformed patch at line 45: ### Eclipse Workspace Patch 1.0

@yched: Do you still have a modified copy on your system or should I apply and reroll manually?

OK. Here is the patch from #17 rerolled for latest HEAD.

Any chance to get this in? Developers will really miss this once they start to use batch API in contributed modules with D6. And as mentioned above they will not even get a notice why their batches dont work. At least it took me quite some time to figure out ...

It is best to have Dries' opinion on this IMHO. Because the D6 architecture (with menu callbacks and all) are fitted around more include files, it would not be fortunate to require proxy functions for batches. But unfortunately we are past beta 1 now, so we need to be (more) careful.

We still need a decision here. Gabor, Dries?
In case the proposed solution is rejected we should at least update the docs and/or add a function_exists() check for the callback.

Bumping gently ;) It would be a shame to release D6 without this very simple yet effective and code-saving addition IMO.

I agree that this is useful and saves some code, but what is more important is that it improves consistency. I quickly reviewed the patch and noticed that locale.inc is include_once-d in locale.inc after the patch which is quite interesting...

It would be great to have a deeper review from yched. I am fine with the concept but the actual patch looks like needing some eyes looking closer :)

Better to keep this simple, and tackle any new batch operations in a different patch. Solve this problem quickly and save different issues to other patches.

The patch still applies (with a small offset) and seems to work just fine (incl. auto-import of translations).

Now looked through the code again twice and did not identify issues. The whitespace issues I noticed in a code comment actually turned out to be for consistency reasons. Thanks for this consistency improvement, especially for the performance improvements now possible with contributed modules using batches. Committed.

Awesome. Thanks :)