Filefield: Wrap ajax callbacks in ob_start() / ob_get_clean()

Relevant page callbacks:

For filefield, I found
- filefield/progress/[funky noise]
- filefield/ahah/[node type]/[field name]/0 (POST)
- a few images, which do not matter for us

Associated router paths declared in filefield_menu():
- filefield/ahah/%/%/% -> filefield_js(), filefield_edit_access()
- filefield/progress -> filefield_progress(), user_access('access content')

I strongly recommend we set access callback for these items to TRUE, and do the checking in the page callback instead. And in addition, do the ob_start() and ob_get_clean() and output this stuff in an alert() or something, or send it to watchdog.

If we were not living in a Drupal world, I would, in addition, wrap the stuff in try/catch.

And the %/%/% should probably be dropped altogether, so we have a route of filefield/ahah instead. The arguments can be checked in the page callback instead.

Let's not get this filled with noise from non-filefield problems.

I was not aware of the use of register_shutdown_function() problem.
I did a bit of grep on a test site, I think I have most of the common modules installed.

.../sites/all/modules# grep -raon "register_shutdown_function(.*" .

./contrib/devel/performance/performance.module:159:register_shutdown_function('performance_shutdown');
./contrib/devel/devel.module:487:register_shutdown_function('devel_shutdown');
./contrib/devel/devel.module:740:register_shutdown_function('devel_shutdown_real');

./contrib/backup_migrate/backup_migrate.module:632:register_shutdown_function('backup_migrate_shutdown', $settings);
./contrib/backup_migrate/backup_migrate.module:688:register_shutdown_function('backup_migrate_shutdown', $settings);

./contrib/imce/inc/page.inc:469:register_shutdown_function('file_delete', $temp);
./contrib/ctools/ctools.module:172:register_shutdown_function('ctools_shutdown_handler');
./contrib/imagecache/imagecache.module:429:register_shutdown_function('file_delete', realpath($lockfile));
./contrib/node_import/node_import.inc:1980:register_shutdown_function('node_import_lock_release');

In the unfortunate case of Devel module, it actually registers a shutdown function inside it's shutdown function! This ensures it ABSOLUTELY runs last.

At least it's a finite number of steps, that we could always beat by adding one step more, if we wanted.
We would not even need to run 3x, because devel registers its function in hook_boot(). We will be a bit later.

But I think in case of devel, we can use the mechanisms provided by devel itself, to avoid the shutdown from dealing damage.

--------

performance_shutdown():
- registered in hook_boot()
- does some module_invoke_all(), which could be dangerous.

devel_shutdown():
- registered in hook_boot()
- registers devel_shutdown_real(), and does nothing else

devel_shutdown_real():
- registered in devel_shutdown()
- runs some checks to avoid breaking non-html pages.

backup_migrate_shutdown():
-> called only on specific page requests, that are not relevant to us

imce + file_delete():
-> called on specific page requests which are not relevant for us

ctools_shutdown_handler():
- registered on hook_init(), if (!empty($_REQUEST['ctools_ajax']))
- not sure if relevant to us.

imagecache + file_delete():
-> not relevant?

node_import_lock_release():
-> not relevant, i think.

Conclusion:
- The steps outlined in #1 would bring us one step further already. At least, they would eliminate problems with printed stuff before our exit() statement.
- At the beginning of the ajax page callback, we would do ob_start(), and register a shutdown function.
- At the end of the ajax page callback, we would ob_get_clean() + watchdog, then headers + print, then ob_start() again, register a shutdown function, and exit().
- The shutdown function would call ob_get_clean() again, and write the result to watchdog.
- To make peace with devel, we use drupal_set_header() to set the headers.

Ideally this would be all in a dedicated and reusable ajax module, but I think we don't really want to add new dependencies to filefield.

I think the real problem is not just those "evil other modules", but the fact that D6 lacks a special-case handling for ajax requests.
This is why modules need to add safety belts to their ajax requests.

"feasible or worth to implement"
I think the first step (ob_start + ob_get_clean in page callback) is quite cheap, and it might already help a few people.

Are you sure that a majority of the problems is caused by shutdown functions?

I think these things really need to be puzzled out separately.
- Put file size limits on <input type="file" />, taking into account module settings as well as PHP settings. EDIT: Yes, filefield_js() does a check on server side if $_POST is empty.
- Capture as much as possible on server side, and write stuff to watchdog instead of printing it. ob_start() and ob_get_clean() can improve the situation.
- Capture the rest on client side: Try to find out if we had a memory problem, file size limit, a "headers already sent" or something else, and show a meaningful error message. This should be discussed in a different issue.

There will always be some issues remaining, but the fewer the better.

EDIT: Forgot to add
- Don't use the menu system for access checking. Do this in the page callback. The default "page not found" is a html page, and we don't want that.

Sorry, last time I made an <input type="file" /> myself is a while ago. I thought there was something like <input type="file" max-filesize="1MB" /> to let the browser do the check, but that was wishful thinking.
So you are right, it needs to happen on server side.