Cache usernames in fb_username_alter

I'm also seeing this as an issue. In fact I don't want the facebook module to rewrite my usernames as I'm also integrating with CiviCRM and want the data stored in Civi to be the information that is shown. Perhaps a configuration option can be added to make this optional as well as caching the results?

after using xhprof i was able to identify that this was also my problem causing an additional 10 secs onto each request. Any ideas how to cache or disable this?

I just had the same issue.

Try this

function fb_username_alter(&$name, $account) {
  // This function can be called very early in the bootstrap process, before
  // the modules are initialized, in which case we will fail to alter the
  // name.
  if (variable_get('fb_connect_theme_username_1', FALSE)) {
    if ($GLOBALS['_fb'] && $fbu = fb_get_fbu($account)) { // @TODO - is fb_get_fbu() a performance hit here?
      try {
        $name = cache_get('fb_username_' . $fbu);
        if (!$name) {
          // Use fql query instead of graph api, because it will succeed more often.
          $data = fb_fql_query($GLOBALS['_fb'], "SELECT name FROM user WHERE uid=$fbu", array(
              'access_token' => fb_get_token($GLOBALS['_fb']),
                  ));
          if (count($data) && isset($data[0]['name'])) {
            $name = $data[0]['name'];
            cache_set('fb_username_' . $fbu, $name, 'cache', CACHE_TEMPORARY);
          }
        }
      } catch (Exception $e) {
        fb_log_exception($e, t('Failed to alter username for facebook user %fbu', array(
                    '%fbu' => $fbu)));
      }
    }
  }
}

I have wrapped the whole function to not even do a check if the Admin variable in "FB Connect" is disabled and then added some drupal caching to lookup the users fb_uid for the username.

I have set the caching to TEMPORARY... I am not sure how long this would last, but it as increased the speed of my site byt 600%, also not sure if I have missed anything.

Hope it works for you.

Subscribing

I cant seem to get what you are suggesting to work. What is it exactly you did?

You open Dev for 6 fb.module. Copy line 1373-1389. Paste it over lines 1417-1445 in fb.module for dev of 7

Attached is the patch based on #3.

@inolen I would have to agree.
I tried the code from v6 and it seems to work like my code in #3. I didn't take a look at v6 as I wasn't using it.

Let's not reinvent the wheel... if ver6 works let's use it. I am just puzzled as to why it was changed?

Attached is the patch based on #3, based on the newest (2011-10-01) dev release of FB.

I would think that using the patch above would be better simply because it uses cache_set and cache_get which are compatible with set-ups with a distributed caching mechanism like memcached.

The d6 code appears to use session to store the data, which does not work with certain set ups (in terms of caching). For this reason I personally would use the patch above.

Sorry, just catching up to this. And I'm a bit confused myslef about what happened during the D7 upgrade.

Do the FB_CONNECT_VAR_THEME_USERNAME variables need to change? now that theme_username also alters the name? It might be that in D7 only one of those variables is needed, or it might be that altering needs its own variable.

I wonder if theres a privacy issue here. I wonder if we will end up showing user names to users who would otherwise be unable to see it. I.e. if it is cached while the user is logged in, then rendered to a user who would not have permission to see it, normally. I'm not even certain there is a privacy setting for that. Just thinking out loud.

So I agree there should be a caching of username alters, for performance it is needed. But I think it needs its own new variable to enable/disable it. The FB_CONNECT_VAR_THEME_USERNAME will control whether to use XFBML markup, while a new variable controls whether to alter usernames. Doesn't that make sense?

I like the idea of a new variable, I just used what was there as it made sense at the time.

But hind-site is a great thing

The above patch has a bug in it. cache_get() returns an object not a string.

I'm getting malformed patch line 41 trying to apply it.

related: #1193778: Facebook user name always overrides Drupal username

See #1249026: Page Load slows when Facebook Connect set to primary. Patch #26 of that thread attemps to solve this problem not by caching, but skipping the username alter in most cases.

I've just created a patch that adds a check-box in the admin menu to decide if it should cache the username or not. It defaults to not. Please test the patch as I couldn't do it against the latest beta release.

tried the patch and got this. did it against June 20 official D7 version

git apply ca*patch
cache_fb_usernames-1188256-15.patch:57: trailing whitespace.

cache_fb_usernames-1188256-15.patch:60: trailing whitespace.

cache_fb_usernames-1188256-15.patch:62: trailing whitespace.
// If no static cache is set we try the persistent cache but here we have to check
cache_fb_usernames-1188256-15.patch:75: trailing whitespace.

cache_fb_usernames-1188256-15.patch:83: trailing whitespace.

warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.
aegir@devmac:~/p/7/m/all/fb$ patch -p1 < c*h
patching file fb.admin.inc
Reversed (or previously applied) patch detected! Assume -R? [n]
Apply anyway? [n] y
Hunk #1 succeeded at 392 with fuzz 1 (offset 10 lines).
patching file fb.module
Reversed (or previously applied) patch detected! Assume -R? [n]
Apply anyway? [n] y
Hunk #1 FAILED at 54.
Hunk #2 succeeded at 70 with fuzz 2 (offset 5 lines).
Hunk #3 FAILED at 1606.
Hunk #4 succeeded at 1804 with fuzz 2 (offset 29 lines).
2 out of 4 hunks FAILED -- saving rejects to file fb.module.rej

Try this one. I tried to apply it using patch -p1 < ../urls_inside_custom_node_are_not_rewritten-1298460-16.patch and it worked fine.

This one also fixes some bugs that the previous one has.

patch applied

Ok. Let me explain it a little. This patch adds a new check box in 'admin/structure/fb/settings' called 'Cache formated Usernames'. If sett to On, dff will cache the username once retrieve from Facebook into an persistent cache bin. If you select Off it will avoid the persistent cache but it will still use the static variable bin (see drupal_static) which means that if it is called multiple times during a request it will query only once the Faceboko server. So, it anyway adds some speed improvement even if is sett to not save the username into a persistent cache bin.
But don't forget that by default the persistent cache is disabled so after applying the patch you have to go to admin/structure/fb/settings and check On.

Please give me feedback regarding the functionality of this patch. As I already said, i couldn't really tested on the latest beta D7 version.

This bit looks wrong: $name = &drupal_static(__FUNCTION__);

I'm pretty sure that static cache needs to be an array mapping fbus to names. Otherwise you'll return the same name no matter which $account is passed in.

Yes! That is so true :). Attached is the fixed ver.

Didn't apply cleanly.

patch -p1 < cache_fb_usernames-1188256-15.patch
patching file fb.admin.inc
Reversed (or previously applied) patch detected! Assume -R? [n]
Apply anyway? [n] y
Hunk #1 succeeded at 392 with fuzz 1 (offset 10 lines).
patching file fb.module
Reversed (or previously applied) patch detected! Assume -R? [n]
Apply anyway? [n] y
Hunk #1 FAILED at 54.
Hunk #2 succeeded at 70 with fuzz 2 (offset 5 lines).
Hunk #3 FAILED at 1606.
Hunk #4 succeeded at 1804 with fuzz 2 (offset 29 lines).
2 out of 4 hunks FAILED -- saving rejects to file fb.module.rej

Hmmm... not sure I like that fix for drupal_static(). It would be more consistent with other modules to make the static an array (keys are fbu, values are name). Like the example on http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drupa...

What about this one? I've also cleaned it a little ... white spaces and so on. Should apply cleanly.

I've also manage to test it and it worked fine for me and by doing this I've also found out that this hook is called a lot of times if on the page you have nodes of different users. It will try to alter the username of each of the owners of those nodes. I got 8 calls on one page.

@SocialNicheGuru did you reversed the changes applied by the previous patches? First checkout a clean version of the module and than try to apply the patch. You can try with git apply -v /path/to/patch/cache_fb_usernames-1188256-15.patch while in the root of the module.

Did anybody tried the latest patch? I have it live for some time now and worked ok for me (although I've applied it on a previous dev release).

I haven't tested... but I looked it over and I like it. I especially like the static cache, because a really smart bit of code could pre-determine which names a page is going to need to display, then call the facebook batch API, to make just one call to facebook instead of many.

Will test when I have some time....

Updated the patch to work against beta3 and corrected it to actually alter the user name if configured so (it was only caching it and that's it ... Doh).

Rerolled against latest dev. Works wonders for my local dev environment. Pageload decreased from 17s to 6s. Still really bad, but it's getting better.

Something is wrong with the patch. It used to add a setting to control the username alter feature. now it's adding settings about locale info.

I think this updated patch reflects what this issue is about. The parts of the previous patch related to i18n, are they from another thread?

Another problem with this function is that it calls facebook even when the user names are human-readable. That is, a user might change their username from 1234...@facebook to "John Smith" and this alter function will still query facebook for a full name (because there is a row in the fb_user table).

Probably, it is worth doing a strpos to find "@facebook" in the name.

In your cases, when you go to admin/people/people, do you see human-readable names or do you see 1234...@facebook?

I disabled the unique user names. So it's kinda hard to tell...

Lukas,

Do me a favor if you would, add a clause like this to fb_username_alter():

  if (!strpos($name, '@facebook')) {
    // Only alter unique names, leave human-readable names alone.
    return;
  }

Try that right before the if ($fbu = fb_get_fbu($account)) line. And re-run your performance test. I'd like to know what difference it makes. This will have the side effect that two users with the same name on facebook will typically show different names on drupal. I.e. "John Smith" and "John Smith 2"

If this does improve performance, the two questions in my mind are (a) where best to put this code in fb_username_alter() maybe it should be the very first check, and (b) should this be a configurable behavior?

although #36 did not get a lot of review, I've pushed it to .dev.

I think this updated patch reflects what this issue is about. The parts of the previous patch related to i18n, are they from another thread?

I think it is related to this one http://drupal.org/node/1298460