drupal_get_filename() is broken; see #341140: drupal_get_filename() when database is down, does not deal with phptemplate themes.

When that is fixed in Drupal 6, replace _zen_path() with drupal_get_path('theme', 'zen').

Comments

johnalbin’s picture

Status: Postponed (maintainer needs more info) » Postponed
akalata’s picture

Status: Postponed » Closed (fixed)
donquixote’s picture

Component: PHP Code » PHP code
Status: Closed (fixed) » Needs work

drupal_system_listing() is expensive.

I don't understand the exact problem with drupal_get_path('theme', 'zen'), but following the title of #341140: drupal_get_filename() when database is down, does not deal with phptemplate themes, it sounds like this only applies if the database is down? Also, what exactly happens if you call drupal_get_path() instead of _zen_path() ?

My proposal:
We check if the database is there, or if whatever condition applies that allows us to use drupal_get_path().
We could also check if the returned path contains the zen.info file.
Only if this fails, we call drupal_system_listing().

I will create a patch, if someone can tell me what the exact problem is we need to work around.

--------------

For reference, here the current code of _zen_path(): (in 6.x-2.1)

<?php
/**
 * Returns the path to the Zen theme.
 *
 * drupal_get_filename() is broken; see #341140. When that is fixed in Drupal 6,
 * replace _zen_path() with drupal_get_path('theme', 'zen').
 */
function _zen_path() {
  static $path = FALSE;
  if (!$path) {
    $matches = drupal_system_listing('zen\.info$', 'themes', 'name', 0);
    if (!empty($matches['zen']->filename)) {
      $path = dirname($matches['zen']->filename);
    }
  }
  return $path;
}
?>
donquixote’s picture

Status: Needs work » Needs review

Here is my proposed replacement of _zen_path().

<?php
function _zen_path() {
  static $_path = FALSE;
  if (!$_path) {
    if (db_is_active()) {
      $_path = drupal_get_path('theme', 'zen');
    }
    else {
      $matches = drupal_system_listing('zen\.info$', 'themes', 'name', 0);
      if (!empty($matches['zen']->filename)) {
        $_path = dirname($matches['zen']->filename);
      }
    }
  }
  return $_path;
}
?>

I could make this a patch, but for now I just want some feedback..

barraponto’s picture

looks good. but if you want actual feedback, from people testing the code and benchmarking, you got to provide a path. but i believe it makes lots of sense.

donquixote’s picture

Here we go.

j0rd’s picture

Component: PHP code » layout.css

Before this patch, _zen_path (on my VM, slow disk access) in XHProf took the most amount of time of any function call to execute. This was due to around 3000-6000 calls to is_dir. With this patch, is_dir is called twice upon them being primed in the database.

Before patch, load time for primed homepage ~2.5 seconds. Calls to is_dir 1,600 taking 600 ms.

After patch, load time for primed homepage ~1.5 seconds. Calls to is_dir 12, taking 24 ms.

catch's core patch does roughly the same thing as this patch. http://drupal.org/node/341140#comment-4300406

I can't imagine Drupal core changing anything anytime soon, so while we wait, I'd recommend we implement this.

j0rd’s picture

Component: layout.css » PHP/JS code
Status: Needs review » Reviewed & tested by the community

Not sure why tag changed, resetting. Also changing status to reviewed & tested.

johnalbin’s picture

Status: Reviewed & tested by the community » Fixed

I don't know why the $path was renamed to $_path, so I changed it back. Otherwise it's fine. :-)

donquixote’s picture

Stupid habit I once had, naming all static variables with a leading underscore.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.