it feels to me that this code should be moved into pm_block_user. the problem is that we are passing around single recepient object forcing each filtering module to execute queries against single users. This could mean performance issues (theoretically! of course in practice it could prove to be neglegible). However if we pass the whole $message['recipients'] array into the hook, perhaps using drupal_alter to allow altering of the recepients array, then we will enable the blocking/filtering module to excersise best sql that it sees fit, perhaps even one that is optimized to perform well on multiple objects at the same time instead of single object ?


  if (!empty($message['recipients']) && is_array($message['recipients'])) {
    foreach ($message['recipients'] as $uid => $recipient) {
      $block_results = module_invoke_all('privatemsg_block_message', $author, $recipient);
      if (count($block_results) > 0) {
        unset($message['recipients'][$uid]);
        if ($show_warnings) {
          foreach ($block_results as $block_result) {
            drupal_set_message($block_result);
          }
        }
      }
    }
  }
CommentFileSizeAuthor
#4 privatemsg.block_recipients.patch4.74 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Postponed (maintainer needs more info)

Yeah, I thought about that too. There is currently however one showstopper:

the current validate hook doesn't allow to change the $message array, it's read only.

Two possible solutions:

- allow validate to change $message, this means that the hook will change to validate_alter and we need to return errors with $message, for example $message['__validate_errors'].
- We have the presave_alter hook which allows to change $message. The validate hook could somehow remember which users are blocked and only fail if all recipients are blocked. If not, a presave_hook could remove all recipients that were marked as blocked before.

Downside of both solution is that it's more complicated than what we currently have but could lead to a better performance.

another point is that thing with $show_errors, but that could be changed as part of #375999: #288183 followup: Developer friendlify privatemsg_new_thread API. : array('success' => TRUE, 'errors' => array(), 'warnings' => array())

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Unless implementing our own version of a mix of module_invoke_all and drupal_alter, replacing hook_block_message with hook_privatemsg_message_validate is quite complex. In my personal opinion, unecessary complex, because the hook will only be called for the answer form, when sending a message and in the privatemsg_get_link function. And in the last case, we don't have a possible replacement.

So my advice is to keep that hook for now and set the issue to wont fix.

naheemsays’s picture

naheemsays commented

I agree.

The other issue raised is about how the validate function(s) cannot(?) easily modify what is being submitted.

I do not think performance should be a reason for the change as I doubt there will be many people often sending messages to hundreds/thousands of individuals all at the same time.

Being able to easily modify a message object on validation instead of only giving errors could also allow additional feature modules such as a module that may create a new thread when threads get to a certain length. The quota module shows that such things can be done, but its a two step process.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new privatemsg.block_recipients.patch4.74 KB

After discussing this with litwol in IRC, we concluded that there is another way..

Change hook_privatemsg_block_message($author, $recipient) to privatemsg_block_message($author, $recipients), aka: instead of calling the hook in a row, pass all recipients and let the hook handle the recipients. based on the implementation, it can do it in a single query or loop over it, if it's easier. It's a bit more complicated to return blocked users, but it's not *hard. Currently, it's array('uid' => $uid, 'message' => 'bla bla bla'. Unfortunately, something like array(uid => 'bla bla bla') is not possible because of how module_invoke_all works.

Attached is a first patch which does this, it's not very well tested but seems to work.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: #288183 followup: clean up _privatemsg_validate_message logic » #288183 followup: change hook_privatemsg_block_message to work on multiple recipients
naheemsays’s picture

naheemsays commented
Status: Needs review » Reviewed & tested by the community

Tested and it works.

Probably unlinked, but for some reason the body textarea is given a red outline if no recipients can receive a message. Should the username textfield not be highlighted instead?

Since this is a patch that changes the API I think it would be a good idea to get it in ASAP.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

@nbz

That is a limitation of the current validate function and adressed at #375999: #288183 followup: Developer friendlify privatemsg_new_thread API.

naheemsays’s picture

naheemsays commented

no probs, so that means this patch is ready to go.

litwol’s picture

litwol commented
Status: Reviewed & tested by the community » Fixed

Very good :). committed.

I made a small bug fix:
used to be: 

  $result = db_query('SELECT recipient FROM {pm_block_user} WHERE author = %d AND recipient IN ('. db_placeholders($recipients) .') GROUP BY recipient', $author->uid, array_keys($recipients));

became: 

$args = array_merge(array($author->uid), array_keys($recipients));
  $result = db_query('SELECT recipient FROM {pm_block_user} WHERE author = %d AND recipient IN ('. db_placeholders($recipients) .') GROUP BY recipient', $args);

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.