Closed (won't fix)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
22 Jan 2012 at 09:26 UTC
Updated:
31 Aug 2012 at 10:31 UTC
Jump to comment: Most recent file
Comments
Comment #1
michaelmol commentedIt appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.
Comment #2
michaelmol commentedComment #3
shlapa commentedThank you for review.
I tried to fix most of your suggestions:
Comment #4
shlapa commentedComment #5
michaelmol commentedPlease check release naming conventions againg. Your branch should named 6.x-1.x (and not the addition of dev).
In addition to that please make sure that automated tools for checking you code gives out clean results, it will help reviewers to do a manual review.
Review of the 6.x-1.x-dev branch:
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.
You can repeat this review at: http://ventral.org/pareview/httpgitdrupalorgsandboxshlapa1412518git-6x-1...
Comment #6
michaelmol commentedComment #7
shlapa commentedOK, i removed file with third-party library so users will have to upload it manually.
Also fixed branch and code style issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxshlapa1412518git-6x-1x
Please review.
Comment #8
tonybuckingham commentedAfter enabling the module, and entering my application "key" and "secret", I clicked on the "this" link to "request security token from LinkedIn". This resulted in a blank page. Checking the error logs showed that php had encountered a fatal error.
I can see where you are "throwing" this error, but since you are not "catching" it anywhere, php interprets this as a fatal error. The appropriate behavior when using php's Exception model is to use them with "try-catch" logic. A more user friendly way to handle the missing OAuth library would be to actually report the error to the user instead of letting php trap a fatal error. For example:
Line 240, linkedin_api.module:
You should "catch" the Exception potentially thrown by the "init" function:
You should use this method anywhere you might throw an Exception.
Line 216, linedin_api.module
The else clause for this "if" is:
Unable to find OAuth.php library
It should actually be something like:
The Libraries API module is not installed
While it's certainly a good idea to check for the existence of the "libraries_get_path" function, I would also include a dependency on the "Library API" module in your .info file:
Line 218, linkedin_api.module:
If the path doesn't exist, require results in a fatal error:
PHP Fatal error: require() [function.require]: Failed opening required 'sites/all/libraries/oauth/OAuth.php'
Comment #9
klausiClosing due to lack of activity. Feel free to reopen if you are still working on this application.