The OAuth module in Drupal should serve as a conduit to the OAuth PHP library; it should not contain the OAuth code itself when that is available elsewhere. We should separate this out using libraries and new adapter classes, if need be.

CommentFileSizeAuthor
#7 oauth-1433274-req_problem.patch2.35 KBkotnik
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

steinmb’s picture

juampynr’s picture

Title: Decouple OAuth code from Drupal module via libraries » Decouple OAuth code from Drupal module via Libraries API
Version: 7.x-3.x-dev » 6.x-3.x-dev
Assigned: jobeirne » Unassigned
Status: Active » Needs work
steinmb’s picture

juampy, do you have time to sum up what in the 6.x branch that needs work?

best reg
Stein

juampynr’s picture

Sure. Basically its about making the same changes as the above commit on the 6.x-3.x branch. I started yesterday but found a couple of issues when testing it.

juampynr’s picture

Status: Needs work » Fixed
voxpelli’s picture

Um - the OAuth library was included in the module because it was patched to provide support for some things that hadn't yet been included in the upstream library and as far as I remember not everything has yet been that? Yet this module now points to the upstream library as the one to be included and thus breaks backwards compatibility with previous versions when it comes to eg. http://oauth.googlecode.com/svn/spec/ext/consumer_request/1.0/drafts/2/s... and likely also http://oauth.googlecode.com/svn/spec/ext/body_hash/1.0/oauth-bodyhash.html

kotnik’s picture

Regarding http://drupalcode.org/project/oauth.git/commitdiff/3c81a0c.

You can not do module_enable or depend on another module during the $phase=='install' in hook_requirements(). This is the phase during Drupal installation and it completely breaks it - and having OAuth where it should be is no way crucial to the installation of Drupal. This *must* go to $phase=='runtime'.

Attached a patch with the fix for this.

kotnik’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev
Status: Fixed » Needs review

Please consider adding patch in #7 in addition to the one in #2, since the change in #2 alone prevents installation of Drupal if you use a profile with OAuth.

juampynr’s picture

Great feedback folks. What I will do instead is to move the library we had inside the module to Github. That way, we can still patch it.

Done that, next step will be #1591692: Replace current OAuth library.

juampynr’s picture

Status: Needs review » Active

Gonna start with the following:

  1. Rervert commits regarding Libraries API in 7.x-3.x and move them to 7.x-4.x as this is a major change that would break existing sites upgrading automatically.
  2. Restore removed Oauth.php and move it to Github, so we can have control over it and apply already submitted patches. Updated hook_requirements and README.txt to reference this library instead of the Googlecode one at 7.x-4.x
  3. Backport steps 1 and 2 to 6.x-3.x and 6.x-4.x respectively.

Further work regarding changing to another library or writing a new one will be discussed at #1591692: Replace current OAuth library

juampynr’s picture

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

Reverted changes in 7.x-3.x: http://drupalcode.org/project/oauth.git/commitdiff/9f8ac3a

Branch 7.x-4.x is ready for #1591692: Replace current OAuth library

Backporting.

juampynr’s picture

Status: Active » Fixed
juampynr’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev
Status: Fixed » Active

Changed README and hook_requirements at 7.x-4.x so it references our library, which has been moved to Github. This OAuth.php is the one we have in the module at previous versions.

http://drupalcode.org/project/oauth.git/commitdiff/ef716bf

juampynr’s picture

Status: Active » Fixed

Applied kotnik's patch. Thanks!

http://drupalcode.org/project/oauth.git/commitdiff/4ee27bb

So this closes this issue. Here is a summary:

  • 7.x-3.x and 6.x-3.x libraries stays as they were before
  • 7.x-4.x uses Libraries API and the OAuth library has been moved to Github.

Status: Fixed » Closed (fixed)

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