In 7.x-4.x twitter.lib.php has been removed from the module. Rather than simply installing the module, users must now additionally install the Libraries module and obtain the twitter.lib.php which is hosted offsite on Github.

I think this complicates the install process, when the twitter.lib.php could easily be dual-licensed MIT/GPL and restored to the module codebase. Doing so would simplify the installation, reduce points of failure, and reduce the amount of support traffic we see in the issue queue.

I don't understand the reasons for removing twitter.lib.php from the module, so I'm submitting a patch for its restoration, and hoping this leads to some discussion.

Comments

Status:Active» Needs review
StatusFileSize
new41.77 KB

Basic patch to restore twitter.lib.php and remove the additional requirements that its removal created.

Related discussions ...

* #1569982: Integrate with Libraries API
* #1820292: Update twitter.lib.php to match 7.x-3.x in 6.x-3.x
* #1836014: Upgrade REST API to 1.1
* #1853710: REST library can't be deployed inside a install profile

As stated in a couple of the above issues -

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.

My underlying logic, though, is: it's just easier on our users if we don't move twitter.lib.php out.

Why make thousands of sites perform an extra installation task, when tracking library updates internally is a single Git command at our (maintainers') end?

You can't include MIT licensed code in the module distribution and the original work is by James Walker (@walkah) as stated in the file so it's not juampy's decission.

In case this issue can't be resolved the library integration should be done right
http://drupal.org/node/1853710

Hence my suggestion of dual license. MIT is GPL-compatible?

@walkah's code was contributed to d.o and GPL; half a dozen other d.o contributors (including myself) have code in there also.

@juampy's rewrite is MIT. I'm asking if he will consider dual-licensing it GPL for the purposes of keeping things simple and our userbase sane.

Using libraries allows Twitter related modules to reuse it, plus reduces our work as maintainers as we only need to keep one library instead of 2 (and soon 3) versions of it. The downside is that it adds an extra step in the configuration, but apart from the problem with the installation profile that @rodrigoaguilera mentioned today, I have not seen any issues created related with this step, correct?

Since March 2013 all the Twitter related module will need to either implement their own library using OAuth or reuse Twitter module's one, so I thought that separating it from the module would make it easier for them.

There is an alternative of creating twitter_rest module as a separate project and make it a dependency of the Twitter module. However this is the reason why the Libraries module was created (to avoid having so many API modules).

I am a bit confused about which path to follow at the moment, but I need further discussion before reincorporating the library back to the module.

Ok, now I undertand better.

My choice would be to keep it as it is. Additional steps:
- resolving the issue with multisite and instalation profiles :)
- provide a drush command to download the library like the colorbox module for example
- I don't know if drush commands work with disabled modules, in case they don't let the user enable the module without the library and then show errors or messages until the library is properly installed just like the colorbox module does.

Thanks for having this conversation, juampy. I see also that some discussion was recorded here.

Using libraries allows Twitter related modules to reuse it,

Existing modules already re-use the Twitter library and Twitter functionality (eg OpenID Selector module reuses Twitter support from Twitter module).

I understand that adding Libraries to Twitter means that other modules can use the library without installing Twitter module, but I'm not sure how major an advantage that is.

plus reduces our work as maintainers as we only need to keep one library instead of 2 (and soon 3) versions of it.

We disagree on this point. I think it's easier to maintain a single file across three branches of a Git repository than it is to maintain the same file in a separate repository on a separate site (case in point that when I raised this issue, the two repos had fallen out of sync for some months).

I also think that it's significantly easier for our users to keep it all together, and for a module with 20K users I think that's more important than the maintainer saving a git command or two.

Since March 2013 all the Twitter related module will need to either implement their own library using OAuth or reuse Twitter module's one, so I thought that separating it from the module would make it easier for them.

It might, but it seems significant that most of the Twitter related modules have install rates of 150 or lower, and Twitter module is reporting 20K sites ...

I'm not confident that the maintainers who duplicated Twitter functionality in separate modules will be any more amenable to reusing a shared library than they were to contributing to this module, and again I'm not sure that the utility of this change benefits the wider userbase.

I support other modules using your new Twitter library from this project, but I think we can find a better way to do it than moving the library out of this project and off to Github.

There is an alternative of creating twitter_rest module as a separate project and make it a dependency of the Twitter module. However this is the reason why the Libraries module was created (to avoid having so many API modules).

I agree, and as we've seen separately with OAuth module, this can add confusion. If we had performance concerns about Twitter module being too heavyweight, I'd recommend restructuring so that Twitter module was the bare library and we had submodules for Twitter User Account, Twitter Post, Twitter Views etc.

- provide a drush command to download the library like the colorbox module for example

This would be great, but again it doesn't really help the wider userbase. (People still install stuff via FTP, right?)


Just to be clear - I'm really delighted with your library rewrite, and myself prefer Github's interface to Drupal.org. My concern here is mostly that we're complicating the process for our users for limited developer benefit.

Status:Needs review» Needs work

OK as this library is so Drupal dependant (depends on OAuth, uses variable_get(), uses drupal_http_request()...) I am up for restoring the library (and let you, @grobot, the task of reincorporating it). It should not be harmless to carry out the work in this branch as we will just remove dependencies.

This is, off the top of my head, what needs to be done (numbers refer to 6.x-4.x and 7.x-4.x branches).

* Remove libraries dependency and review hook_requirements (6, 7).
* Add twitter.lib.php in twitter.info (7).
* Rewrite the twitter_get_libraries() statement in twitter.inc by require('twitter.lib.php') (6).
* Rename Twitter6() by Twitter and merge the existing twitter.lib.php that overrides the doRequest method into class Twitter. This was done because drupal_http_request() has a different signature in Drupal 7.
* Get the latest version of the library from github and merge it back into the module (6, 7).
* Update README.txt (6, 7).
* Test adding accounts, pulling tweets, posting, posting through actions and rules and sign in (6, 7).
* Create a new release (6, 7). There is currently a showstopper for 6 related with posting using Rules (it works, but it raises warnings).

@grobot, it would be great if you can work on the above, as I would like to keep on working on unlink-users branch.

@grobot, let's move the library to a submodule called twitter_lib. If we do it this way, we can allow other Twitter modules to download Twitter and just enable twitter_lib so they do not need to hide/deactivate the Twitter module hooks.

@juampy, good plan

Please mention on the main plugin page that it requires twitter library and where it can be obtained from.
I did upgrade of this plugin in one of our sites and it went without any issues. No problems until I logged out. I could not loggin after that - WSOD. Logs showed me a Fatal error when trying to include a file libraries/twitter/twitter.lib.php.

I am sorry @vbalsys, but it is not recommended to upgrade modules in production sites without previously reading the Release Notes.

Version:7.x-4.x-dev» 6.x-4.x-dev

Restored twitter.lib.php for 7.x-5.x.

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

Will create a branch 6.x-5.x as a backport of 7.x-5.x.

Started to backport 7.x-5.x into 6.x-5.x.

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

Twitter authenticated and non-authenticated accounts can be added at admin/settings/twitter.

Pending to review how tweets are pulled on cron run and then listed. After that, review the submodules which have already code from Drupal 7.

StatusFileSize
new60.42 KB

Here is an attachment of how it looks like so far.

twitter_admin_6.png

Tweets can now be listed at /tweets and be filtered at /tweets/screen_name.

This work is tightly related with #1847406: Move account listing to the module settings. I am just double linking these two issues here.

Version:6.x-4.x-dev» 6.x-5.x-dev

Reviewed twitter_signin. There is a bug which I registered at #1875966: Logged in users through twitter_signin are not redirected back to where they were.

I will continue porting twitter_actions and twitter_post submodules.

Status:Needs work» Fixed

Reviewed twitter_actions submodule. Now pushing a beta release for Drupal 6.

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

Updated issue summary.