[D7] Facebook Comments Block

+1

I did a code review and run an automated test.

The automated test shows some code standards issues.
Take a look at the report.

A suggestion would be to use the theme function with a template file for your HTML markup and pass the variables to it.

This way, users can easily edit the markup of the module when needed and it is better to split the logic and markup.

The function of the template file is to create a file with mainly HTML and to split the smart functionality from basic HTML markup.
Take a look at for example the page.tpl.php file from Drupal. You don't have to use print as the file is rendered in the theme function.

Also it is better to pass the variables to the theme function and not via a preprocess functions.
See the examples of hook_theme.

Not related to this module but to get a review bonus, you have to manually review a module. The one you have added to the issue summery was already fixed so it didn't need a review anymore.

Hello @mqanneh,

Here is my manual review :

As mentionned by @mvdve, you should send vars to your template through the theme() function, and change the hook_theme() like this :

<?php
/**
 * Implements hook_theme().
 */
function facebook_comments_block_theme() {
  return array(
    // You should rename your array key to : facebook_comments__block (2 underscores before 'block')
    'facebook_comments_block' => array(
      // You should rename your template file like this : facebook-comments--block.tpl.php.
      // And do the same for the 'template' value : facebook-comments--block.
      'template' => 'templates/facebook-comments-theme',
      'variables' => array(
        'facebook' => NULL,
      ),
    ),
  );
}

/**
 * Implements hook_block_configure().
 */
function facebook_comments_block_block_view($delta = '') {
  $block = array();
  switch ($delta) {
    case 'fb_comments':
      $theme_vars = array();
      global $base_url;
      $url = $base_url . '/' . current_path();

      $facebook_app_id = variable_get('facebook_comments_block_settings_app_id', '');
      $facebook_app_id_script = (($facebook_app_id != '') ? "&appId=$facebook_app_id" : '') . '";';

      $theme_vars['facebook']['facebook_app_id'] = $facebook_app_id;
      $theme_vars['facebook']['facebook_app_id_script'] = $facebook_app_id_script;
      $theme_vars['facebook']['data_attributes']['href'] = $url;
      $theme_vars['facebook']['data_attributes']['data-width'] = variable_get('facebook_comments_block_settings_width', '500');
      $theme_vars['facebook']['data_attributes']['data-numposts'] = variable_get('facebook_comments_block_settings_number_of_posts', '5');
      $theme_vars['facebook']['data_attributes']['data-colorscheme'] = variable_get('facebook_comments_block_settings_color_schema', 'light');

      $block = array(
        'subject' => t('Facebook comments'),
        'content' => theme('facebook_comments_block', $theme_vars),
      );
      break;
  }
  return $block;
}
?>

You can delete the template_preprocess_facebook_comments_block() function, it will be useless with the code above.
Then, you have to change your template file like this to make it more readable :

<div id="fb-root"></div>
<script>
window.fbAsyncInit = function() {
  FB.init({
    appId      : <?php print "'".$facebook['facebook_app_id_script']."'"; ?>,
    xfbml      : true,
    version    : 'v2.3'
  });
};

(function(d, s, id){
   var js, fjs = d.getElementsByTagName(s)[0];
   if (d.getElementById(id)) {return;}
   js = d.createElement(s); js.id = id;
   js.src = "//connect.facebook.net/en_US/sdk.js";
   fjs.parentNode.insertBefore(js, fjs);
 }(document, 'script', 'facebook-jssdk'));
</script>
<div class="fb-comments" <?php print drupal_attributes($facebook['data_attributes']); ?> ></div>

That's all for me, I hope it will help you :)

Regards.

You also need to change this :

<?php
/**
 * Implements hook_block_configure().
 * Change name of hook to hook_block_view()
 */
function facebook_comments_block_block_view($delta = '') {
 ?>

Hello,

I've checked your code and everything seems to be good for me.

Regards.

Checked the code and looks good.

Can you describe what the difference is between you module and these two:
Facebook Comments Social Plugin
Facebook Comments Box

Hello
It's a good small but nice useful module. I enabled it and tested it. However, the following I found while doing a manual review.

1. You are using some new variables in your hook_block_save() code. But the thing is if someone disables and uninstalls your module those variable will still persist in the database variable table. So, it's always better to add a .install file and use hook_install() & hook_uninstall() to set and delete variables initially while module is enabled or uninstalled respectively.
   Variables:
   • facebook_comments_block_settings_app_id
   • facebook_comments_block_settings_color_schema
   • facebook_comments_block_settings_width
   • facebook_comments_block_settings_number_of_posts
2. In your template file, the JavaScript that you mentioned can't it be written in a JS file and then included in footer?

I hope these make sense. Please let me know if you have any queries.

Thanks!

Hello,

I've tested your module, it's always good.
I suggest you a little improvement in your array declarations:

<?php 
/**
 * Implements hook_block_view().
 */
function facebook_comments_block_block_view($delta = '') {
  $block = array();
  switch ($delta) {
    case 'fb_comments':
      global $base_url;
      $url = $base_url . '/' . current_path();

      $facebook_app_id = variable_get('facebook_comments_block_settings_app_id', '');
      $facebook_app_id_script = (($facebook_app_id != '') ? "&appId=$facebook_app_id" : '');

      $js_vars = array(
        'facebook_app_id' => $facebook_app_id,
        'facebook_app_id_script' => $facebook_app_id_script,
      );

      $theme_vars = array(
        'facebook' => array(
          'data_attributes' => array(
            'href' => $url,
            'data-width' => variable_get('facebook_comments_block_settings_width', '500'),
            'data-numposts' => variable_get('facebook_comments_block_settings_number_of_posts', '5'),
            'data-colorscheme' => variable_get('facebook_comments_block_settings_color_schema', 'light'),
          ),
        ),
      );

      $block = array(
        'subject' => t('Facebook comments'),
        'content' => theme('facebook_comments__block', $theme_vars),
      );
      
      drupal_add_js(array('facebook_comments_block' => array('facebook_settings' => $js_vars)), array('type' => 'setting'));
      drupal_add_js(drupal_get_path('module', 'facebook_comments_block') . '/js/facebook_comments_block.js', array('scope' => 'footer', 'type' => 'file'));
      break;
  }
  return $block;
}
?>

Just tell me when you're done and I'm gonna revalidated your module.

Regards.

Thanks, the module is ok for me :)

@mqanneh, nice changes. RTBC++

Hi @mqanneh, I make some short code review, but I have found just minor notices:

facebook_comments_block.js
Is it really needed to don't use drupal js behavior system?
Will it works with blocks loaded by ajax?

facebook_comments_block.install

• 2 - you use diffent count of empty lines before file PHPDoc comment;
• 13/16 - in all places you use variables with default values, so it is really needed to pre-configure values of variables?

facebook_comments_block.module

• 69/70 - I think empty line shoul be placed here, like at 15 line;
• 39 - by default all form element are options, so I think you can remove this line;
• 108 - I suggest you to define all global vars at begining of function;
• 112 - seems one pair braces are extra;
• 134 - drupal js coding standards suggest you to use lowerCamelCase style for variables names.

Also, looks like project page is a bit short, please follow the instructions at https://www.drupal.org/node/997024 page.

Assigning to myself for next review that may be tonight.

+1

Review of the 7.x-1.x branch (commit b0e5661):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Manual Review :

1. Project page needs updation.See the tips for a good project page https://www.drupal.org/node/997024
2. Readme.txt does not follows the guidelines for in-project documentation and/or the README Template.
3. Whats the need to clear the cache in facebook_comments_block_install, facebook_comments_block_enable, facebook_comments_block_disable facebook_comments_block_uninstall Please add a comment.
4. hook_help is missing in module.It would be nice to add it.
5. Add the configure link of block in the facebook_comments_block.info file.

Rest seems good to me.No blocker found.
Assigning to mpdonadio to take a final look if he has time.

@mqanneh, a suggestion: you have used drupal_add_js() it wont work always.
We need to use #attached property to attach the assets
in a cache-friendly way. In D8 drupal_add_css(), drupal_add_js() and drupal_add_library()
are removed in favor of #attached, details here.

Below is the modified code, review it:

/**
 * Implements hook_block_view().
 */
function facebook_comments_block_block_view($delta = '') {
  $block = array();
  switch ($delta) {
    case 'fb_comments':
      $theme_vars = array();

      global $base_url;
      $url = $base_url . '/' . current_path();

      $theme_vars['facebook']['data_attributes']['href'] = $url;
      $theme_vars['facebook']['data_attributes']['data-width'] = variable_get('facebook_comments_block_settings_width', '500');
      $theme_vars['facebook']['data_attributes']['data-numposts'] = variable_get('facebook_comments_block_settings_number_of_posts', '5');
      $theme_vars['facebook']['data_attributes']['data-colorscheme'] = variable_get('facebook_comments_block_settings_color_schema', 'light');

      $block = array(
        'subject' => t('Facebook comments'),
        'content' => array(
          '#markup' => theme('facebook_comments__block', $theme_vars),
          '#attached' => facebook_comments_block_attach(),
          ),
      );
      break;
  }
  return $block;
}

/**
*function to attach js to prevent cache block problems
*/
function facebook_comments_block_attach() {
  $js_vars = array();
  $attach = array();
  $path = drupal_get_path('module', 'facebook_comments_block');
  // add js files
  $attach['js'] = array(
      $path . '/js/facebook_comments_block.js' => array (
       'type' => 'file',
       'scope' => 'footer',
      )
  );
  $facebook_app_id = variable_get('facebook_comments_block_settings_app_id', '');
  $facebook_app_id_script = (($facebook_app_id != '') ? "&appId=$facebook_app_id" : '');
  $js_vars['facebook_app_id'] = $facebook_app_id;
  $js_vars['facebook_app_id_script'] = $facebook_app_id_script;
  // add js settings
  $settings = array(
      'facebook_settings' => $js_vars,
  );
  $attach['js'][] = array(
    'data' =>  array('facebook_comments_block' => $settings),
    'type' => 'setting',
  );
  return $attach;
}

@mpdonadio correct me if I am wrong.

Sorry I accidentally removed issue summary.

Automated Review

Review of the 7.x-1.x branch (commit f307b99):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

FILE: ...mattd/Documents/Drupal/PAR/pareview_temp/facebook_comments_block.info
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 7 | ERROR | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: ...attd/Documents/Drupal/PAR/pareview_temp/js/facebook_comments_block.js
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 11 | ERROR | [x] Expected 1 newline after opening brace; 0 found
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/mattd/Documents/Drupal/PAR/pareview_temp/README.txt
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 51 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 227ms; Memory: 6.75Mb

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation. There are a few different modules that do similar things. This is no longer a blocking issue, though.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows / No: Does not follow] the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Not seeing anything. The drupal_attributes() sanitizes the block settings.

Coding style & Drupal API usage

(+) Your behavior should use the context and settings that have been passed in. It also needs .once() functionality to make sure it doesn't get called during AJAX.

$form['facebook_comments_settings']['facebook_comments_block_settings_width'] should have a validator on it. See element_validate_integer_positive

Avoid global $base_url. Use the API, `$url = url(current_path(), array('absolute' => TRUE));` should work.

(+) facebook_comments_block_block_view() shouldn't returned themed output. It should return a render array with the theme elements in it.

(+) Avoid drupal_add_js(); use #attached in the render array.

(+) facebook_comments_block_block_info() will need to define DRUPAL_CACHE_PER_PAGE as the block will vary page to page.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Thanks for your contribution, mqanneh!

I updated your account so you can 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 stay 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.