file includes for theming hooks fail with theme overrides.

I too have just noticed this issue. I however do not think this is a bug, but more by design.

I originally had my template preprocess functions in a separate file, which I had included by setting the 'file' and 'path' properties in my modules hook_theme. I got the idea to do this from the hook_theme documentation of the file property here.

The current implementation would allow the theme to set the 'file' and 'path' properties which allow's the theme to break all its stuff out into separate files. This of course, then breaks the module includes, which might be needed to ensure template preprocess functions are run.

There are several solutions and workarounds, and I'm not sure which one would be best, but at the very least, the documentation for hook_theme should be updated to indicate that if a module puts its template_preprocess_HOOK functions in a separate file, that the file will not be included if a theme overrides the template, and that the module will need to load the file itself.

Solutions could include:

• Making the file property in the registry an array, and loading up each include. Obviously the path property would probably need to be the same.
• Have modules ensure that any template preprocess hooks are loaded up. This seems to defeat the purpose of having the file and path properties, at least for modules.
• Have themes that override these templates, make sure that they include the needed module preprocess functions.

None of those options seem ideal. At the very least, this needs to be documented somewhere.
This is only a problem when include files contain the preprocess functions, normal theme function overrides will not cause this problem.

I ran into this issue today and it was difficult to debug. I think cdale's first solution is best, because developers should be able to expect the system to know and load any files that are required by a themeing function.

For example, if you have the following theme hook defined by your module:

'example_theme_hook' => array('file' => 'theme.inc', 'path' => $path . '/theme/', 'template' => 'example-theme-element', 'arguments' => array('arg1' => ''));

the default preprocess function is template_preprocess_example_theme_hook(), if your theme defines its own preprocess function mytheme_preprocess_example_theme_hook(), the default processing function will not be defined when the theme preprocess function is called becasue theme/theme.inc will not be loaded. But if the default preprocessing function *is defined* it will be called by drupal.

I stepped through _theme_build_registry and noticed that the file and path keys for example_theme_hook are lost when the registry is rebuilt. This happens after the call for _theme_process_registry for 'theme_engine'.

I think the module's path and file key should be stored in the theme registry. Maybe as a new key called ['require_files'] which would hold an array of all files which must be included when a theme hook is called.

I believe I have a rather clunky work-around for this apparent bug.

My situation:

• A custom module that declares a theme function, 'mythemefunction', in its implementation of hook_theme().
• A custom preprocess function, 'template_preprocess_mythemefunction', in its implementation of hook_theme(), and stored in a .theme file referenced in the 'file' key of the array.
• A custom template called 'mytemplate.tpl.php' in the module folder, called by the 'template' key in the array returned in hook_theme().
• A theme where the template from the module folder, mytemplate.tpl.php, has been copied directly into the theme folder.

Here's a copy of the theme function:

function mymodule_theme() {
  return array(
    'mythemefunction' => array(
      'arguments'   => array(
        'node'          => NULL,
        'num_items'     => 3,
        'latest'        => TRUE,
        'vocabs'        => NULL,
        'content_types' => NULL,
        'multilingual'  => NULL,
      ),
      'file'        => 'mymodule.theme',
      'template'    => 'mytemplate',
    ),
  );
}

When clearing the cache, the first page load uses mytemplate.tpl.php in the theme folder, but any subsequent page loads use mytemplate.tpl.php in the module folder.

Initially, I thought I could introduce a preprocess function into my theme, mytheme_preprocess_mythemefunction, which would get called after the one in the module, and I could use $vars['template_files'] to add a new template suggestion. This did not work!

I noticed that $vars['directory'] was being set to the path of the module, so that would be where Drupal was looking for the mytemplate.tpl.php file. I wrote my theme's preprocess function as:

function mytheme_preprocess_mythemefunction(&$vars) {
  $vars['directory'] = path_to_theme();
}

This is when I noticed that path_to_theme() returned the path to the module, not the theme at all! This is very odd, and not at all what I would expect in this situation. I am now using:

function mytheme_preprocess_mythemefunction(&$vars) {
  $vars['directory'] = drupal_get_path('theme', 'mytheme');
}

The only problem with this approach is that the module's preprocess function doesn't get called at all now (probably because of the change to the $vars['directory'], so I had to copy everything out of that preprocess function into the one in my theme.

I hope this helps some bright person figure out what's going on here and perhaps suggest a fix for Drupal for this.

subscribing

Here small module to catch this bug

Follow this steps:
1) put module at your modules folder
2) enable module (menu
3) navigate to new menu "Post" url http://example.com/andy - it shows Name: {your user name}
3.1) save variables that dumped to screen to compare them later

4) copy andy-page.tpl.php to your theme folder and rebuild theme-registry (for example submit admin/build/themes)
5) revisit "Post" menu - there's no name
5.1) Compare var dump with 3.1

if you look at variables theres no 'name' key

After digging into code... theme.inc

_theme_build_registry()
Is loosing "file" key when template override is found in theme folder

  // And then the same thing, but for the theme.
  if ($theme_engine) {
    _theme_process_registry($cache, $theme_engine, 'theme_engine', $theme->name, dirname($theme->filename));
  }

This bug leads to including all preprocess functions to be included in .module file!

Suppose 'file' key of theme registry should be processed different then now
'arguments' already copies from cache but 'file' should be inside folder

_theme_process_registry()
...
      // If 'arguments' have been defined previously, carry them forward.
      // This should happen if a theme overrides a Drupal defined theme
      // function, for example.
      if (!isset($info['arguments']) && isset($cache[$hook])) {
        $result[$hook]['arguments'] = $cache[$hook]['arguments'];
      }
...

Tested d-7 version - it's not affected because all files described in .info of module so core knows what file to include

I finally had a chance to look into this. Or maybe it just bugged me enough. :)

I've attached a patch that I think resolves this issue. Not sure if it's the best solution, but I think it works quite well, and does not break any existing functionality.

@cdale thanks! it works!

I've known about this potential problem but never encountered a real error which baffled me. Lucky I guess.

IIRC, this happens because of the two find functions. drupal_find_theme_templates & drupal_find_theme_functions. When phptemplate invokes them to auto discover the overrides, they reset a lot of the elements when it should not. This affects theme function overrides also.

The problem should be tackled there.

This must be fixed in 7 first.

@dvessel - The issue can not be tackled there as it is quite conceivable that themes and theme engines would want to break the hooks out into different files and paths if they wanted, or needed to. i.e. if they need to override a large amount of theme hooks. Therefore, it is quite correct for drupal_find_theme_templates & drupal_find_theme_functions to override those properties.

The issue described here is only a problem for theme templates, not theme functions. When a themer overrides a theme function, they fully expect to have to replicate all of the functions functionality which is quite correct. However, when overriding a template, the default template_preprocess_HOOK is supposed to run all the time regardless, unless the overriding theme sets the 'override preprocess functions' property. Not to mention the possibility of having other modules installed which might also run a MODULE_template_preeprocess_HOOK which would expect certain properties to be set that would not be because the default template preprocessor did not run.

My patch fixes this by making sure the default template_preprocess_HOOK gets loaded, as is specified in the documentation of hook_theme(). The path and file properties may be used by the theme to include its own preprocess hook from a different file, but it would most likely still need (and expect) the default to run.

As has already been mentioned in this issue, this is not a problem in D7, because of the code registry.

Feel free to correct me if you think I've missed the mark, but I do not think that I have.

The defined file setting was never meant to be used by themes. Merlinofchaos would know for sure but I'm 99% certain that he intended its use for modules. Modules tend to be more complex and there was a push to split modules into more manageable chunks. Setting that file is to ensure all the theming functions are available.

I've been tempted to use that attribute to include files from my themes but didn't. It wasn't designed for it.

Now having it as an array so themes can use it would be awesome. Something to look into in a separate issue.

For theme functions, it doesn't necessarily always have to be for *only* the theme function. There could be dependent functions which it invokes. So, it should never be lost there either. It is not only about preprocess functions and theme functions. It's all the theme *related* functions so your implementation of looking for the specific preprocess function is too narrow.

I can see the problem with drupal_find_theme_templates(), but I don't quite understand what is wrong with drupal_find_theme_functions(). I do acknowledge that your point is well made though. Will this still be an issue with the code registry though? I notice in theme() for drupal 7, everything now uses drupal_function_exists, which seems to render the file and path properties pretty useless. But perhaps that is for another issue.

Do you have any ideas how this could be properly fixed? I think I need to dig into that section of the code a bit more.

I just had a thought... Does the code registry pass theme files too? Or just module registered files?

The fix might be easy. First we need to know what should be carried over. There might be some things we don't want to carry over but I'm not sure ATM.

In drupal_find_theme_templates, near the middle, it could be something like this.

if (isset($cache[$hook])) {
  $templates[$hook]['template'] = $template;
  $templates[$hook]['path'] = dirname($file->filename);
}

And below that where the pattern matches are done has to be fixed too.

But the "path" might be a problem since it might direct the "file" to the wrong path. I'd have to step through the code to know for sure. It's all pulled together within _theme_process_registry.

RE:#14 I'm not completely familiar with the code registry so what I said might not apply for 7. We need more feedback.

I have slightly lost track of this thread as it has become more and more technical, but my issue from comment #3 above is that when a module defines a custom template for a theme function, then a theme overrides that template, the theme's override template is ignored (except immediately after a cache clear). Is this because this is not the way the system was intended to work? If so, what would be the preferred method here?

Chris, the problem your encountering is not related to this. The theming "hook" must match the template name. It's definitely a headache that needs to be fixed.

function mymodule_theme() {
  return array(
    'mythemefunction' => array( // <- the hook is different from...
      ...
      'template'    => 'mytemplate', // <- template name. Not so good.
    ),
  );
}

dvessel, thanks. That solves my problem entirely, but you're right, it either needs to be rectified or better documented, because it implies that a template file can be used regardless of its name.

So what's wrong with D7?
templates are not in code-registry?
What kind of feedback is needed?

patch #7 work fine for D6, so what to do to bring progress?

I'll create a patch for 6 and investigate further for 7. Will most likely be done by this weekend.

This should be fixed when #310467: Slimmer hook_theme() lands.

So lets revert to 6.x as commited #310467: Slimmer hook_theme()
patch from #7

This needs to be throughly tested. All it's doing is changing the attributes that change when being discovered. Everything else is simply passed along. Not sure how it will behave.

Another patch coming. There's an error for finding theme functions in there I need to fix.

As I'm digging through this I realize it's not so straight forward. Here's the problem.

The "path" and "file" are linked closely to allow the inclusion of the support files. See theme():

  // Include a file if the theme function or preprocess function is held elsewhere.
  if (!empty($info['file'])) {
    $include_file = $info['file'];
    if (isset($info['path'])) {
      $include_file = $info['path'] .'/'. $include_file;
    }
    include_once($include_file);
  }

When the theme provides an override the "path" will change pointing to the theme. When a "file" is set from a module, it will never resolve to the right location because of this change in the path. They are too closely linked.

This fix changes how the files are included. It shouldn't break any existing modules (I hope.). The "file" now merges the path with what's set for the file and pushes them into a new "files" (plural) attribute. It is processed in this way for each invocation of "HOOK_theme" and stored in the registry.

By the time theme() includes the required files, it will not look for the path since it's part of the includes file. This will also allow themes to set it's own "file" and allow dynamic inclusion. I included this because it's useful to themers (Studio theme does this for example and I always wanted to use this.). I'm pretty sure it won't break anything else.

Please review.

Trying to review

1) cant apply cleanly - done by hand
2) code-style changed to d6 (spaces and strings)
3) On both sites this patch works fine without notices

So here same patch

andypost, that patch was done with the latest in the DRUPAL-6 branch. And the code style change is how it's done in 6 onward.

@dvessel Without changing code-style patch is more readable, imo

I cant apply @dvessel patch because of line endings - when I test it on debian (everything works fine)

As this pointed out by dvessel in #465514: Using the file attribute in the theme registry, this could break the Studio theme; therefore, subscribe.

We need another review to rtbc this bug for d6

Duplicate of issue 591804.

@Darren Oh, Are you sure this fixed already in #591804: Theme registry build bug: External file with template_preprocess is not loaded if tpl.php is copied into theme

Yes. The patch failed to apply. When I tried to apply it manually, I discovered that the problem was already fixed.

Confirm that this was fixed in 6.16 http://drupal.org/node/732000