At the moment, the twitter module uses a hardcoded variable for defining the twitter host, making impossible to use a different service, for example for testing purposes. I will attach a patch for using a Drupal variable.

CommentFileSizeAuthor
#9 host_and_protocol_configurable-1346790-9.patch2.51 KBsjovanig
#8 twitter_oauth_missing_server.patch401 bytesjose reyero
#6 1346790_add_host_for_oauth_authentication_3.patch821 bytesjuampynr
#3 use-variables-for-hosts-backport-D6-1346790-3.patch1.98 KBsjovanig
#1 use-variables-for-hosts-1346790-1.patch2.48 KBDavid Hernández

Comments

David Hernández’s picture

David Hernández commented
Assigned: David Hernández » Unassigned
Status: Active » Needs review
StatusFileSize
new use-variables-for-hosts-1346790-1.patch2.48 KB

Patch attached

AlessMascherpa’s picture

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

Tests passed ;)

sjovanig’s picture

sjovanig commented
Title: Change hardcoded host to use a drupal variable » Backported to D6
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new use-variables-for-hosts-backport-D6-1346790-3.patch1.98 KB

Backported to D6

jjcarrion’s picture

jjcarrion
Primary language Spanish
Location Spain
commented
Status: Needs review » Reviewed & tested by the community

test passed

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Title: Backported to D6 » Change hardcoded host to use a drupal variable
juampynr’s picture

juampynr commented
Version: 6.x-3.x-dev » 7.x-3.x-dev
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 1346790_add_host_for_oauth_authentication_3.patch821 bytes

The patch given at http://drupal.org/node/1346790#comment-5266844 caused that a user was unable to add a Twitter account using OAuth. The attached patch fixes this issue.

juampynr’s picture

juampynr commented
Status: Needs review » Needs work
Issue tags: +needs backport to 6.x

Fixed and applied to 7.x branch.

http://drupalcode.org/project/twitter.git/commitdiff/a9f03daf0e2c17a1cf3...

jose reyero’s picture

jose reyero commented
Priority: Normal » Major
StatusFileSize
new twitter_oauth_missing_server.patch401 bytes

This introduced a bug, now TwitterOauth can't find the server because the constructor for this class is missing a call to the parent's contructor

sjovanig’s picture

sjovanig commented
Status: Needs work » Needs review
StatusFileSize
new host_and_protocol_configurable-1346790-9.patch2.51 KB

I think introducing variable_set() and variable_get() inside Twitter class is not a good idea.

For abstraction questions the best solution is to use setters and getters to configure host or another class variable.

The patch I attach does:

  1. Provides default value for $host variable. This avoid to call parent::__construct() in each extender class.
  2. Removes calls to variable_set() and variable_get(). e.g. in tests you can set host easily through $instance->set_host()
  3. Introduces a new variable: $protocol. I think this is required, not always will be 'http' and now it can be set with set_protocol() and get_protocol()
juampynr’s picture

juampynr commented
Status: Needs review » Fixed

Finally did a mix of all suggestions. Now there is a TwitterConf class which implements the Singleton pattern. This allows us to change the host to a fake one during, for example, automated tests. Each test suite would have at the top the following code:

  $conf = TwitterConf::instance();
  $conf->set('api', 'twitter.localhost');
  // From now on, calls to the Twitter API will be made to the new host set

Commit for the Drupal 6 version.

Commit for the Drupal 7 version.

Please reopen if there is anything missing regarding the issue title.

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