The documentation of message_create() states that the $account parameter is ignored when the $args['uid'] parameter is set, as is the case in oa_messages_create(). As such, the call to user_load can be removed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jorrit created an issue. See original summary.

Jorrit’s picture

Status: Active » Needs review
FileSize
825 bytes
Jorrit’s picture

FileSize
750 bytes

Fix the patch.

mpotter’s picture

Looking at the OG message_create code, I don't see any evidence of this. It is setting the $values['user'] field with the account and not overriding the 'uid' value. I would need to see a set of test cases that show this is not an issue. The chance of this breaking something obscure seems too high.

Keep in mind that messages get created by things like the oa_event_import when handling incoming comment replies, etc which are tricky to test. At some point, even with the 'uid' field it's going to need to do a user_load which should cache the result, so calling it a second time shouldn't be a performance issue.

Unless there is an actual bug being fixed here I am leery to make this kind of change.

Jorrit’s picture

I understand that you might not want to merge this. The fact that message_create sets $account is a bug on their end, because they ignore the value later on in Message::__construct():

    if (!isset($values['uid']) && isset($values['user'])) {
      $values['uid'] = $values['user']->uid;
      unset($values['user']);
    }

In the case of oa_messages_create $values['uid'] is set, so $values['user'] is ignored.

In my case I want to use oa_messages_create() to create several extra notifications for users that are not necessarily loaded at that time and I want to make it as efficient as possible. I'll leave it up to you to decide if this is worth considering.

mpotter’s picture

Ahh, yes, you are correct about the logic in the message constructor. That is the code I needed to see.

I can see the use-case for messages from users that are not loaded. Can you confirm that when this patch is used that the user is still not loaded somewhere else? Seems like it would need the full user object in case the message text references the user (like to expand the user's name in the notification subject)

Jorrit’s picture

You're right, in most realistic scenarios user_load() will be called somewhere in the same page load for the user id. As you noticed, I am working with message and oa_messages today and I noticed that both a uid and user were passed to message_create. Because it wasn't costing me a lot of time to work out a patch I submitted one.

My use-case is that I am building an event invite system where entire groups can be invited. Each user will get a personal invitation, which doesn't contain any user details, as the message is something like 'You've been invited to event XXXX'.