Drupal bad practices

Fri, 2014-05-09 18:47

Below is a list of bad practices I have seen from other people and myself. Feel free to comment if you have your own thoughts.

You are a bad Drupal developer if:

  • You have no idea what is the difference between uninstalling a disabling a module

  • Remember - Disabling module does not remove any database tables from the database, nor remove any variables the module might have stored. You must disable a module to uninstall it. Uninstalling will remove any tables defined using hook_schema, and it will also invoke hook_uninstall which modules use to remove their variables and other data.

  • You abuse variable_set() and system_settings_form

  • It's really easy to store a variable or create a settings form using them, but storing tons of data will cause your site to hold all the data in memory for each and every page request.

  • You don't really use Form API's #tree property.

  • If you want to get the values in form values array in a structured way, go ahead and use #tree property.
    When creating configuration forms that you store values Drupal core variable system, note that you can throw anything to the database. Drupal will serialize your data anyway. If you have forms that has dynamic number of variables to store (per-node-type configurations, etc), consider using the #tree in the parent fieldset. This will put only a few rows to the the variable table, and will perform faster in when unseriazing data.

    Example (don't do this!):

  $form = array();
  $types = node_type_get_names();
  foreach ($types as $type => $label) {
    $form['my_module_config_' . $type] = array(
      '#type' => 'select',
      '#options' => drupal_map_assoc(range(1, 10)),
      '#title' => t('Select value for %type', array('%type' => $label)),
      '#default_value' => variable_get('my_module_config_' . $type, 5),
    );
  }

  return system_settings_form($form);

Above snippet simply gets the list of node types available, and adds a select list for each node type.
Why this bad:

  • Many rows in the variable table
  • It's good in performance to unserialize() a large array than unserialize a number of small elements.
  • Deleting a node type will not remove the row from the variable table unless you hook into that process as well.
  • Form is ugly

Do this:

  $form = array();
  $types = node_type_get_names();
  $form['my_module_config_node'] = array(
    '#type' => 'fieldset',
    '#title' => 'Node type configuration',
    '#tree' => TRUE, //  $label) {
    $form['my_module_config_node'][$type] = array(
      '#type' => 'select',
      '#options' => $opts,
      '#title' => t('Select value for %type', array('%type' => $label)),
      '#default_value' => !empty($values[$type]) ? $values[$type] : 5,
    );
  }

  return system_settings_form($form);

Why this is good:

  • Only one row in the variable table
  • Requires only one unserialize() call.
  • If you delete a node type, it will be removed when saving the form next time
  • Only one variable_del('my_module_config_node') in hook_uninstall.
  • It's sexy to have fieldsets and big arrays. Drupal is all about giant arrays after all.

Even if you use system_settings_form, you can add your own form validation hooks to alter $form_state['values'] before saving them to database.

  • You use t() function in your installation hooks, hook_schema and hook_menu

  • Note that t() won't work well during installation. Also, don't wrap your schema definition labels with t() function calls. It's no use unless you are looking for some troubles. Same goes to hook_menu implementations. Drupal will pass the title and description to t() function automatically unless you override the title callback.

  • Don't use user_access() frequently enough

  • Don't check if the user has a particular role, path checks, or anything to work around an access check. Use user_access() to check proper permissions.

  • You edit the tpl.php file in module folder

  • Some modules provide template files to make it easier for you to override the default output. Copy the template file from the module to your theme's templates folder and make your changes in the copied file.

  • Drupal core is >=7.x and you still call drupal_add_js() and drupal_add_css()

  • It works, right? Not always. Cached pages won't probably have the the assets because you are not attaching them what Drupal tends to cache. Use #attached property to attach the assets in a cache-friendly way.

  • You haven't used #states at all and you still think your forms are sexy

  • Make your forms sexier without any single line or JavaScript. See the awesome documentations here and here.

  • You can't live without PHP module in core or Views PHP module

  • For the 132,946th time, don't enable PHP module, and try your best to avoid putting PHP snippets in the database. If you need a special Views field, filter or sort handler, just extend the most matching handler and ut your precious PHP code there. PHP in a block ? hook_block_info / hook_block_view. PHP in a node ? hook_menu, and a custom page callback.

  • You always use mail() function directly to send emails

  • It's just a drupal_mail call and a hook_mail implementation. Don't use mail() or call the send method in the mail class directly.

  • You don't enable CSS and JS aggregation in Drupal core

  • I live in the country that has cheapest access to the Internet (though I doubt the quality is good). No regular use wants to know how well commented your CSS files are. If you have forgotten, go to Administer > Site Configuration > Development > Performance and enable CSS/JS aggregation. If your site is broken after that, see my point above to not use drupal_add_js and drupal_add_css.

  • You have 10+ roles in your site

  • Slideshow manager, Developer, Administrator, Blog writer, Junior Blog writer, Content approver, Backup maker... it's messy isn't it?

    Edit: Removing this item. Number of roles will depend on the requirement, but nonetheless, try to keep the number of roles as little as possible. It will not only make administering permissions easier, but also the server use less memory (permissions are cached in a static array indexed by the roles the current user has. If you have too similar roles with a large number of granted permissions, that can make the memory use slightly high). Also see user_role_permissions().

  • phpmyadmin cries internally when you SHOW TABLES

  • Reuse the fields as they match, uninstall modules properly, and don't host all your sites in a single database.

  • You have Colorbox, Shadowbox, Views Slideshow and 10 more modal and slideshow plugins enabled

  • Note that many of those module tend to add the assets even if you don't use it anywhere. Try to keep the number of such module low and reuse them.

    You are probably not aware about this, but ctools module comes with an awesome modal framework. Custom forms, in-modal-form validation, graceful degradation, and almost everything Chuck Norris might need.

  • You include jQuery in a tpl.php file

  • The demo web page of almost every 989,947 jQuery plugins out there, they include jQuery in the page source. If you are copying anything from that demo file, don't copy jQuery. It comes with Drupal core, and reusing it can save you nearly 70kb of page size.

  • You have several page-* and node-*tpl.php files

  • For smaller variants, implement hook_preprocess_node in your theme and make those changes. You can change, remove and add custom elements there. Besides, adding too much logic to tpl.php files is not a very good idea either.

  • variable_set() calls in regular page loads

  • Note that if you change a variable, Drupal has to refresh its caches which can add up a lot of time. Doing it on each page is nearly a disaster.