Either I misunderstand something or this is a reasonably serious bug in the D6 theme registry.

Attached are a tiny module (demomodule) and a slightly hacked stark theme to demonstrate it. As provided here, they work. When you copy the .tpl.php into the theme, however, the template_preprocess no longer runs, as I'll explain below.

The setup: simple module, with a hook_theme that points to an external inc and also provides for a tpl.php. The very simple theme just outputs theme('sometext') instead of the node contents.

This functions fine if you don't copy the tpl.php into the theme. Once you do that, the template_preprocess never runs *except* immediately after a cache clear.

What seems to be going on here is that _theme_build_registry() goes out and gets all the information from the modules' hook_theme() successfully. But a little lower in _theme_build_registry() it calls _theme_process_registry() on phptemplate. Phptemplate finds the .tpl.php, builds a different cache entry (this time without the ['file'] entry, which overwrites the one provided by the module, so we no longer have a ['file'] entry in the theme registry. As a result, the external file (demomodule.templates.inc in this case) does not get loaded, its template_preprocess_xxx does not get run, and we have #fail. Except when the theme registry is rebuilt, in which case the file is loaded for one call :-)

The very strange result of this bug is that you get content in a block or whatever ONLY on the call where you rebuild the theme registry (which is a call that happens to load the external inc file)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

merlinofchaos’s picture

Status: Active » Needs review
FileSize
4.85 KB

Here's an initial patch that I think will address this issue. There's also a related issue where paths don't get carried forward through patterns as well.

This patch creates an 'include_files' array that is calculated and carried forward during the registration so it always exists. This means the API in hook_theme() doesn't change at all. I did have to do a little bit of minor extra work to make sure that modifications in hook_theme_registry_alter() do not get confused.

rfay’s picture

Status: Needs review » Needs work

Thanks for the amazingly fast work on this.

This patch does not resolve the described symptom, though. It can easily be demonstrated with the attached module and theme; you do have to copy the tpl.php from the module into the theme to demonstrate it.

rfay’s picture

Here's a patch, actually from merlinofchaos, that does in fact resolve the issue in testing.

rfay’s picture

Status: Needs work » Needs review
Crell’s picture

Magnifique! I tracked this problem down as well a few months ago but never figured out a good patch for it. I just marked #489254: theme file not getting included as a dupe of this issue.

Crell’s picture

I can confirm that this works and fixes the problem in Drupal 6. I've been bitten by this a couple of times in my views plugins. Of course, we need a Drupal 7 version of it as well before we can commit it.

mattyoung’s picture

.

rfay’s picture

FileSize
404 bytes
646 bytes

I studied the patch and it looks good to me. Before this bug I didn't know what a theme registry was, though :-) So it needs more eyes.

I did check it for D7, however, and D7 does not show this bug. It's possible that we'll want to add some of merlinofchaos's finesse to the D7 theme registry, but it probably doesn't need to be done for this bug.

Attached are a D7 module and D7 theme to test with, if you're interested. There is a tpl.php in the module which can be copied into the theme to replicate the D6 scenario.

sun’s picture

rfay’s picture

Priority: Normal » Critical

I'm changing this to critical, because it represents a breach of behavior promised to the themer. I hope we can get this into the next release.

TBarregren’s picture

subscribing

swellbow’s picture

mattyoung’s picture

I always thought this is the way it's supposed to work for efficiency reason: no hook template, don't search there.

merlinofchaos’s picture

That's a different bug, Matt. This bug actually loses included files that need to be retained.

rfay’s picture

FileSize
27.98 KB

I believe we have introduced a new issue here, and a different file is not being included when it should.

Test case:

1. Create a trivial panels page to replace /user
2. Go to the page
3. Click the 'edit panel' tab

Result:

warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'theme_panels_common_content_list' was given in /home/rfay/workspace/d6git/includes/theme.inc on line 656.

I did debug through the theme registry build, and found that $registry['panels_common_content_list'] was properly created and seemed to have the correct values.

The screenshot is attached.

rfay’s picture

Status: Needs review » Needs work
Michelle’s picture

I can confirm #15. Don't have a fix but had the same error when I tried the patch.

Michelle

sp3boy’s picture

I think I have experienced problems with this, namely that when running iCal exports from a Calendar module view, the presence of a xyz_preprocess_views_view_field() function in my own xyz theme's template.php was inhibiting the loading of sites/all/modules/views/theme/theme.inc which meant template_preprocess_views_view_field() was not in memory which meant I got no output. Other view displays gave the expected output, I guess because they invoked a theme hook that was not used in my theme which triggered the loading of the views theme.inc correctly.

As I'll never get back the 15 hours it's taken me to work out what was going on, I'll give the patch a try as soon as I can.

sp3boy’s picture

I've installed Panels (a first for me) and reproduced the error in #15.

I think the problem is that in this new version of the code in _theme_process_registry()...

      // If a path is set in the info, use what was set. Otherwise use the
      // default path. This is mostly so system.module can declare theme
      // functions on behalf of core .include files.
      // All files are included to be safe. Conditionally included
      // files can prevent them from getting registered.
      if (isset($info['file']) && !isset($info['path'])) {
        // First, check to see if the fully qualified file exists.
        $filename = './'. $info['file'];
        if (file_exists($filename)) {
          require_once $filename;
          $result[$hook]['include files'][] = $filename;
        }
        else {
          $filename = './'. $path .'/'. $info['file'];
          if (file_exists($filename)) {
            require_once $filename;
            $result[$hook]['include files'][] = $filename;
          }
        }
      }
      elseif (isset($info['file']) && isset($info['path'])) {
        $filename = './'. $info['path'] .'/'. $info['file'];
        if (file_exists($filename)) {
          require_once $filename;
          $result[$hook]['include files'][] = $filename;
        }
      }

...for $filename = './'. $info['file']; with the Panels theme hook panels_common_content_list, the $filename value is "./includes/common.inc" which just happens to match the D6 core file mysite/includes/common.inc

So that gets passed to the require_once instead of the intended file mysites/sites/all/modules/panels/includes/common.inc and also gets stored in the $result[$hook]['include files']

Hopefully if the first two "recipes" for $filename are reversed, to check the module path as the priority, the correct include file will get picked up?

     if (isset($info['file']) && !isset($info['path'])) {
        // First, check to see if the fully qualified file exists.
        $filename = './'. $path .'/'. $info['file'];
        if (file_exists($filename)) {
          require_once $filename;
          $result[$hook]['include files'][] = $filename;
        }
        else {
          $filename = './'. $info['file'];
          if (file_exists($filename)) {
            require_once $filename;
            $result[$hook]['include files'][] = $filename;
          }
        }
      }
      elseif (isset($info['file']) && isset($info['path'])) {
        $filename = './'. $info['path'] .'/'. $info['file'];
        if (file_exists($filename)) {
          require_once $filename;
          $result[$hook]['include files'][] = $filename;
        }
      }

I will try testing this notion tomorrow, comments are welcome.

sp3boy’s picture

A test of the revised patch appears to work - I don't get the Panels error, I do get the correct output from the Calendar iCal view.

I attached the new patch for the convenience of anyone reading this issue for the first time.

pbuyle’s picture

When using this patch wtih devel_themer, I get many include errors like the following
warning: include_once(user.pages.inc) [function.include-once]: failed to open stream: No such file or directory in DRUPAL_BASE/sites/all/modules/devel/devel_themer.module on line 380.
warning: include_once() [function.include]: Failed opening 'user.pages.inc' for inclusion (include_path='.:/usr/share/php:/usr/share/pear') in DRUPAL_BASE/sites/all/modules/devel/devel_themer.module on line 380.

sp3boy’s picture

I get the same problem with devel_themer. Looks like devel has a clone of theme() which will need changes to match the changes to _theme_process_registry().

I will try to figure them out tomorrow as I use devel_themer a fair bit myself.

sp3boy’s picture

FileSize
1.43 KB

I know that a separate issue will need to be raised against Devel module for this, but here is a patch that hopefully resolves the knock-on problem with Devel.

I've tested it with Devel and Panels installed (and Calendar which is where my problems began in #446614: iCal empty.

Additional testing would be appreciated if anyone has the time. Just to confirm, this latest patch needs to be applied to the Devel module as well as the patch in comment #20 to test its effectiveness.

Any suggestions on what the protocol is for co-ordinating a contributed module change with a Drupal core change would also be welcome.

sp3boy’s picture

Status: Needs work » Needs review

Sorry, meant to change status.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the outstanding work on this, sp3boy.

IMO, #20 is RTBC. merlinofchaos, please give your stamp of approval.

Here is the difference between merlinofchaos's patch in #3 (posted by me, but from him) and sp3boy's patch in #20.

--- includes/theme.inc
+++ includes/theme.inc
@@ -286,13 +286,13 @@ function _theme_process_registry(&$cache, $name, $type, $theme, $path) {
       // files can prevent them from getting registered.
       if (isset($info['file']) && !isset($info['path'])) {
         // First, check to see if the fully qualified file exists.
-        $filename = './'. $info['file'];
+        $filename = './'. $path .'/'. $info['file'];
         if (file_exists($filename)) {
           require_once $filename;
           $result[$hook]['include files'][] = $filename;
         }
         else {
-          $filename = './'. $path .'/'. $info['file'];
+          $filename = './'. $info['file'];
           if (file_exists($filename)) {
             require_once $filename;
             $result[$hook]['include files'][] = $filename;
merlinofchaos’s picture

Looks good to me.

tomgf’s picture

The patch provided at #20 solved a problem that was driving me insane: themed views were not visible unless that the theme registry was rebuild on every page.

What capture my attention was that –as mentioned at the beginning: “the template_preprocess never runs *except* immediately after a cache clear”.

Thank you and I hope to see these changes on a future Drupal release (this was not done in 6.15).

neclimdul’s picture

Version: 6.14 » 6.x-dev

Hope we can get this committed before the next release. I'd really like to use it in some of my modules and it's one of the few remaining wtf's in the d6 theme system. Moving it to -dev so maybe it'll grab some attention.

rfay’s picture

I emailed Gabor asking him to take a look at it.

merlinofchaos’s picture

For now I've been telling people to use workarounds like this:

/**
 * Drupal has a bug that causes theme file includes to get lost. However,
 * it is really convenient to have preprocess functions separated out
 * so that they 1) don't load code when unnecessary and 2) are easy to
 * find for non-developers.
 *
 * Until this bug http://drupal.org/node/591804 is fixed, we must have
 * this include.
 */
function panels_everywhere_init() {
  ctools_include('theme', 'panels_everywhere', 'theme');
}

But it would be nice to not have that.

rfay’s picture

Just to be clear in the long train of issues here:

#20 is the RTBC patch.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed to Drupal 6.

reubenavery’s picture

jeezus this about ruined my day. patch in #3 seems to have corrected this. (keeping fingers crossed)

coming to this issue by way of views_attach #518128: Views attach don't display when theme overrides default templates.

sp3boy’s picture

The patch in #20 is what was committed I believe.

tim.plunkett’s picture

Can anyone here take a look at #732420: Problem with Theme developer after upgrade to Drupal 6.16? Specifically the issue itself, and comments 8 & 9?

EDIT: Thanks to rfay and sp3boy. sp3boy, I rerolled your patch, and it works like a charm.

Status: Fixed » Closed (fixed)

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

andypost’s picture