breaks with php code - nodes
Jürgen Depicker - May 24, 2007 - 11:52
| Project: | Notify |
| Version: | 5.x-1.0 |
| Component: | Code |
| Category: | task |
| Priority: | minor |
| Assigned: | beginner |
| Status: | active |
Description
Thanks for this wonderful module. It works like a charm. Or worked ;-). I created a page with only one line of php code inside (a drupal_goto depending on user id of logged in user) and this page broke cron. It never completed. It took me a while to nail down the culprit, but I found eventually.
The code breaks during the construction of the mail body. Maybe there should be a check there somehow on the input type, to break the loop when the input type is set to php code. But I have no idea about how to do that right now... I deleted the offending node, which solved the problem.
I know that I could have anti-dated the node too, so it wouldn't show up in notifications. But both solutions are not perfect.
Any ideas anyone?
#1
What???
Do you mean that when working out the mail body, cron is actually executing the php code in the node body???
If you can confirm this, it is actually critical!
I guess the same would apply for D5.
#2
I believe the 'offending' code is in _notify_send, since it loads all nodes. I guess node_load automatically executes the php code.
(notify.module, line 337...)
// Fetch all new nodes and 'load' it to get proper body, etc.$nresult = db_query(db_rewrite_sql('SELECT n.nid FROM {node} n WHERE (n.status = 1 OR n.moderate = 1) ' .
'AND n.created > %d AND n.created <= %d ORDER BY n.created'), $period, time());
$nodes = array();
while ($node = db_fetch_object($nresult)) {
$nodes[$node->nid] = node_load($node->nid);
}
#3
Thinking a bit more about it, it is actually logical that the php code is executed. That's the intended behavior.
But I understand that in some particular cases like yours, that may create havoc.
I am sure the same problem exists in core.
What happens if, using only core, you have a node with a drupal_goto() declaration within the teaser of a node, and you view the teaser as part of a list of nodes (i.e. on the front page, or on a page like taxonomy/term/tid)?
I haven't had time to look closely at the code. I will do so, but it doesn't seem to be a notify-specific bug.
See if you can reproduce a similar behavior with core.
#4
I would argue that PHP code in nodes with side effects (goto's, database changes) are dangerous and that it is general better to use/write a module for such cases.
#5
All other stuff breaks too. For as long as the node is still on the paged query page you're viewing, the result is always the page destination of the drupal_goto.
I believe there's no way around this, it is just something which might be mentioned in the notify module's readme: 'beware with php nodes; preferably anti-date them'. This may even be mentioned elsewhere too - 'where' is the question ;-). It is actually to be expected to create havoc, one just needs to know, and I didn't at first so my site was broken for a while without me understanding why!
Since now I'm all that wiser :-), I think this is not a bug, just a side-effect one has to keep in mind.
#6
Ok, we agree that, after all, notify.module works as intended.
But this 'feature' and the unexpected consequences definitely need to be documented, within the module (in README.txt?), and in the handbook, too, since the same seemingly broken behavior may happen in core.
But I have another case scenario, probably more common: when the content is different for different user roles (e.g. anonymous and authenticated).
For example, in this page: http://www.reuniting.info/introduction/contact , there is a link to the webmaster's private message box. But this link is only accessible for registered users. Some PHP tests the user role (for anonymous:
if ($user->uid == 0)), and changes the link accordingly: anonymous users are redirected to the login page instead (I know, we ought to use the contact.module, but that's beyond the point).Now, cron.php runs as the user who launched cron. If cron.php is accessed by the server's cron tab, it will run as anonymous.
But if an admin goes to http://www.example.com/cron.php, cron will be run as if it were the admin.
All this means that the php code will be run accordingly, and it is conceivable that some information embedded in the node that was meant only for a higher role, will be sent to all users.
Remember that the admin can force the notifications to be sent by clicking the appropriate button at ?q=admin/user/user/notify . Those emails could regularly be processed with the role of an admin.
I don't think it's possible to force cron to run as an anonymous user. We would have to check that.
Meanwhile, this is another scenario to be aware of and at least document.
#7
Fortunately you are wrong - or so I think, because there's
function _notify_switch_user($uid = NULL)switching to the recipient user before anything is loaded/sent. So I believe we can rest in peace so to speak, if you take care of getting this documented somewhere?#8
Good.
A patch for README would be welcome.
Otherwise I'll take care of it.
#9
Actually, this is important enough for a mention to be added in the setting page.
We ought to make use of the hook_help() function.
#10
OK, here's a proposal, with some extra info added at the end of the help text (patch attached):
<?phpfunction notify_help($section) {
switch ($section) {
case 'admin/help#notify':
$output = '<p>'. t('The notification module allows users to subscribe to periodic e-mails which include all new or revised content and/or comments much like the daily news letters sent by some websites. Even if this feature is not configured for normal site users, it can be a useful feature for an administrator of a site to monitor content submissions and comment posts.') .'</p>';
$output .= '<p>'. t('The administrator sets the frequency of the e-mails in the notify administration interface. They can also set how many e-mail failures should occur before notify stops sending notifications. Note that cron must be enabled for notifications to be sent out.') .'</p>';
$output .= t('<p>You can</p><ul><li>set up your site to run tasks automatically at required intervals. For more information, see <a href="%admin-help-system">cron</a>.</li><li>administer notify <a href="%admin-settings-notify">administer >> settings >> notify</a>.</li></ul>', array('%admin-help-system' => url('admin/help/system'), '%admin-settings-notify' => url('admin/settings/notify')));
$output .= '<p>'. t('For more information please read the configuration and customization handbook <a href="%notify">Notify page</a>.', array('%notify' => 'http://www.drupal.org/handbook/modules/notify/')) .'</p>';
$output .= '<p><em>Warning!</em> Usage of php nodes with special commands like <code>drupal_goto()</code> will break <b>notify</b> as well as other modules (e.g. the <i>all recent posts</i> page will show only that specific php node till it \'drops\' of the page). A quick and dirty solution to this problem is to anti-date the php node by some years.</p>';
return $output;
?>
#11
Ahum... I forgot to use
t(), sorry, so patch attached using t().<?phpfunction notify_help($section) {
switch ($section) {
case 'admin/help#notify':
$output = '<p>'. t('The notification module allows users to subscribe to periodic e-mails which include all new or revised content and/or comments much like the daily news letters sent by some websites. Even if this feature is not configured for normal site users, it can be a useful feature for an administrator of a site to monitor content submissions and comment posts.') .'</p>';
$output .= '<p>'. t('The administrator sets the frequency of the e-mails in the notify administration interface. They can also set how many e-mail failures should occur before notify stops sending notifications. Note that cron must be enabled for notifications to be sent out.') .'</p>';
$output .= t('<p>You can</p><ul><li>set up your site to run tasks automatically at required intervals. For more information, see <a href="%admin-help-system">cron</a>.</li><li>administer notify <a href="%admin-settings-notify">administer >> settings >> notify</a>.</li></ul>', array('%admin-help-system' => url('admin/help/system'), '%admin-settings-notify' => url('admin/settings/notify')));
$output .= '<p>'. t('For more information please read the configuration and customization handbook <a href="%notify">Notify page</a>.', array('%notify' => 'http://www.drupal.org/handbook/modules/notify/')) .'</p>';
$output .= '<p>' . t('<em>Warning!</em> Usage of php nodes with special commands like <code>drupal_goto()</code> will break <b>notify</b> as well as other modules (e.g. the <i>all recent posts</i> page will show only that specific php node till it \'drops\' of the page). A quick and dirty solution to this problem is to anti-date the php node by some years.') . '</p>';
return $output;
?>
#12
I couldn't apply the patch. Please look at: http://drupal.org/patch/create
I am looking at it anyway, right now.
#13
I think there is a bug in core.
when the help.module is disabled, there is still the help link at ?q=admin/by-module . You follow the link to ?q=admin/help/notify you get 404. That bad design. I'll check if there is an issue for this.
Not everybody will have help.module enabled. This is serious enough to make sure that people get to see it. It has to appear on the settings page regardless.
#14
"Get help" when help module is not active : http://drupal.org/node/133092
#15
hook_help() works even when help.module is disabled, provided the help is not displayed in admin/help/*.
Since I wanted it on the setting page, we're ok.
I am adding a strong warning with a pointed to the handbook page, which I have just updated a bit. The interest is that this part of the documentation can grow over time (and users can add comments) without encumbering too much the module itself and the admin pages.
#16
Improved text.
I think it is better to speak in general terms, and leave the details (about drupal_goto()) and the workarounds (antedate) for the handbook: http://drupal.org/handbook/modules/notify , because those are the issues we encountered, but they are certainly other pitfalls to avoid.
#17
Nicely done! It surely looks good on the handbook page.
#18
Have a look at Parenthesis mistake for _notify_send period calculation too please.
#19
I am subscribed to all issues, so I would have seen it anyway :)
Here is an example why it is a bad idea to give specific workarounds: they give a false sense of security.
You say you can ante-date your node with the drupal_goto().
Imagine you forget about this, and later upgrade to a version with this additional feature: http://drupal.org/node/150109 . You set the module to notify you of any single change/update.
Later you go to update the php node with drupal_goto(). No antedating is possible then. The node is scheduled to be sent by email. You are fucked up again.
I know I have one such node on one site. I need to create a separate module for it.
#20
Patch is committed.
The documentation in the handbook needs to be completed.
I also want to review some elements of this discussion.