Hi,
Thanks for the awesome webform module, I used this module a lot, I just like to point out that the function theme_webform_mail_headers($form_values, $node, $sid, $cid) in the webform.module file needs a minor tweak of having the input parameters are optional.

Having a function definition of
theme_webform_mail_headers($form_values='', $node='', $sid='', $cid='')
OR
theme_webform_mail_headers($node='', $submission='', $cid='')

My thanks

Comments

quicksketch’s picture

quicksketch
he/him
commented

Could you explain why these need to be optional?

sndev’s picture

sndev commented

Hey quicksketch,

There are a few reasons why it should be, the first one is on the function definitions themselves :
2.x
function theme_webform_mail_headers($form_values, $node, $sid, $cid) {
$headers = array(
'X-Mailer' => 'Drupal Webform (PHP/'. phpversion() .')',
);
return $headers;
}
OR
3.x
function theme_webform_mail_headers($node, $submission, $cid) {
$headers = array(
'X-Mailer' => 'Drupal Webform (PHP/'. phpversion() .')',
);
return $headers;
}

These input parameters are not really needed/required in the function. Another is a situational reason to prevent errors like these:

“Error * warning:
Missing argument 4 for example_webform_mail_headers() in /home/example_site/public_html/sites/all/themes/example/template.php on line 220.

Thanks for your quick response, kudos for these good and useful module.

quicksketch’s picture

quicksketch
he/him
commented
Category: bug » support

Missing argument 4 for example_webform_mail_headers() in /home/example_site/public_html/sites/all/themes/example/template.php on line 220.

This is because you're upgrading from 2.x to 3.x is it not? If those parameters were optional and you didn't update your code, then you've actually got WRONG parameters coming into your overridden theme function. So instead of $form_values you've got $node and instead of $node you've got $submission, etc. Making these parameters optional wouldn't accomplish anything other than silently failing instead of giving you a warning.

Because these are theme functions, Webform can't make assumptions about what a user would want to use them for. It's entirely feasible that a user might want to set a header based on a property of the $node or $submission variables.

However now that I think about it, we shouldn't be passing in $cid, we should be passing in the $email variable that contains the entire e-mail configuration. It sounds like you're worried possibly about memory requirements of passing around full objects/arrays, but because of the way PHP works, passing in an entire array or object is in fact no more expensive than passing in a string (do a Google for PHP variable reference counting).

Basically, I do not think this is a valid request. API changes are a natural part of major version releases.

sndev’s picture

sndev commented

Hey quicksketch,

Your right, I was upgrading from 2.x to 3.x and yes the error was from an overridden theme function not keeping up with the api changes of a major version release to 3.x I guess a reason why it would be good to redefined the function definition of theme_webform_mail_headers to have these input parameters as optional is just to reflect the current definition of the function block that those input parameters are not really needed. But these are subjective reasons and probably not worth the effort of making the change. I leave it up to your decision as a maintainer of the webform module. Thanks for your time.

BTW thanks for that nice tidbit on passing arguments. good one.

quicksketch’s picture

quicksketch
he/him
commented
Status: Active » Fixed

I've opened a new issue for the $email variable problem I accidentally ran into in #3. #731832: Email header and templates should receive entire $email variable instead of $cid

We won't be making these parameters optional, especially since that would imply that when theming e-mails, modules wouldn't need to pass in this information, leading e-mails that did use these variables to work when the variable was passed in and break when it wasn't. This information isn't optional if the calling module is required to pass it in, which is the case here.

Status: Fixed » Closed (fixed)

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