GitHub Connect

Vicom,

Thanks for contributing this module. The code looks good and follows the coding standards, if I'm being picky there are a couple extra trailing spaces on some lines. You can see these using coder module set to minor.

One thing that I did notice was a questionable use of the _github_connect_save_github_user() function to merge an account that already exists. While I understand the idea to make it easier to merge the accounts, I see possible abuse with this functionality. Since the email that is sent when I connect is an email that I can easily change on my github account, I could potentially assign that address to another users email address and login with my github giving me full access to their account.

While I'm not sure the absolute best way to handle this (whether it's simply to return false and disallow the user to login via github or to require the user to provide their login password during the merge process) it is an issue that should be addressed before moving forward.

There are a couple error conditions that I've run into as well that leave the end-user a bit confused. These bugs I will open in the issue queue on your sandbox.

Finally, for usability sake, you might add the installation instructions referring to the "correct URLs" to the admin configuration page. With the ability to add a module on the module page, many people may not look for the readme.txt file itself to look for these details.

Thanks again for this contribution.

Vikom,

Great work here. I really like the way you added the password to verify the local account prior to the account merge. Yes I probably wouldn't have noticed the security issue if it weren't for the fact that I didn't have an email address in my public facing Github profile.

In Drupal 7, the ability to add a module to a system directly from the admin/modules page was added.
Install through the Drupal interface. While it does make it easier for people to add modules, it also makes it less likely they will read any accompanying docs such as the README.txt file.

Thanks for adding the help text on the admin page describing how the Github side needs to be configured.

I'm going to mark this as RTBC. Thanks again for this contribution.

It appears you are working in the "master" branch in git. You should ideally be working in a version specific branch. Please see the documentation about release naming conventions and creating a branch in git. This is a best practice but does not block a project application.

I noticed a couple of small code style issues, but none that should block an application. Things like:
* An extra newline between the docblock and the function.
* Functions without docblocks.

You mention that there is no module that provides this feature, but I believe the fundamental feature is OAuth login, right? There is the oauth connector module which provides the ability to login using any oauth based site.

Are there some features in this module that are specific to Github and would not be possible using OAuth Connector? Can you leverage the OAuth or OAuth connector modules and keep just the Github specific parts in this module?

I'm marking needs work to get an answer on the last piece about duplication but generally still think this should be RTBC once we find an answer to the overlap/duplication piece.

Thanks for your contribution, Vikom! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.