Preprocess function overrides $variables['file'] [#297952]

Comment	File	Size	Author
#16	preprocess-rename-file-to-template_file-D7.patch	2 KB	swentel
#13	preprocess-rename-file-to-template_file.d7.patch	2 KB	swentel
#12	preprocess-rename-file-to-template_file.d7.patch	1.59 KB	swentel
#7	preprocess-rename-file-to-template_file.d6-4.patch	1.4 KB	aaron
#7	preprocess-rename-file-to-template_file.d7.patch	1.39 KB	aaron

Comment #1

dvessel commented 22 August 2008 at 15:02

Status:

Active

» Closed (works as designed)

Hi Aaron. :)

What your experiencing existed for a long time. It has nothing to do with the new theme system. This happens because the scope of the function to render the templates is the same as the template file. You would see the same thing from Drupal 4.7 or maybe even earlier.

So, I don't see how it's well established. Or I'm misunderstanding you.

http://api.drupal.org/api/function/_phptemplate_render/5
http://api.drupal.org/api/function/theme_render_template/6

Log in or register to post comments

Comment #2

aaron commented 23 August 2008 at 13:56

Status:

Closed (works as designed)

» Active

Opening this for more discussion, after posting the following to the developer's list:

I raised an issue at #297951: Preprocess overrides $file originally for the Filefield module, and then (at this issue) for the Drupal core theme preprocess hook architecture. The second issue was marked 'by design', but I believe the design is flawed, and that this needs to be addressed.

As the preprocess theme hook has only been opened for developers in Drupal 6, this wasn't much of an issue before, and the flaw has only now become apparent. Without any documentation I can find, the $file variable is reserved, meaning that theme developers may not expose a variable named $file to a template file when inserting a preprocess hook, as outlined at http://drupal.org/node/223430.

The $file variable is well established in Drupal, and often used in contributed module theme functions. Now that it's relatively easy for themers to override theme files with their own template files, this issue will begin to crop up.

The current work-around is that if the theme function you wish to override uses a $file variable, then you have to copy that variable in your preprocess hook to another variable, such as $original_file. This is unwieldy, unfortunate, and currently undocumented (as far as I can tell).

I have a feeling we'll see this problem become more prevalent as developers move to Drupal 6. In core, $file appears only 22 times, and not in any core theme functions (that I'm aware of). However, that is misleading, as core Drupal currently does a poor to middling job of file handling (which will hopefully be remedied soon).

In the contributed DRUPAL-6--1/modules branch, $file appears 265 times, and 1186 times in the contributions/modules branch. There are several contributed modules using $file in theme functions, such as the aforementioned Filefield, Imagefield, Media Mover, and File Framework. (I'm sure there are others, those are just from a quick grep). And I suspect that at the very least, Imagefield and Filefield will warrant relatively frequent theme overrides for individual sites.

Assuming hook_file (#142995: Add hook_file and make files into a 1st class Drupal object) makes it into Drupal 7, I suspect we'll see even more use of the $file variable in themes, particularly as more developers jump into file asset management.

Here are some options available to us. There are probably other options as well:
1) Do nothing. Let themers struggle when they try to override a theme $file variable using a preprocess hook.
2) Document the flaw, so at least developers know the difficulties involved in doing this.
3) Declare $file a restricted variable, forbidden in theme functions.
4) Encourage developers to find all instances of $file in contributed module themes and rename them.
5) Use another magic variable name for use in theme_render_template, such as $file__ or $template_file, and declare it restricted.
6) ???

I believe we are doing a disservice to developers if we tell them to use a flawed preprocess system. I strongly suggest we implement option 5 before this problem becomes more severe, ideally for Drupal 6, as many developers are migrating there as we speak.

Log in or register to post comments

Comment #3

merlinofchaos commented 23 August 2008 at 17:20

It took me a minute to understand the nature of the problem here, as Aaron didn't actually explain why $file is restricted. At first I'd thought it was restricted for the same reason that $id or $zebra is -- that some preprocess extracts it and uses it. But actually, it's this:

function theme_render_template($file, $variables) {
  extract($variables, EXTR_SKIP);  // Extract the variables to a local namespace
  ob_start();                      // Start output buffering
  include "./$file";               // Include the file
  $contents = ob_get_contents();   // Get the contents of the buffer
  ob_end_clean();                  // End buffering and discard
  return $contents;                // Return the contents
}

Since all local variables are available to the template when it's included, and $file is passed in as a local variable.

This is pretty easily fixed:

function theme_render_template($template_file, $variables) {
  extract($variables, EXTR_SKIP);  // Extract the variables to a local namespace
  ob_start();                      // Start output buffering
  include "./$template_file";        // Include the file
  $contents = ob_get_contents();   // Get the contents of the buffer
  ob_end_clean();                  // End buffering and discard
  return $contents;                // Return the contents
}

I see no reason not to make this change.

Log in or register to post comments

Comment #4

merlinofchaos commented 23 August 2008 at 17:23

BTW I'm not really in favor of $file__. $template_file strikes me as both accurate and sufficient. As a bonus, I think $template_file is already the file to include in $variables anyway, so there's not too much exciting that we're overwriting there.

Log in or register to post comments

Comment #5

dvessel commented 24 August 2008 at 00:53

Yeah, I didn't understand why $file needed to be used by a module dev. I still don't actually but it's an easy fix.

Log in or register to post comments

Comment #6

merlinofchaos commented 24 August 2008 at 06:04

Well if you're theming an image or something, 'file' is a pretty appropriate variable for the filename. For example.

Log in or register to post comments

Comment #7

aaron commented 25 August 2008 at 02:19

Status	File	Size
new	preprocess-rename-file-to-template_file.d7.patch	1.39 KB
new	preprocess-rename-file-to-template_file.d6-4.patch	1.4 KB

Thanks for the fix! Works as advertised.

There are only 10 modules in HEAD using a $template_file variable, and none in a theme function declaration (although Theme Editor comes dangerously close with $template_files).

I've added some documentation, and we can document it as well in the handbook page so that developers know not to use that variable in future theme functions.

Attached are patches for d6.4 and d7 (current HEAD).