Skin 6.x.1.1 installed and enabled.
Configuration:
- Save a copy of optimized CSS and JS files: enabled.
- No skin paths defined

When an anonymous user accesses a non-aliased cached page (for example, http://www.example.com/node/192) the following error is displayed:

Fatal error: Call to undefined function drupal_match_path() in /home/devplp5/public_html/d6/sites/all/modules/skin/skin.module on line 53

http://api.drupal.org/api/function/hook_exit provides a clue:

Only use this hook if your code must run even for cached page views. If you have code which must run once on all non cached pages, use hook_init instead. Thats the usual case. If you implement this hook and see an error like 'Call to undefined function', it is likely that you are depending on the presence of a module which has not been loaded yet. It is not loaded because Drupal is still in bootstrap mode.

I have not seen this problem with aliased pages, nor have I seen the problem when logged into the site. If you clear all caches, then visit a non-aliased page as an anonymous user, the error will not appear. But as soon as you refresh your browser the error appears because the page has now been cached on the server.

Working under the presumption that the code in hook_exit() cannot be moved to hook_init(), I looked further into the failure -- path.inc suggests the following:

* These functions are not loaded for cached pages, but modules that need
* to use them in hook_init() or hook exit() can make them available, by
* executing "drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH);".

I inserted drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH); as the first line in both hook_init() and hook_exit() -- problem resolved.

However, there's no sense in calling drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH); unless there are actually some paths defined for skinning, so I wrapped the contents of hook_init() and hook_exit() inside of a conditional statement.

/**
 * Implementation of hook_init().
 */
function skin_init() {
  if (variable_get('skin_paths', '')) {
    drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH);
    $skin = drupal_match_path(drupal_get_path_alias($_GET['q']), variable_get('skin_paths', ''));
  
    // If viewing a "skin" page, capture the output so we can manipulate it in
    // skin_exit().
    if ($skin) {
      ob_start();
    }
  }
}

/**
 * Implementation of hook_exit().
 */
function skin_exit() {
  if (variable_get('skin_paths', '')) {
    drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH);
    $skin = drupal_match_path(drupal_get_path_alias($_GET['q']), variable_get('skin_paths', ''));
  
    // On skin page; grab the output and manipulate it.
    if ($skin) {
      print preg_replace_callback('/"\/([^"]+)?"/', '_skin_replace_callback', ob_get_clean());
    }
  }
}

I would have provided a patch but this module's files on CVS have Windows line endings (\r\n) instead of Unix line endings (\n). The patch file created by my IDE replaces every line in the module. Please refer to http://drupal.org/coding-standards for guidelines on line endings.

Comments

joelstein’s picture

joelstein commented
Status: Active » Fixed

Good catch! Of course, we could simply re-implement drupal_match_path() in the skin module. If somebody has caching enabled, then they probably want the speed benefits of not having to load the remaining bootstrap phases. Something to think about.

Still, since I agree that this is critical, let's try to make this work without re-implementing a function.

I took a slightly different approach than what you posted. With your code, if I have a page like "node/2" in my skin settings, and page caching is enabled, then the first time I visit that page, it shows me the full URLs in the source code (Skin worked its magic). But not the second time (that is, when the actual cached page was served up).

So, I dug around in the drupal_bootstrap() code, and came up with something which works every time. It uses hook_boot() instead of hook_init(), since hook_boot() is called even for cached pages (except in aggressive mode). It loads the DRUPAL_BOOTSTRAP_PATH phase, which allows us to see if the current path is skinnable. Then, if so, it loads the DRUPAL_BOOTSTRAP_FULL phase, so that the file functions in _skin_replace_callback() are available when we need them.

/**
 * Implementation of hook_boot().
 */
function skin_boot() {
  // Bootstrap the "path" phase, so we can match the path (because if we're
  // viewing a cached page, these functions won't have been defined yet).
  drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH);

  // Viewing a "skin" page.
  if (drupal_match_path(drupal_get_path_alias($_GET['q']), variable_get('skin_paths', ''))) {
    // Bootstrap the rest of Drupal, so we have access to the "file_" functions
    // in _skin_replace_callback.
    drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
    // Capture the output so we can manipulate it in skin_exit().
    ob_start();
  }
}

/**
 * Implementation of hook_exit().
 */
function skin_exit() {
  // On skin page; grab the output and manipulate it.
  if (drupal_match_path(drupal_get_path_alias($_GET['q']), variable_get('skin_paths', ''))) {
    print preg_replace_callback('/"\/([^"]+)?"/', '_skin_replace_callback', ob_get_clean());
  }
}

It's a bummer that we have to do it this way (that is, loading the full bootstrap)... but for skin pages, I think it's okay. They're not meant to be highly trafficked pages anyway... just a wrapper for third-party tools.

I've committed these changes in http://drupal.org/cvs?commit=443194.

toomanypets’s picture

toomanypets commented

One more minor tweak. I keep the skin module enabled when I'm not using it, but I remove all paths. So could we wrap hook_boot() in a conditional so that it doesn't do anything if no paths are defined?

function skin_boot() {
  if (variable_get('skin_paths', '')) {
    // Bootstrap the "path" phase, so we can match the path (because if we're
    // viewing a cached page, these functions won't have been defined yet).
    drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH);

    // Viewing a "skin" page.
    if (drupal_match_path(drupal_get_path_alias($_GET['q']), variable_get('skin_paths', ''))) {
      // Bootstrap the rest of Drupal, so we have access to the "file_" functions
      // in _skin_replace_callback.
      drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
      // Capture the output so we can manipulate it in skin_exit().
      ob_start();
    }
  }
}
joelstein’s picture

joelstein commented

In that case, it would be easier if you simply disable (but don't uninstall) the module. Then your setting would be saved, and you wouldn't have to add the paths back when you re-enabled it. It you still want this, though, feel free to open a new ticket.

Status: Fixed » Closed (fixed)

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