Wedge
Public area => The Pub => Bug reports => Topic started by: live627 on June 16th, 2013, 02:40 AM
-
I've yet to find out why.
-
My bet would be the scheduled task didn't use to fire because of not having the right dependencies (and thus becoming disabled because the function wasn't loaded and therefore not callable)... check that your scheduled_tasks table has the right content as per install.sql and check it's enabled.
-
I actually meant here. I should be more clear. :P
@Nao> Please ensure the task has the right parameters in the DB. Also, the task itself must return true in order to not get disabled. :^^;:
Index: Sources/Notifications.php
===================================================================
--- Sources/Notifications.php (revision 2166)
+++ Sources/Notifications.php (working copy)
@@ -670,7 +765,7 @@
continue;
// Assemble the notifications into one huge text.
- $body = template_notification_email($m['notifications']);
+ $body = template_notification_email($m['notifications'], $m['id']);
$subject = sprintf($txt['notification_email_periodical_subject'], $m['name'], $m['unread']);
sendmail($m['email'], $subject, $body, null, null, true);
Index: Themes/default/Notifications.template.php
===================================================================
--- Themes/default/Notifications.template.php (revision 2166)
+++ Themes/default/Notifications.template.php (working copy)
@@ -64,46 +64,14 @@
}
-function template_notification_email($notifications)
+function template_notification_email($notifications, $member_id)
{
global $txt;
@@ -111,7 +79,7 @@
foreach ($notifications as $notifier => $notifs)
{
- list ($title) = weNotif::getNotifiers($notifier)->getProfile();
+ list ($title) = weNotif::getNotifiers($notifier)->getProfile($member_id);
$str .= '
<h3>' . $title . '</h3>
@Dragooon> Why is this template so complicated? Ideally, the variables should be put into $context in the source file. I think templates/views should never do any logic work, only presentation.
-
@live627: It isn't doing any logical work, I wouldn't count getting parameters from an object as logical work.
-
Okay, on a fresh Wedge installation periodical seems to be working. So it might be a problem just here.
-
That points to the scheduled task then, of which I know I changed the semantics recently for calling (changes in DB table)
-
This is still an issue. @Nao please confirm what is in the scheduled tasks table matches what a new installation would generate.
-
Missed this topic... (As I did many others, obviously!)
Yes, the database structure had yet to be updated; including the size for 'task'.
If it still doesn't work, then... It's something else, I guess.
-
It wasn't just structural but also content in that change. But if it's updated now to match, we'll see if it starts working.
-
Fatal error: Call to undefined method Notification::getEmail() in I:\www\wedge\trunk\Themes\default\Notifications.template.php on line 119
That's when trying to run the schd task.
-
I had a quick look, and to be honest with you, I'm not exactly sure where the thing goes wrong... getEmail is a Notifier method, not a Notification one, but then as I said before, Notifiers in general are somehow quite unclear to me, and I tend to easily forget the difference, so... Maybe Dragooon, or someone else, can fix this for me..? :^^;:
-
Is that someone else me? :P
I'm also finding it hard to understand. I've been doing my head in trying to make it work. The emails send, but the data isn't quite right. :^^;:
-
Hmm, not particularly you, but, just in case you had a ready-made solution, I'd have gladly taken it... ;)
-
Ok, so I've been playing around some more. The biggest issue is that the email body is duplicated since it is HTML. II have yet to understand why.
The above error is because it is calling on the Notification object and wants the getEmail() method from a Notifier.
Index: I:/repos/wedge/trunk/Themes/default/Notifications.template.php
===================================================================
--- I:/repos/wedge/trunk/Themes/default/Notifications.template.php (revision 2194)
+++ I:/repos/wedge/trunk/Themes/default/Notifications.template.php (working copy)
@@ -116,9 +116,10 @@
$str .= "\n" . $title . "\n" . str_repeat('=', strlen($title)) . "\n";
foreach ($notifs as $n)
- $str .= $n->getEmail();
-
- $str .= "\n\n";
+ {
+ list (, $n) = $n->getNotifier()->getEmail($n, array());
+ $str .= $n . "\n\n";
+ }
}
return $str;
Let's save some memory! $valid_notifiers is only checked for existence, so why not use a bool instead of an object?
Index: I:/repos/wedge/trunk/Sources/Notifications.php
===================================================================
--- I:/repos/wedge/trunk/Sources/Notifications.php (revision 2194)
+++ I:/repos/wedge/trunk/Sources/Notifications.php (working copy)
@@ -609,7 +608,7 @@
{
$status = isset($data['email_notifiers'][$notifier->getName()]) ? $data['email_notifiers'][$notifier->getName()] : 0;
if ($status < 2 && (empty($data['disabled_notifiers']) || !in_array($notifier, $data['disabled_notifiers'])))
- $valid_notifiers[$notifier->getName()] = $notifier;
+ $valid_notifiers[$notifier->getName()] = true;
}
if (empty($valid_notifiers))
Objects shouldn't be this hard to understand. I think it's the duplication that's the problem. Perhaps Notification can go, and its leftovers merged with weNotif. Also, shouldn't Notifier be abstract?
-
Last point first, Nao didn't want the Notifier to be completely abstract so that anything common to all notifiers could be extended easily. Quite what would be common to all notifiers, I don't know, I can't think of anything but that's the theory behind it.
The notification system is hard. When I wrote the likes notification I started from one of the others that was done and just hacked it about until it worked. I actually don't understand all the guts of it, I don't think I ever have, actually...
-
Okay, then I'm relieved to see I'm not the only one with problems... :P
Since I did extensive rewrites of the system, I guess I have a correct understanding of it, so to make everything clear, it all boils down to three structures:
- the Notification object, holds a single notification for a member, a certain type. It provides methods that give low-level access to the notification's ID, type, etc.
- the Notifier object, is a 'generic' notification type, which provides plenty of default higher-level methods for a notification plugin (getEmail, etc.)
- the plugin's notifier, inherited from Notifier, called through issue().
Basically, a Notification object always has an associated Notifier.
If you get that, you're already halfway through the system.
Which is, err... Pretty much where I am, lol.
Anyway, I've made a few changes to John's suggestion, and will commit these. Unfortunately, I'm not bothered enough to test my changes, so... Hopefully it works, ah ah.
foreach ($notifications as $notifier_name => $notifs)
{
$notifier = weNotif::getNotifiers($notifier_name);
list ($title) = $notifier->getProfile($notifs);
$str .= "\n" . $title . "\n" . str_repeat('=', strlen($title)) . "\n";
foreach ($notifs as $n)
{
list (, $body) = $notifier->getEmail($n);
$str .= $body . "\n\n";
}
}
(getEmail only has one parameter now, because I removed $email_data, which doesn't have any purpose, and thus 'gets in the way', IMHO...)
-
Commit it and I will test it.
If you get that, you're already halfway through the system.
-
I couldn't find any..?
-
Example: some of the get***() methods are present in both Notification and Notifier.
-
They're named similarly (e.g. getEmail is in Notifier, and getText in Notification), but I don't think they overlap..?
-
So I got the email this morning, but it was in French, and had no line breaks.
-
So I got the email this morning, but it was in French, and had no line breaks.
-
Yes you do :P
Welcome back!
I re-disabled the task yesterday right after 'forcing' it, and started work on fixing everything.
I didn't discuss it because, well, it's just plain boring, but here's what new so far.
- Plain text e-mails will now correctly show linebreaks, and better whitespace handling generally.
- There's now an option in Notifications.php to send in HTML, and I've set it to true, because from what I discovered in the sendmail function, HTML e-mails actually send two copies in multipart -- a raw e-mail, and a HTML e-mail. I modified sendmail() to allow for the sender to provide two different bodies for each context; i.e., in the case of notifications, we're sending both versions available. The only drawback is that we have to run both of these through prettify_urls()... Ah, well... I guess if you want it, you can always use the mail queue.
- I've modified a lot of the code in sendmail, mostly to make it easier to understand... Well, there's a part that got repeated twice in the original code (including SMF), now it's no longer the case.
The only problem that remains, right now, is with the language files, obviously... Wedge will use the language file from the current user at the moment the scheduled task is triggered.
I'm thinking of loading the users per language file, and then re-loading all language files, but... Well, how am I supposed to do that..?! Maybe there's already a built-in function to help with that in scheduled tasks, but I don't know anything about that area of Wedge, so...
-
Test. (Sorry.)
-
Well, there's a part that got repeated twice in the original code (including SMF), now it's no longer the case.
I'm thinking of loading the users per language file, and then re-loading all language files, but... Well, how am I supposed to do that..?!
$emaildata = loadEmailTemplate($message_type, $replacements, $needed_language);
sendmail($row['email_address'], $emaildata['subject'], $emaildata['body']);;
-
Nope, didn't remove the 'fix', because we need a 7bit-compatible fallback in all e-mail types anyway. The hotmail fix provides that.
Your solution, would it reload the Notifications language files, too..? Because that's where we find the strings for HTML e-mails, isn't it..?
-
Well, the email strings need to be moved to the EmailTemplate language file anyway, which loadEmailTemplate() loads anyway.
-
You mean duplicated...? Coz they're the same strings as used in the notif popup... ^^
-
Bump.
Also. How about triggering scheduled tasks only if the current user's language is the default..? Then at least we can avoid what happened last time.
-
How about triggering scheduled tasks only if the current user's language is the default..? Then at least we can avoid what happened last time.
-
How about triggering scheduled tasks only if the current user's language is the default..? Then at least we can avoid what happened last time.
I think that it would be best to test for scheduled task triggers only if the current visitor is using the default forum language; otherwise, we'll be risking what happened the other day, when everyone received an e-mail in French...
And frankly, I don't think it's realistic to expect the scheduler to 'know' what language files need to be reloaded specifically for this task.
Of course, I could 'quite simply' do some magic trick to loadLanguage...
function loadLanguage($template_name, $lang = '', $fatal = true, $force_reload = false, $fallback = false, $force_reload_all = false)
{
global $theme, $context, $settings, $boarddir, $db_show_debug, $txt, $helptxt, $cachedir;
static $already_loaded = array(), $folder_date = array();
if ($force_reload_all)
{
loadLanguage(array_keys($already_loaded), $lang, $fatal, true, $fallback);
return '';
}
There you go, this adds a trigger to force a mass-reload of all existing language files loaded so far.
Of course, that's going to be relatively expensive, but if we don't have more than a couple of active language files, it's just a matter of gathering all users that are on the same language as the current user, then send the e-mails, then gather the other users by language, and for them, call $force_reload_all, and send the e-mails to them. Then, restore $txt (which, of course, we'll have saved in the first place.)
Dunno which solution is the best, really... What do you think of this function hack, though?