The way Twitter Sign in handles the Twitter response is incomplete. In my own implementation, I've had a couple of times that Twitter sent back an empty response - or at least a response that doesn't contain any user data.

The current code doesn't handle this possibility. In my implementation I put the code within a try/catch statement to catch this:

try {
  $account = user_save('', $edit);
} catch (Exception $e) {
  watchdog('oauth_login', "Exception saving new account: ".$e->getMessage(),'',WATCHDOG_WARNING);
  return false;
}
CommentFileSizeAuthor
#16 twitter-n652680-16-6.x-5.x.patch3.61 KBDamienMcKenna
#15 twitter-n652680-15-7.x-6.x.patch3.79 KBDamienMcKenna
#13 twitter-n652680-13-7.x-5.x.patch3.78 KBDamienMcKenna
#1 652680_twitter_error_handling.patch740 bytessteinmb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

steinmb’s picture

steinmb CreditAttribution: steinmb commented
Status: Active » Needs review
FileSize
652680_twitter_error_handling.patch740 bytes

Yeah it is important that we catch this before passing them on. Twitter.com might be overloaded, network problems etc. Rerolling without doing a lot of testing just to get this issue rolling again. It is hard to belive that it's been not a single comment since it was posted back in 2009.

13rac1’s picture

13rac1 CreditAttribution: 13rac1 commented
Version: 6.x-3.x-dev » 6.x-5.x-dev
Status: Needs review » Needs work

Applies with fuzz to 6.x-5.x. Should ported to 7.x?

$ git apply 652680_twitter_error_handling.patch 
error: patch failed: twitter_signin/twitter_signin.module:159
error: twitter_signin/twitter_signin.module: patch does not apply
$ patch -p1 < 652680_twitter_error_handling.patch 
patching file twitter_signin/twitter_signin.module
Hunk #1 succeeded at 154 with fuzz 2 (offset -5 lines).
13rac1’s picture

13rac1 CreditAttribution: 13rac1 commented
Status: Needs work » Needs review
xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
CreditAttribution: xurizaemon commented
Version: 6.x-5.x-dev » 7.x-5.x-dev
Issue summary: View changes

Duplicated by #1047262: Twitter does not hanlde app access deny case (patch there also). Should be re-rolled for 7.x-5.x.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
CreditAttribution: xurizaemon commented
Status: Needs review » Patch (to be ported)
steinmb’s picture

steinmb CreditAttribution: steinmb commented

Did this patch make it into 6.x?

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
CreditAttribution: xurizaemon commented

Doesn't look like it (http://drupalcode.org/project/twitter.git/blob/refs/heads/6.x-5.x:/twitt...) and I hope it would but ideally now coming in via 7.x-5.x first.

https://api.drupal.org/api/drupal/modules%21user%21user.module/function/...

does user_save really throw an exception in d6?

I don't think it does, so that patch feels like it might be improved to identify the cause of the exception here. Oh right, it will be from the SQL insert?

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
CreditAttribution: xurizaemon commented

(Sorry, on mobile. Am trying to wrestle this queue into shape - if this patch is good enough to go in, help me better understand the details please.)

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Patch (to be ported) » Needs work
Parent issue: » #2402307: Plan for Twitter v7.x-5.9 release
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 652680_twitter_error_handling.patch, failed testing.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

Does this problem still happen in the 7.x-5.x or 7.x-6.x branches?

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs work » Needs review
FileSize
twitter-n652680-13-7.x-5.x.patch3.78 KB

Yes, this could still be a problem with the current version.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Related issues: +#2402311: Plan for Twitter v6.x-5.2 release, +#2402309: Plan for Twitter v7.x-6.0 release
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Version: 7.x-5.x-dev » 7.x-6.x-dev
FileSize
twitter-n652680-15-7.x-6.x.patch3.79 KB

Ported to 7.x-6.x.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Version: 7.x-6.x-dev » 6.x-5.x-dev
FileSize
twitter-n652680-16-6.x-5.x.patch3.61 KB

Ported to 6.x-5.x.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs review » Fixed

Committed. Thanks!

  • DamienMcKenna committed 4004563 on 6.x-5.x authored by steinmb 
    Issue #652680 by steinmb, DamienMcKenna: Don't assume user registration...

Status: Fixed » Closed (fixed)

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