Feb 19, 2012

drupal_goto() is evil!

It is not, but almost! The problem with drupal_goto() is not the function itself, the problems are the drupalers (you and me and everyone who codes Drupal) that use it at will.

It is a very VERY dangerous, evil, mind-breaking, heart-breaking function if not used correctly.

Some background:

drupal_goto(), once called, immediately redirects the user to wherever we tell it to, and with doing that we are leaving plenty of potentially necessary code behind.

You don't believe me? I've seen this happening (yeas, it happened)!

Breaking form submission

Possibly the worst place you can use it is on a form validation or submission function. For that we have $form_state['redirect'], more info on how to use it can be found drupal_redirect_form() documentation.

<?php
function module_node_form_form_alter(&$form, &$form_state, $form_id) {
  ... some stuff ...
  $form['submit'][] = 'module_submit_function';
  ... some other stuff ....
}
 
function module_submit_function($form, &$form_state) {
  if (my condition) {
    drupal_goto('a random page');
  }
}
?>

In case you haven't spotted the problem, we are breaking the node saving process, so a node that is probably half way in the DB, will live without a leg (or something, who knows what was left out of it).

Breaking ajax calls

This happens when called inside a hook_init() implementation, and the if() (if there is one at all) that decides if it should be called or not, is badly thought. Picture and ajax call: the ajax call is made, the drupal_goto() kicks in, a redirect is made, html is returned and Drupal.ajax dies! (And a nasty alert -which I hate- shows up with an incredibly large amount of html code printed in it, and our clients go crazy and we have to spend some good time tracking down that evil drupal_goto() which works great according to whoever put it there, but is giving us headaches because we love ajax).

Breaking batch calls

Same as above, but in batch calls, probably no client will cry since they rarely use batch operations, but I still get pretty mad.

Breaking drush

Yes, it also breaks Drush, and you get something like:

Drush command terminated abnormally due to an unrecoverable error: [error]

Which tells me nothing, NOTHING! (But I'm a smart guy, and quickly use grep to look for drupal_goto() in all our custom modules).

Don't assume, be sure

I keep telling all of our developers to use the drupal_goto():

  • only IF their condition is absolutely met and they are absolutely sure it is the right condition and they have thought about it enough!
  • never inside an else (recipe for disaster).
  • NEVER, EVER, DOESN'T MATTER HOW TEMPTED THEY ARE should they use it inside a form validation or submission.
  • Also, never on access denied, the site won't complaint, but a proper 403 should be used (or google will get confused).
  • If you have another case, let me know!

Don't assume, be sure!.

It saved me lots of headaches!

Comments