Make message_notify more generalised

I'm happy to provide a patch to illustrate what I mean, as I will be doing it for a project anyway.

This is the kind of direction I was thinking.

The main differences being that the notification message type has an extra short msg field and the send mail function is now a send message function which uses ctools plugins for notifiers.

I have only added the email notifier, but for example a twitter notifier might compose it's message from the short msg field (using the body text if the short msg is empty - because the body field is required and a twitter only msg wouldn't have a long version). Then it would send out via the twitter module's api or something.

The part that is a bit messy is the message type with the extra field.
On one hand it allows you to cover pretty much any use case with the notification type.
For example, if you were to add to the users account a setting where they can choose their preferred notification method and you have am admin page for sending out messages. You want the admin to be able to go to the message page and write one message, which is then sent out to all the users. Having the separate fields allows for this. Alternately, the admin would have to complete one form for long form messages and one for short messages.

But then for people who only want email notifications you have an extra field getting in the way.
And to account for people who only want to use short messages you would want the body text not required, and then it is there unnecessarily.
Or as you mentioned before, in that case you don't really need the extra small msg field because the body text would become the small message text.

I don't really like that aspect of the patch.

Maybe a better solution is to have the main message_notify module have message_notify_send_message() and the ctools plugins like in the patch (or something similar), but then have the email plugin living in a sub module, and the creation of the message_type_email bundle would be done in the sub module. The message_notify module itself wouldn't make any message type categories or bundles, it would just be the send message related functions.

Then the message_type_notification bundle as I have done in the patch could be a separate module of it's own for it's own special use case.

Ideas, comments, criticism welcome.

I will probably do another patch with the email notifier split out to a sub module that provides the message_type_email as it is in the current version of the module and the email notifier as in this patch. - Doing away with the message_type_notification that is in this patch - it can live elsewhere.

Damn that patch is incomplete.
I'm working on a revised version at the moment anyway, after further thought.
I'll post it soon.

OK, here is a version I like much more.

message_notify is just an API type module and has a message sending function that can be used by any notifier. It handles saving messages & error reporting etc.
CTools plugins are used for notifiers.

There is a new sub-module message_notify_email, which adds the message_type_email message type as in the current module. It also adds the email notifier plugin.

This solution makes the send message function a bit more complex but also more generic.

The only downside in having the send message function do all the message field saving is that it is hard to account for unusuall fields like node reference fields, which have an nid instead of a value.

I was tempted to abandon the idea due to that and just stick with completely separate modules for each notifier that are completely separate from message_notify, although I really want to avoid the duplicate code that would come as a result.

So it also has a (currently undocumented) callback that the notifier plugins can declare to handle any rendered values for unusual fields like that.

I have tested using the message notify example module and it is working.
That module would probably be called message_notify_email_example instead.

The plugin system would need to be properly documented so others could make their own more easily.

Let me know if you like the idea, have some changes for the idea or hate the idea. - Or any other comments/criticisms.

Uploading the whole module instead of a patch.

Here you go.

Thanks for the feedback, I'll make the changes probably later today, but for a couple of your notes:

+++ b/message_notify.moduleundefined
@@ -2,45 +2,89 @@
+  // Allow the notifier to do some custom field rendering if they have any

Do you have a use case for this?

Not really, I added that bit in at the last minute.
The reason I added it is because I haven't had a chance to fully learn about entity wrappers and for some reason I figured (without investigating) that it would default to trying to save data to the 'value' column, which I thought would break for fields like node references, with their 'nid' column.

Now I realise this isn't the case, so the code below should be able to handle any type of field right?

+++ b/message_notify.module
@@ -2,45 +2,89 @@
+          if (!empty($wrapper->type->{$rendered_fields[$rendered_name]}->format)) {
+            $format = $wrapper->type->{$rendered_fields[$rendered_name]}->format->value();
+            $wrapper->{$field_name}->set(array('value' => $message->$rendered_name, 'format' => $format));
+          }
+          else {
+            $wrapper->{$field_name}->set($message->$rendered_name);
+          }

So I can do away with the extra callback.

Let's use classes here (you can look at entityreference module, to see how we use classes). So we won't need to do things like function_exists() when calling a method.

So create a notifier handler class with a send function and then the email notifier handler class implements that?
That sounds cleaner.

It's super busy at the moment so it'll probably be a few days before I get back to this. Not too long though.

Thanks, looks interesting. I'll have to have a play with that.
Also, I will be able to finish this pretty soon.

Yeah, sorry about that.

I'll update the patch as soon as possible this week.

Partly done, hopefully I will have something for you tomorrow.

Thank you so much for working on this.

I just went back to find the work I mentioned in #17 and can't find it anywhere. Damn annoying.

Some review of your latest patch:
Amitaibu, feel free to correct me on any of this. Particularly the class part as I am least familiar with that aspect.

+++ b/message_notify.module
@@ -2,66 +2,89 @@
   // Set default values.
-  $options += array(
+  // Defaults can be overridden by notifiers and directly via the function
+  // parameter.
+  $options += $notifier['default_options'] += array(
     'save on fail' => TRUE,
     'save on success' => TRUE,
-    'rendered subject field' => FALSE,
-    'rendered body field' => FALSE,
-    // Override email address.
-    'mail' => FALSE,
-    // Override user's language, and use Message language.
-    'language override' => FALSE
+    'rendered fields' => array(
+      'rendered_body' => MESSAGE_FIELD_MESSAGE_TEXT,
+    ),
+    'save rendered fields' => array(),
   );

This part should be replaced with PHP's array_merge_recursive() or maybe even just array_merge() so that the hard coded array valudes are overridden by $notifier['default_options'], which are overridden again by $options.

+++ b/message_notify.module
@@ -2,66 +2,89 @@
+  // Allow the notifier to do some custom field rendering if they have any
+  // special requirements.
+  if (!empty($notifier['render_callback']) && function_exists($notifier['render_callback'])) {
+    $notifier['render_callback']($wrapper, $message, $account);
+  }

This part can be removed. There isn't really any use case for it.

+++ b/message_notify_email/message_notify_email.module
@@ -0,0 +1,62 @@
+/**
+ * Implementation of hook_ctools_plugin_api().
+ */
+function message_notify_email_ctools_plugin_api($module, $api) {
+  if ($module == 'message_notify' && $api == 'notifier') {
+    return array('version' => 1);
+  }
+}

This is not required.

+++ b/message_notify_email/message_notify_email.module
@@ -0,0 +1,62 @@
+/**
+ * Implementation of hook_ctools_plugin_dierctory().
+ *
+ * Used to let the system know we implement plugins.
+ */
+function message_notify_email_ctools_plugin_directory($module, $plugin) {
+  if ($module == 'message_notify' && !empty($plugin)) {
+    return 'plugins/' . $plugin;
+  }
+}

&& !empty($plugin) is not required because $plugin will not be empty.

+++ b/message_notify_email/plugins/notifier/MessageNotifier.class.php
@@ -0,0 +1,19 @@
+class MessageNotifier {

Now that a class system is being used, maybe message_notify should implement an abstract notifier class with all the required functions at this stage maybe just sendMessage and settingsForm are required.

Then MessageNotifier can extend the abstract class and provide its implementation of sendMessage but doesn't have to implement settingsForm if it doesn't need to.

There is some examples of this in the entityreference module. There is also some decent help in ctools via the advanced help module and

+++ b/message_notify_email/plugins/notifier/MessageNotifier.class.php
@@ -0,0 +1,19 @@
+  function message_notify_email_send(Message $message, $account) {

This can be called something more generic like sendMessage as the same function needs to be used for all notifier classes.

+++ b/message_notify.module
@@ -2,66 +2,89 @@
+  // Send out via the requested notification method.
+  if (!empty($notifier['send_callback']) && function_exists($notifier['send_callback'])) {
+    $result = $notifier['send_callback']($message, $account);
   }

Now that the plugin is going to be a class this part needs to be replaced.
There is no longer need for function_exists etc.
You can load the notifier class using ctools_plugin_load_class() and then call the class's sendMessage function.

Because of the new class structure we can be sure that function will exist.

I still have to have a look into the settingsForm part to see what is actually required in that area.
This is only a visual code review at this stage. I won't have time to actually test any patches hands on or make any additions to the patch for a few days.

Damn, that review was for the patch in #20.

Having a quick look at the new one now.

A quick look through.
A number of the above comments have been addressed already.
(there are some more detailed comments in the previous review)

+++ b/message_notify.module
@@ -2,66 +2,91 @@
   // Set default values.
-  $options += array(
+  // Defaults can be overridden by notifiers and directly via the function
+  // parameter.
+  $options += $notifier['default_options'] += array(
     'save on fail' => TRUE,
     'save on success' => TRUE,
-    'rendered subject field' => FALSE,
-    'rendered body field' => FALSE,
-    // Override email address.
-    'mail' => FALSE,
-    // Override user's language, and use Message language.
-    'language override' => FALSE
+    'rendered fields' => array(
+      'rendered_body' => MESSAGE_FIELD_MESSAGE_TEXT,
+    ),
+    'save rendered fields' => array(),
   );

As above. replace with array merge.

+++ b/message_notify.module
@@ -2,66 +2,91 @@
+  // Allow the notifier to do some custom field rendering if they have any
+  // special requirements.
+  if (!empty($notifier['render_callback']) && function_exists($notifier['render_callback'])) {
+    $notifier['render_callback']($wrapper, $message, $account);
+  }

As above, this can be removed.

+++ b/message_notify.module
@@ -2,66 +2,91 @@
+  if (!empty($notifier['handler'])) {
+    $result = $mailnotifier->message_notify_send_email($message, $account);
   }

message_notify_send_email should be called something more generic as it will be used for non-email notifiers as well.

+++ b/message_notify_email/plugins/notifier/email.inc
@@ -0,0 +1,41 @@
+class EmailNotifier {

As above, it would be good to have an abstract class for notifiers defining all the available functions and then extend it here.

+++ b/message_notify_email/plugins/notifier/email.inc
@@ -0,0 +1,41 @@
+public function message_notify_send_email(Message $message, $account) {

Needs a more generic function name.

+++ b/message_notify.moduleundefined
@@ -73,37 +98,43 @@ function message_notify_send_mail(Message $message, $options = array()) {
+function message_notify_ctools_plugin_api($module, $api) {

I think it's actually needed for CTools, not sure. I'm ok with keeping it, for versioning in case one day we will need it.

Yeah, I think in my patch it was in message_notify & message_notify_email.

It should be in message_notify but doesn't need to be in message_notify_email.

I will test ASAP (within the next day or so).

Sounds like a good idea though.