In the function message_subscribe_get_subscribers, the user load function is used multiple times when checking access permissions for all the subscribed users.

This has come up as a performance issue on a high traffic site with thousands of users subscribed to single items.

The function should be using user_load_multiple as per the Drupal api page:
https://api.drupal.org/api/drupal/modules!user!user.module/function/user...

This function should be used whenever you need to load more than one user from the database.

CommentFileSizeAuthor
#16 message_subscribe-n2464653-16.patch1022 bytesDamienMcKenna
#11 message_subscribe-switch-to-user-load-multiple-2464653-4.patch1009 bytesT-lo
#9 message_subscribe-switch-to-user-load-multiple-2464653-3.patch1003 bytesmaikeru
#7 message_subscribe-switch-to-user-load-multiple-2464653-2.patch995 bytesmaikeru
#1 message_subscribe-switch-to-user-load-multiple-2464653-1.patch1.05 KBT-lo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

T-lo’s picture

T-lo
Primary language English
Location Bristol, UK
CreditAttribution: T-lo commented
FileSize
message_subscribe-switch-to-user-load-multiple-2464653-1.patch1.05 KB
T-lo’s picture

T-lo
Primary language English
Location Bristol, UK
CreditAttribution: T-lo commented
Assigned: T-lo » Unassigned
Status: Active » Needs review
amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Thanks. Please remove trailing spaces from the Patch. Also another minor comment:

+++ b/message_subscribe.module
@@ -248,21 +248,24 @@ function message_subscribe_get_subscribers($entity_type, $entity, Message $messa
+  $users = user_load_multiple($uids);

$users => $accounts

Last thing - next time if I can please open this in GitHub, as it's easier to review there, thanks :)

Status: Needs review » Needs work

The last submitted patch, 1: message_subscribe-switch-to-user-load-multiple-2464653-1.patch, failed testing.

maikeru’s picture

maikeru CreditAttribution: maikeru commented

FYI: I think this fails because it needs array_keys in the call to user_load_multiple

T-lo’s picture

T-lo
Primary language English
Location Bristol, UK
CreditAttribution: T-lo commented

@maikeru, you're right, I realised that while porting to the current full release branch..

I'll get this re written later this week.

maikeru’s picture

maikeru CreditAttribution: maikeru commented
Status: Needs work » Needs review
FileSize
message_subscribe-switch-to-user-load-multiple-2464653-2.patch995 bytes

I've gone ahead and added array_keys in there, since I needed to create a patch for our deployment process.

Status: Needs review » Needs work

The last submitted patch, 7: message_subscribe-switch-to-user-load-multiple-2464653-2.patch, failed testing.

maikeru’s picture

maikeru CreditAttribution: maikeru commented
Status: Needs work » Needs review
FileSize
message_subscribe-switch-to-user-load-multiple-2464653-3.patch1003 bytes

And, fix the undef $uid

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Thanks, minor comment:

+++ b/message_subscribe.module
@@ -249,14 +249,16 @@ function message_subscribe_get_subscribers($entity_type, $entity, Message $messa
+  $users = user_load_multiple(array_keys($uids));

$users => $accounts

T-lo’s picture

T-lo
Primary language English
Location Bristol, UK
CreditAttribution: T-lo at Investis Digital commented
FileSize
message_subscribe-switch-to-user-load-multiple-2464653-4.patch1009 bytes

thanks @maikeru

@amitaibu I've switched it to $accounts

darrenwh’s picture

darrenwh CreditAttribution: darrenwh commented

Hi,
No need to create the variable $account, just do this on line 263/261
if (!entity_access('view', $entity_type, $entity, $accounts[$uid])) {
Apart from that all good.
Darren

T-lo’s picture

T-lo
Primary language English
Location Bristol, UK
CreditAttribution: T-lo at Investis Digital commented

Hi Darren,

The $account variable was already there, we're just changing how it's populated.

Is creating a variable that's only used once a problem? Perhaps you could create another issue if so

darrenwh’s picture

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

All looks good

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented

I'll try to a look on that one.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Reviewed & tested by the community » Needs review
FileSize
message_subscribe-n2464653-16.patch1022 bytes

A minor update per @T-lo's suggestion.

T-lo’s picture

T-lo
Primary language English
Location Bristol, UK
CreditAttribution: T-lo at Investis Digital commented
Status: Needs review » Reviewed & tested by the community

  • amitaibu committed a9ec4fe on 7.x-1.x authored by DamienMcKenna 
    Issue #2464653 by T-lo, maikeru, DamienMcKenna: Switch from user_load to...
amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented
Status: Reviewed & tested by the community » Fixed

Merged, thanks.

Status: Fixed » Closed (fixed)

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