Refactor reply form out of privatemsg_new()

I wouldn't call it exhaustive testing, but I've been using this patch all day and sent many replies to threads with all types of recipients. Also created a number of new messages including message to entire roles that defers to the batch processing.

I also like that I can actually grok the new message form :)

[Edit], I also temporarily started using the block messages hook to shutdown all replies for a thread, but decided to do it differently because of redundant error messages being printed.

+++ privatemsg.pages.inc	20 Jan 2011 20:22:10 -0000
@@ -186,96 +186,161 @@ function privatemsg_view($thread) {
+/**
+ * Form builder function; Write a reply to a thread.
+ */
+function privatemsg_form_reply(&$form_state, $thread) {

Maybe better privatemsg_reply_form() then current?

_new()
_reply_form()

Changing this in D7 we can backport this patch back to 6 and dont forget about changing docs about

Powered by Dreditor.

re #4: The reason for the redundant messages is that the current code 'requires' a message for each blocked recipient. And then prints them all.

This would be for another issues, but perhaps we could think about how to hide or collapse messages, or at least provide a mechanism to do so. The 'disable replies' use case is a little special though, because it is unilaterally saying all recipients are blocked, and not for any good reason. The normal use cases (limits, bad behavior, etc.) would most likely follow the current per-recipient pattern.

+++ privatemsg.pages.inc	20 Jan 2011 20:22:10 -0000
@@ -186,96 +186,161 @@ function privatemsg_view($thread) {
-  // convert recipients to array of user objects
+  // Convert recipients to array of user objects.
   $unique = FALSE;
   if (!empty($recipients) && is_string($recipients) || is_int($recipients)) {
     $unique = TRUE;
     $recipients = _privatemsg_generate_user_array($recipients);
-  }
-  elseif (is_object($recipients)) {
-    $recipients = array($recipients);
-  }
-  elseif (empty($recipients) && is_string($recipients)) {
+  } else {
     $recipients = array();
   }

Note: Previously the new form accepted an array of recipients or a single recipient object. Obviously, since the tests passed, privatemsg did not take advantage of this fact. My custom module was overriding the new message menu router with a custom menu loader function that turned non-user arguments into an array of recipient objects. (this was necessary because the old logic assumed the string arg was a user).

Due to recent commits, I no longer need to do what I was doing. I can happily pass user_# or group_# etc. and things appear to be working.

So ... This is technically an API change (that may affect custom implementations), but now that we have proper string parsing of 'user_#' its is probably not neccesary to be able to pass an array or object anymore.

If we do want to retain the old functionality we just need to revert the changed lines above.

As a side note: I've never understood (and in my fork I have overridden) why we would not want the to_plain to be unique?

Powered by Dreditor.

I need the recipients to always be unique in the to field, because my magical auto-complete super-widget requires it :) (for matching keys to the human readable version).

One way I could deal with this without having to maintain it as a custom patch is if we move all the recipient parsing to a process function on $form['recipient']. Then I can just override that.

[Edit] The one downside is that modules that just want to form_alter will not have access to the true default_value, because that wouldn't be filled in until the process function runs.

Thoughts?

Whatever we decide, we can probably move the last few comments to a new issue, so this discussion doesn't pollute the D7 porting.