Related to #1569982: Integrate with Libraries API somewhat, 6.x-3.x has diverged from 7.x-3.x

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xurizaemon’s picture

git diff 6.x-3.x:twitter.lib.php origin/7.x-3.x:twitter.lib.php

xurizaemon’s picture

Status: Active » Needs work
+++ b/origin/7.x-3.x:twitter.lib.phpundefined
@@ -5,63 +5,19 @@
-class TwitterConf {
-  private static $instance;
-  private $attributes = array(
-    'host'     => 'twitter.com',
-    'api'      => 'api.twitter.com',
-    'search'   => 'search.twitter.com',
-    'tiny_url' => 'tinyurl.com',
-  );
-
-  private function __construct() {}
-
-  public static function instance() {
-    if (!isset(self::$instance)) {
-      $className = __CLASS__;
-      self::$instance = new $className;
-    }
-    return self::$instance;
-  }
-
-  /**
-   * Generic getter
-   *
-   * @param $attribute
-   *   string attribute name to return
-   * @return
-   *   mixed value or NULL
-   */
-  public function get($attribute) {
-    if (array_key_exists($attribute, $this->attributes)) {
-      return $this->attributes[$attribute];
-    }

should this be retained?

+++ b/origin/7.x-3.x:twitter.lib.phpundefined
@@ -5,63 +5,19 @@
-
+class TwitterException extends Exception {
   /**
-   * Generic setter
-   * @param $attribute
-   *   string attribute name to be set
-   * @param $value
-   *   mixed value
+   * Overrides constructor to log the error.
    */
-  public function set($attribute, $value) {
-    if (array_key_exists($attribute, $this->attributes)) {
-      $this->attributes[$attribute] = $value;
-    }
+  public function __construct($message = NULL, $code = 0, Exception $previous = NULL) {
+    watchdog('twitter', 'Unexpected error: @message', array(
+      '@message' => $message,
+    ), WATCHDOG_ERROR);
+    parent::__construct($message, $code, $previous);
   }
 }

this needs to stay

+++ b/origin/7.x-3.x:twitter.lib.phpundefined
@@ -92,7 +48,9 @@ class Twitter {
+    if (!empty($username) && !empty($password)) {
+      $this->set_auth($username, $password);

we still have old-style auth, presumably only for twitter-compatible use?

+++ b/origin/7.x-3.x:twitter.lib.phpundefined
@@ -176,17 +115,12 @@ class Twitter {
-    // If $values is null then there was an error with Twitter
-    // and it has has already been set with drupal_set_message().
-    if (is_array($values)) {
-      return new TwitterStatus($values);
-    }

this might want to stay, unsure how to repro?

+++ b/origin/7.x-3.x:twitter.lib.phpundefined
@@ -215,32 +149,16 @@ class Twitter {
-    try {
-      if ($use_auth) {
-        $response = $this->auth_request($url, $params, $method);
-      }
-      else {
-        $response = $this->request($url, $params, $method);
-      }
+    if ($use_auth) {
+      $response = $this->auth_request($url, $params, $method);
     }
-    catch (TwitterException $e) {
-      watchdog('twitter', '!message', array('!message' => $e->__toString()), WATCHDOG_ERROR);
-      drupal_set_message('Twitter returned an error: ' . $e->getMessage(), 'error');
-      return FALSE;
+    else {
+      $response = $this->request($url, $params, $method);

always auth now

+++ b/origin/7.x-3.x:twitter.lib.phpundefined
@@ -303,17 +229,10 @@ class Twitter {
-        $response_decoded = json_decode($response, TRUE);
-        if (isset($response_decoded['id_str'])) {
-          // if we're getting a single object back as JSON
-          $response_decoded['id'] = $response_decoded['id_str'];
-        } else {
-          // if we're getting an array of objects back as JSON
-          foreach ($response_decoded as &$item) {
-            $item['id'] = $item['id_str'];
-          }
-        }
-        return $response_decoded;
+        // http://drupal.org/node/985544 - json_decode large integer issue
+        $length = strlen(PHP_INT_MAX);
+        $response = preg_replace('/"(id|in_reply_to_status_id)":(\d{' . $length . ',})/', '"\1":"\2"', $response);
+        return json_decode($response, TRUE);

that's an older workaround for #985544 and should go

+++ b/origin/7.x-3.x:twitter.lib.phpundefined
@@ -303,17 +229,10 @@ class Twitter {
@@ -321,8 +240,7 @@ class Twitter {

@@ -321,8 +240,7 @@ class Twitter {
     if (is_null($format)) {
       $format = $this->format;
     }
-    $conf = TwitterConf::instance();
-    $url =  'http://'. $conf->get('api') .'/1/'. $path;
+    $url =  variable_get('twitter_api', TWITTER_API) .'/1/'. $path;
     if (!empty($format)) {
       $url .= '.'. $this->format;
     }
@@ -349,12 +267,21 @@ class TwitterOAuth extends Twitter {

@@ -349,12 +267,21 @@ class TwitterOAuth extends Twitter {
     }

configuration API version, not sure if there's an issue for this. makes sense if we have a Twitter class which speaks to multiple API versions at once? or...?

+++ b/origin/7.x-3.x:twitter.lib.phpundefined
@@ -553,7 +482,7 @@ class TwitterUser {
   public function get_auth() {
-    return array('oauth_token' => $this->oauth_token, 'oauth_token_secret' => $this->oauth_token_secret);
+    return array('password' => $this->password, 'oauth_token' => $this->oauth_token, 'oauth_token_secret' => $this->oauth_token_secret);

probably we don't need $this->password there any more?

xurizaemon’s picture

tested 7.x-3.x twitter.lib.php in 6.x-3.x, it doesn't authenticate to post tweets, prob need some additional updates to bring the two branches into sync

xurizaemon’s picture

6.x-3.x catches exceptions in Twitter::call().
7.x-3.x did not; instead it was catching them in twitter_post_node_insert().

I think the 6.x-3.x approach was more correct. Have improved on it slightly.

* In Twitter::status_update(), we do not return a TwitterStatus object unless twitter::call() was successful.
* Update other calls to $this->call() to behave similarly.
* In twitter_post_node_insert() we check the result of twitter_set_status() rather than using a try/catch block, because that happens at the API layer.

See 4bc89ca (7.x) and 71e5664 (6.x) for changes.

juampynr’s picture

Great work @grobot!

Could you have a look at https://github.com/juampy72/twitter-rest-php and tell me if there are any missing changes at the library? I made you a collaborator of the project so you have commit permissions there.

juampynr’s picture

By the way, the overall idea is that since March 2013 Twitter REST 1.0 will be turned off, meaning that all Twitter module versions prior to 7.x-4.x and 6.x-4.x won't work even though they use OAuth. This is the reason why I took the chance to upgrade the library and move it out of the module in order to be shared among the different Drupal versions (soon we will have a Drupal 8 branch) and even other Twitter modules.

xurizaemon’s picture

No objection to making twitter.lib.php available elsewhere under a more permissive license, but I don't see any advantage in requiring people to install it separately. IMO this complicates the install process (already hard enough for users), and without any benefit for users of twitter module.

My recommendation is to keep the library included in the module in 7.x-4.x. I commented on this previously in #1569982-6: Integrate with Libraries API, re-posting here for discussion -

Updated install docs and removed Libraries reference to simplify install process there.

@juampy, above and in comment to 21dc9b7 you said this is on your roadmap for 7.x-4.x? If so I'd be keen to hear the advantages you see to splitting the library offsite. I see some disadvantages to hosting twitter.lib.php elsewhere -

  • Additional install/upgrade step for each site (20K+ install base currently)
  • Issues like #1814610: Use proper API URLs can't be handled here. (Github twitter.lib.php is six months / two commits behind.) EDIT: now resolved, but you get the idea ...
  • Adds two additional dependencies (Libraries module, twitter.lib.php)
  • Introduces potential failure points (wrong lib version installed, improper installation, Github outages)

twitter.lib.php contains Drupal specific functions so is unlikely to be re-used by external projects, and AFAIK there are no copyright reasons to move it away. IMO git makes it easy to keep twitter.lib.php updated across branches.

juampynr’s picture

Version: 6.x-3.x-dev » 6.x-5.x-dev
Status: Needs work » Fixed

twitter.lib.php has been reincorporated into the module and now matches the one at 7.x-5.x

Status: Fixed » Closed (fixed)

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