Wrong argument for reset()
kenorb - February 10, 2009 - 15:51
| Project: | Simplenews Template |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | postponed (maintainer needs more info) |
Description
warning: reset(): Passed variable is not an array or object in /home/xcomkids/public_html/sites/all/modules/contributions/simplenews_template/simplenews_template.module on line 91.
#1
Patch attached.
#2
This breaks cron for me. Here's the error I get:
Fatal error: Cannot use string offset as an array in /usr/home/mckean/public_html/modules/simplenews_template/simplenews_template.module on line 93
#3
It seems the above patch breaks cron because simplenews_template_mail_alter() gets executed with an empty $message variable sometimes. Not sure why that happens, but checking the $message variable in simplenews_template_mail_alter() before doing anything with it will keep cron from breaking:
function simplenews_template_mail_alter(&$message) {global $processed;
if(is_array($message)) {
$function = "_simplenews_template_mail_alter_{$message['id']}";
if (function_exists($function)) {
$function($message);
} else if (!$processed) {
_simplenews_template_mail_alter_simplenews_node($message);
$processed = TRUE;
}
}
}
#4
Without patch is not breaking? It's weird because this code is doing the same thing as last one.
Previous code did the same operation twice (where is no sense), specially when $message['params']['context']['account']->tids is not array, there is PHP error.
Second thing that you can't go inside condition if reset will not have array as argument http://php.net/reset what's requirement of PHP.
<?php$tid = reset($message['params']['context']['account']->tids);
if (!empty($message['params']['context']['account']->tids)) {
$tid = reset($message['params']['context']['account']->tids);
?>
So in my opinion it has nothing with $message variable.
#5
The patch you posted fixed the reset() error. However, it caused another problem with cron (at least with my installation). When sending newsletters, cron would crash with the error that I posted in comment #2. If I debug the cron run, function simplenews_template_mail_alter ends up getting called multiple times, and sometimes it gets called with a completely empty $message variable. When $message is empty, doing "is_array($message['params']['context']['account']->tids)" results in the error in comment #2. If $message is empty, there's no point in doing anything with it, which is why I did "is_array($message)" in simplenews_template_mail_alter.
To clarify, yes your code fixes the reset() error. But if the $message variable is empty when it hits your code, the error in comment #2 occurs. What I posted was a workaround for that, not a substitute for your code. Running your patch and my snippet, I no longer get the reset() error and cron does not fail when sending newsletters.
#6
Thank you for that.
This one in attachment is updated and it shouldn't change code flow.
#7
Hello,
I just installed patch from comment #6. I am still getting this error:
The code I have looks like this:
function _simplenews_template_mail_alter_simplenews_node(&$message) {
// Setup newsletter data
$tids = $message['params']['context']['account']->tids;
$tid = is_array($tids) ? reset($tids) : $tids;
if (!empty($tids)) {
$tid = is_array($tids) ? reset($tids) : $tids;
} else {
$tid = $message['params']['context']['newsletter']->tid;
}
$newsletter = taxonomy_get_term($tid);
$node = $message['params']['context']['node'];
$content = $message['body']['body'];
// Retrive and filter the header content
$header = _simplenews_template_get_header($tid);
$header = check_markup($header, _simplenews_template_get_header_format($tid), false);
// Retrive and filter the footer content
$footer = _simplenews_template_get_footer($tid);
$footer = check_markup($footer, _simplenews_template_get_footer_format($tid), false);
// Add headers and footer
$content = theme('simplenews_template_content', $newsletter->name, $node->title, $header, $content, $footer);
// Fetch Simplenews Template styling for this newsletter
$style = _simplenews_template_get_css($tid);
$bgcolor = _simplenews_template_get_bgcolor($tid);
// Markup node body with Simplenews Template style
$content = theme('simplenews_template_mail', $newsletter->name, $node->title, $content, $style, $bgcolor);
// Run HTML and CSS through Emogrifier, if available
$content = _simplenews_template_emogrify($content, $css);
$message['body']['body'] = $content;
}
Please let me know what needs to change. Thank you.
#8
I think the whole concept of this code in this section is wrong, not because of this patch. And it's the same on 5.x version.
Nobody knows what this line is doing:
$tids = $message['params']['context']['account']->tids;
Any why there is no verification if this object exist (specially when cron is run). Or it should just exist.
You've got this error, because $message should be an array, but in your case string is found. This code was originally, so I don't know why you getting a string.
Make sure that you are using the last dev version of simplenews, if not, I think it's another bug.
Or if you think that previous code without patch was working, get back to it and try to add @ to ignore the errors to fix reset() error:
<?php$tid = @reset($message['params']['context']['account']->tids);
if (!empty($message['params']['context']['account']->tids)) {
$tid = @reset($message['params']['context']['account']->tids);
?>
Because I can't reproduce your error.
Or you can add some debugging code, like in the first line of this function add:
if (!is_array($message)) {watchdog('simplenews', 'Illegal message element = "%message".', array('%message' => print_r($message,true)), WATCHDOG_ERROR);
}
I haven't test this code, so you need to check if the syntax is ok. And check your logs on the next error.
#9
@kenorb: Have you created and sent a newsletter issue, to multiple recipients, via cron run on your test site, and did not get the error in #2?
FYI I am using Simplenews 6.x-1.0-rc5 and the latest Simplenews Template 6.x-1.x-dev. I also tested with the latest Simplenews 6.x-1.x-dev. I still get the error in #2 when cron is run (when there are newsletters to be sent) if I do not use the modification I made in #3.
#10
PLEASE NOTICE THAT SIMPLENEWS_TEMPLATE 6.x IS UNSUPPORTED.
The 6.x version of Simplenews_Template was just a quick stab on porting the 5.x version by tobiassjosten. My intention is to harsh out bugs and other issues as soon as possible. Any help is welcome.
#11
But I still don't know if you have this error because of patch in this thread or you reporting the new issue which happen without patch as well. If yes, please create the new issue, then this one will be committed, and after that there can be done another patch of your case.
Because you are showing different function that was patched and I don't know if it's related or not, so it just delaying this issue, because of confusion.
I haven't tried to send mails via cron, because I prefer to run it immediately (because simplenews_template at the moment is in very basic development).
Please simple confirm if you had the error before applied this patch.
#12
I think problem with message passing through file.
message body is not properly set in file.
---------------------------------------------------
function _simplenews_template_mail_alter_simplenews_node($message) {
// Setup newsletter data
$tid = reset($message['params']['context']['account']->tids);
if (!empty($message['params']['context']['account']->tids)) {
$tid = reset($message['params']['context']['account']->tids);
} else {
$tid = $message['params']['context']['newsletter']->tid;
}
.........................................
in this function message is blank pass.
#13
Yes, so original code haven't check if $message is empty, that's why there was error with reset() function which is requiring mandatory argument as array, see: http://php.net/reset.
So that's why I've made this patch because of problem of invalid $message value, and it's not related to different issues and issues when is called by cron and why value of $message is invalid (what was this issue all about).
If there was no any problem with $message argument, there was no any patch here.
If somebody is happy, can debug why function is called with empty $message, but it's a different issue (please create new one as related).
Note: This issue is only about 'warning: reset(): Passed variable is not an array or object' problem and it fixing the error (which was for me very annoying) and nothing more.
If you have different error message tested on development version, please report new issue.
#14
#15
@kenorb: Your patch fixes the reset() warning, but it causes an error that causes cron to fail (which essentially breaks Simplenews core functionality, which is sending newsletters, along with anything else that depends on cron). Without your patch, we get a warning message in our error logs, but all the functionality is there. With your patch, we no longer get the reset() warning in our logs, but then can no longer send newsletters, and anything else that depends on cron is essentially broken. What is the point of fixing a php warning message if it breaks core Drupal functionality?
#16
Ok, it's clear now, thank you.
I'll try to test it maybe tomorrow with cron.
#17
This error shouldn't appear any more if you use this patch:
http://drupal.org/node/243567#comment-1240161
http://drupal.org/files/issues/simplenews_template.patch
#18
I recently got CVS access and I am planning on helping out with this project. Is anyone out there still using the module who have any other input about this issue?
I am currently using the module with a patch of my own, which basically removes the first reset() call alltogether, and seems to be working fine. I'll review this issue in more detail.
#19
I'm not sure if this issue is still there.
There were too many patches to different versions, because nobody committed anything for long period of time and this module have too many bugs. And some bugs are caused because some people using some already patches with different version.
Please commit anything working and publish dev version, so people will be able to make patches against the current version.
#20
subscribing
#21
#22
Thank you all for the work, but this issue seems a bit lost.
I fixed a lot of bugs already, and the module is being worked on again. I hope for a very soon release of a beta3, and perhaps a stable release is not so far away.
Could you please help clarifying? I understand the last patch, but the first chunk is based on some old version, it seems.
There is now a dev release available (only after midnight with the last changes). It would be nice if you can all test it and see if the error is still around.
#23
We were getting the error, and I just downloaded and installed the Nov 6 dev version. The error seems to have gone away. I'll provide further update if anything changes here.