[D7] FB Live

You should run the module through coder (install the coder module with drush dl coder) and review your module (click on the review link on the modules page of your Drupal installation). Coder reports the following:

Coder found 1 projects, 2 files, 1 critical warnings, 13 normal warnings, 12 minor warnings, 0 warnings were flagged to be ignored

Don't forget to set Coder to use the most strict setting, so it will report everything.

The critical warning (fb_live.module):

Line 132: Potential problem: confirm_form() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs)

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./README.txt:      ASCII English text, with CRLF line terminators
  README.txt
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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.

I've attached the report.

In the .module file:
Line 50 you implemented hook_form_alter, but your function name is

/**
 * Implementation of hook_form_alter
 */
function fb_live_form($form, &$form_state, $fb_app) {
...

Line 136 function _flip_array($array). You should namespace your function in order to avoid name collisions. I'm sure there is a drupal function that does exactly this, but I just can't remember the name.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Suggestions:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/asavin/1460432.git fb_live

put this command in your issue summery, as the other one asks for a password and the reviewers may thing something else and skip reviewing the project.

http://ventral.org/pareview/httpgitdrupalorgsandboxasavin1460432git

Though you have mentioned, I would suggest you to fix these errors before applying for review bonus tag, as this defenitely will be pointed out while reviewing your project.

FILE: ...-pareview/sites/all/modules/pareview_temp/test_candidate/fb_live.module
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
265 | ERROR | Function comment short description must be on a single line
337 | ERROR | Function comment short description must be on a single line
337 | ERROR | Function comment short description must end with a full stop
-------------------------------------------------------------------------------

Also it looks like you are setting a variable while installing and not deleting it while uninstalling the module.

Thanks

Regarding this:

Also it looks like you are setting a variable while installing and not deleting it while uninstalling the module.

I see that there are 4 variables created on .module, but just one deleted on un-install. Those 4 variables shouldn't be deleted too ?

A few suggestions I have:

1) When you install this module, it's not really clear which it is out of the many Facebook modules that the fb module provides. I might suggest you name the module in the .info file something like "Facebook Live Updates" as anyone would likely just search for "facebook live" on the modules page to find it.

Otherwise, you should probably put what the module is actually named in the README file, rather than saying just "-Copy the module into the appropriate modules subdirectory and enable it.".

2) I might suggest actually putting it in its own fieldset on the modules page so it's not burried amidst all the fb modules - as although it does expand on the fb module, it is a different module obviously. But point #1 would likely be enough.

3) I notice you have several misspellings in your text. I found some on lines 78, 79 and 109, but there may be more. You may want to do a spell check.

4) You may want to say in the README that your module requires the fb module. Perhaps you could make it the first step in your "Installation and configuration". (-make sure you have the FB module installed: http://drupal.org/project/fb)

5) In general, it's not good security practice to ever have 'access callback' => TRUE in your hook_menu. It's the same thing as giving it a permission and then granting that permission to anonymous (put in your README) and gives you the option in the future to restrict this if the user should ever see fit. I understand that in this case you may override me on that as it's supposed to listen, but just throwing that out there.

Nice work on the module!

There are some coding standard mistakes you have done http://ventral.org/pareview/httpgitdrupalorgsandboxasavin1460432git

You could use some comments in the module. For example:

function _fb_live_flip_array($array) {
  foreach ($array as $key => $val) {
    $array[$val] = $val;
    unset($array[$key]);
  }
  return $array;
}

You have given a description for this function, but that does not explains what this function actually does.

Also I have seen that you are using the PHP ternary if operator in module file. For example:

$print_subs['items'][$i]['children'][] = '<strong>' . $type . '</strong>:&nbsp;<em>' . (is_array($s) ? implode(', ', $s) : $s) . '</em>';

In module file you do not need to use ternary operators, AFAIK ternary operators reduces code readability and maintainability. Ternary operators may look simple and should not be used for complex conditions. Also if you want to see which is faster PHP ternary if operator or if-else condition refer this page

Looks like a great module. However, automated review issues are still outstanding: http://ventral.org/pareview/httpgitdrupalorgsandboxasavin1460432git.

Changing status to needs work.

I'm seeing an error:

DrupalSecure has found some issues with your code (please check the Writing secure core handbook).

FILE: /var/www/drupal-7-pareview/pareview_temp/fb_live.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
352 | ERROR | Printing unsanitized input from $_GET

How is setting a random value in the install file variable_set('fb_live_verify_token', $rand); useful?

You should have spaces between items in an array, especially in the giant DEFINEs at the top of the module. Speaking of which FB_PATH_ADMIN_APPS and FB_PATH_ADMIN_APPS_ARGS don't seem to be defined anywhere. You shouldn't call exit() in a drupal function - just drupal_not_found() and then returning from the function is sufficient. Can you use PHP's array_flip() instead of _fb_live_flip_array()?

Some of those aren't minor, like the missing defines, but there aren't any security problems or critical issues. Marking RTBC from me.

Thanks for those fixes, looks good to me. This issue has been waiting for quite some time, thanks for sticking with it.
Can you sanitize 352 echo $_GET['hub_challenge']; before printing it? Using check_plain()?

Thanks for your contribution, Alex Savin!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Enterprise modules from the community for the community.