I've been poking around inside the module trying to debug some weird behaviour I was having and noticed there's a fair bit of cruft in the codebase, things like:

      if (TRUE) {
        $path = "https://graph.facebook.com/oauth/access_token?client_id=" . $app_id . "&client_secret=" . $fb->getApiSecret() . "&grant_type=client_credentials";
        //$http = drupal_http_request($path. array(), 'GET', NULL, 1, 5); // why doesn't this work???
        $http = drupal_http_request($path);
      }
      else {
        // This attempt to encrypt the secret does not work.
        $path = "https://graph.facebook.com/oauth/access_token";
        $args = array(
                      'client_id' => $app_id,
                      'client_secret' => $fb->getApiSecret(),
                      'grant_type' => 'client_credentials',
                      );
        $http = drupal_http_request($path, array('Content-Type' => 'application/x-www-form-urlencoded'), 'POST', http_build_query($args), 0, 5);
      }

"if (TRUE) {} else {}", really? in production code? also, the reason that your commented out drupal_http_request doesn't work is because you've used a "." instead of a "," to delimit the first two arguments - http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_h...

Leaving debug code in the public repository like this might help the module's maintainers keep track of things but it's a barrier for community developers who want to help by submitting patches and it's definitely not something you'd consider "best practice".

If you really want to make a point that a particular pattern of code doesn't do what you'd expect, leave a comment rather than using control structures like "else" that never fire.

The module maintainers should put their debug code in a separate git branch on their local machines, or similar and keep track of it there rather than pushing it back upstream.

Additionally, when I ran CodeSniffer using http://drupal.org/project/drupalcs it threw 205 coding standards errors on the fb.module file alone in both the latest stable and latest dev versions of the 6.x branch. Most of these are just related to poorly formatted comments but it would be worth giving the whole project the once over with CodeSniffer.

CommentFileSizeAuthor
#9 fb_friend_cleanup_d6-1776004-9.patch23.18 KBthedavidmeister
#9 fb_friend_cleanup_d7-1776004-9.patch23.1 KBthedavidmeister
#6 1776004-fb_example_cleanup_d6-6.patch8.79 KBthedavidmeister
#6 1776004-fb_example_cleanup_d7-6.patch8.79 KBthedavidmeister
#2 1776004-fb_registration_cs-2.patch5.37 KBthedavidmeister
#1 1776004-fb_module_cs-1.patch48.62 KBthedavidmeister

Comments

thedavidmeister’s picture

thedavidmeister commented
Status: Active » Needs work
StatusFileSize
new 1776004-fb_module_cs-1.patch48.62 KB

Here is a patch that addresses the complaints from CodeSniffer in fb.module without modifying any of the module's functionality and also removes "debug" code like the example I posted and "commented out" calls to dpm() and exit().

I noticed while doing this that there's a lot of "inline control structures" like this that aren't generally allowed:

  if($test)
    // Do stuff

All if/else statements need to be wrapped in curly brackets - http://drupal.org/coding-standards#controlstruct

Always use curly braces even in situations where they are technically optional. Having them increases readability and decreases the likelihood of logic errors being introduced when new lines are added.

There's quite a few more files to go, but this is a start :)

thedavidmeister’s picture

thedavidmeister commented
StatusFileSize
new 1776004-fb_registration_cs-2.patch5.37 KB

This patch gives the same treatment to fb_registration.module

Dave Cohen’s picture

Dave Cohen commented

Whoa, the period instead of comma was sloppy. Thanks for pointing that out.

The if (TRUE) is definately because I would prefer the code in the else clause, but couldn't get it to work. Sorry it upsets you. I hold on to the hope that someone out there will explain why it doesn't work. In general its been hard to keep up with the facebook APIs. I don't pretend that this production code is perfect.

I have made some effort to keep the code clean-ish. I've used coder. This is the first I've ever heard of CodeSniffer. If purely cosmetic changes like this are important to you, I'd like a corresponding patch for the 7.x patch, because without that it becomes complicated to merge changes between the branches, which has to happen pretty frequently.

thedavidmeister’s picture

thedavidmeister commented

Like i said, if you have code that you'd like help debugging because it mysteriously doesn't work then comment it out, don't use control structures for that task.

Even better, file an issue in the issue queue with the code you're trying to get working and actively encourage people to apply patches against that particular block of code.

I can see that the code you've written seems to follow some internally consistent formatting for the most part, like you said, but the coding standards outlined at http://drupal.org/coding-standards definitely covers formatting that hasn't been applied in this project. Regardless of how strongly I personally feel about cosmetic changes to code, I'm not wrong in saying that this project is written in a way different to what d.o. expects, and so I did the logical thing and opened an issue for it as a task :)

Coder is good but obviously it has missed some things in this project, there's an online automated code review that you can use at http://ventral.org/pareview

These days, new modules from developers that are still going through the manual approval process generally don't get approved unless the ventral checklist has been worked through so it's a good reference.

I've generated a review of the 6.x-3.x branch and quite a few critical errors, including security warnings were thrown as well as the cosmetic stuff - http://ventral.org/pareview/httpgitdrupalorgprojectfbgit-6x-3x

The 7.x-3.x branch has fewer (but not zero) critical and security errors but there seems to be a similar number of cosmetic issues with the code - http://ventral.org/pareview/httpgitdrupalorgprojectfbgit-7x-3x

I'm happy to submit patches for you to help get this cleaned up as it's certainly a bit of work, but only if there's a good chance they'll actually be applied.

thedavidmeister’s picture

thedavidmeister commented

If you're concerned about keeping D6 and D7 in sync (which is a good thing) then maybe we should do this differently to the other patches I submitted. We could do this one "type" of issue at a time, like all the unbracketed "if" statements for the whole project in both versions, then the comments, then looking into the security warnings, etc...

thedavidmeister’s picture

thedavidmeister commented
StatusFileSize
new 1776004-fb_example_cleanup_d7-6.patch8.79 KB
new 1776004-fb_example_cleanup_d6-6.patch8.79 KB

Ok, so tbh I'm feeling a bit overwhelmed trying to do the whole project and keeping track of what I'm doing without access to push upstream to git for this project simply because there's just so much code here. I'd really like to submit patches in stages and work on another section as each pair of patches is committed if that's ok with you Dave?

Here's two patches for fb_example mostly dealing with incorrectly formatted comments and arrays. If/when these patches get applied I'll checkout the latest dev of 6.x-3.x and 7.x-3.x and run some more files through CodeSniffer :)

Dave Cohen’s picture

Dave Cohen commented

Pushed the #6 patches. Thanks for doing both branches.

I tried to get phpcs set up. If I could make it part of my devel process I'd use it. Unfortunately I couldn't get it to work on my arch linux dev box. Gives open_basedir error and I could not change my allowed paths in any way that would make it work.


[dave@apollo fb-3]$ phpcs 
PHP Warning:  is_file(): open_basedir restriction in effect. File(/usr/bin/../CodeSniffer/CLI.php) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/bin/../CodeSniffer/:/usr/bin/CodeSniffer/) in /usr/bin/phpcs on line 28
PHP Stack trace:
PHP   1. {main}() /usr/bin/phpcs:0
PHP   2. is_file() /usr/bin/phpcs:28
thedavidmeister’s picture

thedavidmeister commented

hrmm, I got it to work on my macbook doing something like this (the Cellar path for php is something that homebrew does):

$ sudo pear install PHP_CodeSniffer
$ git clone --recursive --branch 7.x-1.x http://git.drupal.org/project/drupalcs.git /usr/local/bin/drupalcs
$ sudo ln -sv /usr/local/bin/drupalcs/Drupal $(pear config-get php_dir)/PHP/CodeSniffer/Standards
$ sudo ln -s /usr/local/Cellar/php/5.3.8/bin/phpcs /usr/local/bin/phpcs

I use the Sublime 2 text editor and the Sublime linter package I installed really didn't need much extra configuration to work after that point.

thedavidmeister’s picture

thedavidmeister commented
StatusFileSize
new fb_friend_cleanup_d7-1776004-9.patch23.1 KB
new fb_friend_cleanup_d6-1776004-9.patch23.18 KB

Cleanup for fb_friend contrib module attached for both D6 and D7.

I'll work on fb_permission once this is committed.

thedavidmeister’s picture

thedavidmeister commented

bump on getting patches in #9 committed?

thedavidmeister’s picture

thedavidmeister commented
Status: Needs work » Needs review

any specific reason #9 isn't getting in?

Dave Cohen’s picture

Dave Cohen commented
Component: I am lazy and not paying attention » Code
Assigned: Unassigned » Dave Cohen

Because I let it fall down so far in the issue queue that it wasn't on the front page and I just lost track! Sorry and stay tuned.

thedavidmeister’s picture

thedavidmeister commented

Will do. I might turn this into a meta issue actually, and create issues for each sub-module that needs cleanup.