Why are variables being unset in the preprocess functions

hadsie - July 23, 2009 - 02:32
Project:RSVP
Version:6.x-2.x-dev
Component:Code
Category:feature request
Priority:normal
Assigned:ulf1
Status:closed
Description

I was just wondering what the purpose of unsetting some of the variables in the template_preprocess functions is. For example, in template_preprocess_rsvp_invitation you run:

unset($variables['rsvp']);
unset($variables['invite_target']);
unset($variables['connection']);
unset($variables['params']);

at the tail end of that function. But I'd like to do some more advanced processing with the rsvp object in my template. I can't really see any benefit in unsetting these except to restrict what can be done in the tpl file. I guess I'd ask that these don't be unset unless there is something I'm missing here.

Thanks!
Scott

#1

ulf1 - July 23, 2009 - 19:14

The purpose of the preprocess functions is to prepare all variables in such a way, so that a web designer can use them later in the output file without the fear of introducing unwanted security risks.

You are welcome to remove the unset calls in your installation if you want to have more control in your templates.
But I personally would favor to expose the variables that you are missing in the preprocess functions like I did with the existing variables.

~
Ulf

#2

hadsie - July 25, 2009 - 17:58

In my case the main thing I needed to do differently was the way the image was outputted. I wanted to run it through imagecache. The image variable that's available has already had theme_image run on it.

Basically what I've done is add another preprocessor in my template.php to override the already setup image:

function phptemplate_preprocess_rsvp_invitation(&$variables) {
$node = $variables['node'];
$rsvp = rsvp_function_load_rsvp($variables['invitation_rid']);

$image = rsvp_function_getImage($rsvp->image);
$image = theme('imagecache', 'Medium', $image, $node->title, $node->title);
$variables['image'] = $image;
}

But it seems redundant to have to re-load the rsvp object like this when it simply could have been left in the template at the original preprocess call in rsvp.module.

The views module leaves a $view variable in some of their theme templates, so I'm still not sure what the benefit of unsetting the rsvp object is in this case. What are the security risks by leaving this in?

#3

ulf1 - July 28, 2009 - 17:37

There are a broad range of discussions about cross site scripting possibilities when you output variables the wrong way.
Example: http://drupal.org/node/28984

But besides that, don't you process the image now twice. Once in your preprocessor function and one in the build-in into rsvp? Probably not really a good solution because the build-in version still uses the "uncached" version?

I have no experience with the 'imagecache' theme. Is this part of a module or a theme that comes with drupal itself? If the images are the only issue you have the better solution is probably to code rsvp in a way that it uses the imagecache option if it is available.

Thanks,
Ulf

#4

hadsie - July 29, 2009 - 06:26

This is the imagecache module, but this is really besides the point: http://drupal.org/project/imagecache

I don't understand how leaving the $rsvp variable set inherently causes XSS issues. Of course XSS issues /could/ arise if someone doesn't use proper care when writing code, but limiting the developer in this way isn't going to prevent that.

With the image example here there's a number of different things that a developer might want to do with the output, perhaps the better way to handle this would be in the template file itself instead of in the preprocess function which can't be overridden without breaking the upgrade path for the module.

What I'm suggesting is to just pass the unprocessed image path to the template and then run theme('image' ...) in the template file instead of in the preprocess function.

When you suggest: "... the better solution is probably to code rsvp in a way that it uses the imagecache option ..." I'm not sure if you're suggesting I submit a patch for this or just change it on my local copy (which would break my upgrade path for the module)? I'd be happy to submit a patch, but I don't think it would be beneficial to add support just for imagecache, it just needs to be more flexible.

#5

ulf1 - July 29, 2009 - 15:58
Assigned to:Anonymous» ulf1
Status:active» fixed

"Of course XSS issues /could/ arise if someone doesn't use proper care when writing code, but limiting the developer in this way isn't going to prevent that."

In my eyes, the main purpose of having the preprocess function is exactly to prevent this from happen. Otherwise for the most cases it would be sufficient to pass the input parameter straight to the template. But then the person writing the template has to deal with formating the variables in the correct way.

On the other hand I agree that unsetting the input parameters in the preprocess function prevent you from using them in your derived preprocess function.

I now removed the unset code. The changes will be available with the next dev version.

Thanks,
Ulf

#6

hadsie - July 30, 2009 - 03:39

Thanks Ulf!

Do you think it would be beneficial to move the theme('image'...) call into the template as well to give more flexibility to the themer?

Cheers,
Scott

#7

ulf1 - July 30, 2009 - 15:54

Hi Scott,

As I said earlier, I would rather keep all variables that a web designer can work with as simple as possible. When I move the theme('image'...) part into the template I would work against my own believes and then the question rises where do I start and where do I end.

So I would rather keep it as it is now, and offer public variables that a web designer can use out of the box.
And If a developer likes to implement his own preprocess function he can do that by using the original $rsvp variable. Even if it is not 100% efficient.

Thanks,
Ulf

#8

hadsie - July 31, 2009 - 03:10

Ok thanks Ulf.

- Scott

#9

System Message - August 14, 2009 - 03:20
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.