use eval() in a helper function to avoid clobbering of variables

This is because the variables in your code are overwriting the variables in the module code that calls the eval().

We should move the eval() to a helper function where it can't affect any variables; there's http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_eval/6 in core to help with that.

In the meantime, rewrite your code to not use $row. Though I don't really get why you are doing that loop:

foreach($view->result as $row) $content = true; 

If you want to say 'return TRUE if the view has a result', then use count(), like this:

if (count($view->result)) {
 return TRUE;
}
else {
return FALSE
}

or even:

return (count($view->result) > 0);

BTW: Note that the PHP code evaluation has a bug on 2.1, as it does a 'break' rather than a 'continue' -- subsequent newsletter results will be ignored!

I am using insert view and it works as designed. And just to clarify, if I use the php code snippet and it stops sending a new message because the view results are 0, then the next time the view results are > 0, the newsletter will not start sending again because of a bug? (I'm using 2.1 in Drupal 6).

Is there any other way around this bug?

> the next time the view results are > 0, the newsletter will not start sending again because of a bug

No, not the next time. The next in the loop, at that time -- if you have more than one schedule that the code is trying to test at that particular moment.

OK - thank you for clarifying.

And thank you for your dedication to this module. It's really a great tool.

> BTW: Note that the PHP code evaluation has a bug on 2.1, as it does a 'break' rather than a 'continue' -- subsequent newsletter results will be ignored!

Note to self, and anyone else interested: this is fixed in the dev version.

Switching to using drupal_eval() will require the PHP code entered in the UI to use PHP tags. Which will require either an upgrade function to add them, or manual intervention.

Is it worth the effort write an update function, or should be just show a message telling admins to update their PHP code?

Moving version to D7, as this needs fixing there too (where the function is php_eval() instead, but same principle).

Joachim,

My view (daily_beat) is using 6 blocks so I can have different content showing in the newsletter. Each block will only show content if it has been created/updated in the last 24 hours (the schedule is to send 1 newsletter every 24 hours, only if there is new or updated content).

I put each of the blocks in the body field using Insert View (and the content is showing as expected):

[view:daily_beat=block_1]

[view:daily_beat=block_2]

[view:daily_beat=block_3]

[view:daily_beat=block_4]

[view:daily_beat=block_5]

[view:daily_beat=block_6]

However, I am stumped on how to write the PHP code to return true (and send a message). Here's what I've tried and it works when I have just one block inserted into the view (block_1):

$view = views_get_view('daily_beat');
$view->set_display('block_1');
$view->pager['items_per_page'] = 1;
$view->execute();
$count = count($view->result);
if ($count >= 1) {
return TRUE;
}
else {
return FALSE;
}

Can you please help with how to tweak this php so it returns true if 1 of the 6 blocks returns true?

I was thinking that I could simply put the blocks in as an array:

$view = views_get_view('daily_beat');
$view->set_display('block_1','block_2','block_3','block_4','block_5','block_6');
$view->pager['items_per_page'] = 1;
$view->execute();
$count = count($view->result);
if ($count >= 1) {
return TRUE;
}
else {
return FALSE;
}

But this isn't working (no message is sent).

Thank you for your assistance with this.

That's not to do with this specific bug -- something like that should go in a support request. Or ask on IRC ;)

Right - sorry! I've posted it as support request.

I think it would be fine to change it, as the 7.x version is still in dev mode.

so changing to drupal_eval is totally fine :)

Also there is a case to remove this all together

* Advisory ID: DRUPAL-SA-CONTRIB-2012-146
* Project: Simplenews Scheduler [1] (third-party module)
* Version: 6.x
* Date: 2012-September-19
* Security risk: Moderately critical [2]
* Exploitable from: Remote
* Vulnerability: Arbitrary PHP code execution

-------- DESCRIPTION
---------------------------------------------------------

The Simplenews Scheduler module provides a system for creating automatic
email newsletters. These can be set to be sent at a fixed interval, or PHP
code can be entered to evaluate a condition for a new newsletter issue to be
sent.

The module allows a user with the 'send scheduled newsletters' access to the
scheduling form where PHP code may be entered. This code is then executed the
next time the site runs cron. A site administrator granting permissions is
not given sufficient warning that they are granting this level of access to
the site.

This vulnerability is mitigated by the fact that an attacker must have
already been granted a role with the permission 'send scheduled newsletters'.

Most people seem to use this for testing the output of a view.

Perhaps the whole eval thing can be replaced with a view execution and test result has >= 'n' results, as well as a drupal_alter / module hook ?

Yeah, I think Berdir was talking about changing this to a hook. But I'm not sure on the best architecture to use here, and I think that would be potentially a big restructuring.

That might be best left to a 3.x branch.

In the meantime there's no harm in having the PHP eval provided it's got a proper permission on it, which it does now (the security update on 6.x was accompanied by a corresponding commit on 7.x).

I dont think its a lot of restructure, just a matter of making the decision, as it's still in 7x.-dev phase, i think its allowable if the long term outcome is better

At this point i think we need todo what ever we can to just make a solid release