Gigya [gigya]

Attached is my zipped module code.

1. The name of the constants is not correct; the namespace of the module is not respected. I don't find the utility of defining a constant that has as value its name.
2. <?php
/**
*
*/
function gigyaToolbar_admin_form() {
  include_once($base_path .'modules/user/user.pages.inc');
?>

   There is a better way to include a PHP file; anyway, where is the variable $base_path defined?

3. <?php
    '#description' => t('If you want to track analytics for your Toolbar, you must first sign up for a Gigya account <a href="https://www.gigya.com/site/Register.aspx">here</a> and enter your Gigya partner ID where it says "Your Gigya ID" above.<br/>You can find your Partner ID in the <a href="https://www.gigya.com/site/partners/updatedetails.aspx">account setting page</a>. This allows Gigya to track your toolbar performance and pay you when users search from your toolbar.'),
?>

   The translatable string includes the URL; it would be better to use a placeholder.

4. <?php
    '#description' => t('To add a button that allow users to subscribeto your RSS feed, enter your RSS URL where in says "RSS URL" above.'),
?>

   subscribeto? The full sentence needs to be rewritten.

5. The form fields descriptions are using <br /> they should not use.
6. See the Drupal coding standards to understand how a module code should be written.

1. <?php
    '#title' => t(GIGYATOOLBAR_CUSTOMIZE_THE_TOOLBAR),
?>

   The first argument of t() must be a literal string, not even a constant.

2. <?php
    '#description' => nl2br(t('If you have a page on Facebook, you can add a button that when clicked, shows your page updates and fans and lets users become fans of your page. ' . chr(13) . 'To add the button, go to your page on Facebook and grab the page ID which appears at the end of the URL. ' . chr(13) . 'Example: '. l(GIGYATOOLBAR_FB_BARISTA_TEXT,GIGYATOOLBAR_FB_BARISTA_URL) . '' . chr(13) . 'Enter this ID (116090169739 in this example) where it says "Fan Page ID" above.')),
?>

   There is no need to add <br />; the text is automatically formatted from the theme used.
   l() is not used together t(); that is reported in the documentation page for t(), which reports the following text:

   Here is an example of incorrect usage of t():

   <?php
$output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));
?>

   
   Here is an example of t() used correctly:

   <?php
$output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';
?>

3. <?php
    '#title' => t('Default Status Message'),
?>

   In Drupal, only the first word is in capital case; the other words (if they are not an acronym, or a proper noun) are written in lower case.

4. <?php
    '#required' => FALSE,
?>

   There is no need to use that value, as it is the default one.

5. <?php
function gigyaToolbar_schema() {
  $schema['gigyaToolbar'] = array(
    'description' => t('gigya per-user settings.'),
?>

   Schema descriptions should not be passed to t() anymore.

6. The module namespace is not respected.
7. <?php
    require_once('gigyaToolbar.admin.inc');   
?>

   There is a better way to include PHP code, in Drupal.

Fixed it all.
Please review and let me know.

Thanks a lot for your help.

The Drupal variables used from the module still use a name that is not respect the namespace of the module; some of the constant defined from the module are then not used anymore.

1. As I reported in comment #8, point 6, the namespace of the module is not respected.
2. The titles given to the blocks are too generic. They don't help the user who is viewing them, or the user who selects the blocks that he wants to appear in his web site.
3. <?php
        $blockContent .=' <script src=\'http://toolbar.cdn.gigya.com/toolbar.js\'></script>';       
        return t($blockContent);
?>

   The first argument of t() must be a literal string, not a dynamic string; apart that, why would you need to translate in another language what it seems to be JavaScript code?

4. <?php
            if ( 'Default Status Message' != $stat_msg && '' != $stat_msg) {
                $stat_msg = replaceWildCards($stat_msg,$current_title,$current_url,$current_site_name);
                $blockContent .=' var gs_statusMessage = \'' . $stat_msg . '\'; ' ;
            }
?>

   It's not clear the reason of the check done by the IF-statement; why doesn't the code call directly replaceWildCards() (what is the name of the module?)?

5. <?php
function variable_exists($name) {
    global $conf;
    return isset($conf[$name]);
}
?>

   What is wrong with that code?

6. <?php
function save_configuration($val) {
    //drupal_set_message(t('save_configuration ' . GIGYATOOLBAR_PARTNER . ' ' . variable_exists(GIGYATOOLBAR_PARTNER)));
    variable_set(GIGYATOOLBAR_PARTNER,$val[GIGYATOOLBAR_PARTNER_FIELD]);   
    variable_set(GIGYATOOLBAR_FACEBOOK_ID,$val[GIGYATOOLBAR_FACEBOOK_ID_FIELD]);
    variable_set(GIGYATOOLBAR_TWITTER_USERNAME,$val[GIGYATOOLBAR_TWITTER_USERNAME_FIELD]);
    variable_set(GIGYATOOLBAR_RSS_URL,$val[GIGYATOOLBAR_RSS_URL_FIELD]);   
    variable_set(GIGYATOOLBAR_STATUSMESSAGE,$val[GIGYATOOLBAR_STATUSMESSAGE_FIELD]);
    variable_set(GIGYATOOLBAR_SUBJECT,$val[GIGYATOOLBAR_SUBJECT_FIELD]);
    variable_set(GIGYATOOLBAR_BODY,$val[GIGYATOOLBAR_BODY_FIELD]);
   
    db_query("DELETE FROM {gigyaToolbar}");
    $insertStr = "INSERT INTO {gigyaToolbar} ";   
    $fieldsStr = "(" . GIGYATOOLBAR_PARTNER . "," . GIGYATOOLBAR_FACEBOOK_ID . "," . GIGYATOOLBAR_TWITTER_USERNAME . "," . GIGYATOOLBAR_RSS_URL . "," . GIGYATOOLBAR_STATUSMESSAGE . "," . GIGYATOOLBAR_SUBJECT . "," . GIGYATOOLBAR_BODY . ") ";
    $valuesStr = "VALUES ('%s', '%s', '%s', '%s','%s','%s','%s')";    
    db_query($insertStr . $fieldsStr . $valuesStr,$val[GIGYATOOLBAR_PARTNER_FIELD],$val[GIGYATOOLBAR_FACEBOOK_ID_FIELD],$val[GIGYATOOLBAR_TWITTER_USERNAME_FIELD],$val[GIGYATOOLBAR_RSS_URL_FIELD],$val[GIGYATOOLBAR_STATUSMESSAGE_FIELD],$val[GIGYATOOLBAR_SUBJECT_FIELD],$val[GIGYATOOLBAR_BODY_FIELD]);       
}
?>

   Why does the code use a database table to contain values that it deletes all times? A module settings should be saved in Drupal variables. Also, the SQL query is a concatenation of strings, which should be avoided (db_query() support the use of placeholders).

7. <?php
/**
* Implementation of hook_user().
*
* The main API for gigyaToolbar
*/
function gigyaToolbar_user($op, &$edit, &$account, $category = NULL) {
    //drupal_set_message(t('In gigyaToolbar_user=' . $op ));
}
?>

   If that is the main API for the module (which I take it is the most important function for the module), then you need to develop the module more, before we can review it.

8. <?php
function gigyaToolbar_form_alter(&$form, &$form_state, $form_id) {   
    if ( 'gigyaToolbar_admin_form' == $form_id)  {       
        foreach ($form_state as &$val) {           
            if ( is_array($val)) {    
                if ($val['op'] == 'Save configuration' ) {
                    save_configuration($val);                    
                    return;                                   
                }
            }           
        }                           
    } 
}
?>

   The code is using hook_form_alter() to save the values of a form displayed from itself. That is not the way to do it.

1. <?php
  $delFromSys = db_query("delete from system where name ='gigyaToolbar'"); 
  $delFromBlocks = db_query("delete from blocks where module ='gigyaToolbar'");
?>

   I think that is already done from Drupal; there is no need to add that code.

2. 2. The "output" of the gigyaToolbar module is a toolbar that can be displayed ONLY at the bottom of the page (a footer block).
   In this module, when enabled, there's no need to add a block title (although it's possible to add it).
   So, I don't understand what you meant by writing : "...titles given to the blocks are too generic..."

   If the block must be placed only on the footer, why didn't you implement hook_footer()? You cannot force a block to be placed in the footer.

   <?php
      $blocks[1] = array(
        'info'       => t('Example: empty block'),
        'status'     => TRUE,
        'weight'     => 0,
        'visibility' => 1,
        'pages'      => 'node/*',
      );
?>

   If the module is using just a block, why is it declaring a second block that has a generic title like Example: empty block (that is the title I was referring to)?

   <?php
    case 'view': default:
      // If $op is "view", then we need to generate the block for display
      // purposes. The $delta parameter tells us which block is being requested.
      switch ($delta) {
        case 0:
          $block['subject'] = t('<none>');
          // The content of the block is typically generated by calling a custom
          // function.
          $block['content'] = gigyaToolbar_footer_contents(1);
          break;              
        case 1:
          $block['subject'] = t('Title of block #2');
          $block['content'] = gigyaToolbar_footer_contents(2);
          break;
      }
?>

   The code is returning the content for a second block, and the title of that block is a generic Title of block #2.

3. <?php
    case 'configure':
      // If $op is "configure", we need to provide the administrator with a
      // configuration form. The $delta parameter tells us which block is being
      // configured. In this example, we'll allow the administrator to customize
      // the text of the first block.
      $form = array();
      if ($delta == 0) {
        // All we need to provide is a text field, Drupal will take care of
        // the other block configuration options and the save button.
        $form['gigyaToolbar_footer'] = array(
          '#type' => 'textfield',
          '#title' => t('Block content'),
          '#size' => 60,
          '#description' => t('The above will appear in the block content'),
          '#default_value' => gigyaToolbar_footer_contents(1),
        );     
      }
?>
   <?php
    case 'view': default:
      // If $op is "view", then we need to generate the block for display
      // purposes. The $delta parameter tells us which block is being requested.
      switch ($delta) {
        case 0:
          $block['subject'] = t('<none>');
          // The content of the block is typically generated by calling a custom
          // function.
          $block['content'] = gigyaToolbar_footer_contents(1);
          break;              
        case 1:
          $block['subject'] = t('Title of block #2');
          $block['content'] = gigyaToolbar_footer_contents(2);
          break;
      }
?>

   The user is able to set the content of the block, but then what the user entered is not used when the block is rendered.

4. <?php
    case 2:
      // It is possible that your block will not have any content, since it is
      // probably dynamically constructed. In this case, Drupal will not display
      // the block at all.
?>

   If the block is not shown, why is it defined?

5. <?php
    $result = db_query('SELECT * FROM {gigyaToolbar}');
   
    if ( $gigyaToolbar_db_data = db_fetch_array($result) ) {
        variable_set(GIGYATOOLBAR_PARTNER,$gigyaToolbar_db_data[GIGYATOOLBAR_PARTNER]);           
        variable_set(GIGYATOOLBAR_FACEBOOK_ID,$gigyaToolbar_db_data[GIGYATOOLBAR_FACEBOOK_ID]);
        variable_set(GIGYATOOLBAR_TWITTER_USERNAME,$gigyaToolbar_db_data[GIGYATOOLBAR_TWITTER_USERNAME]);
        variable_set(GIGYATOOLBAR_RSS_URL,$gigyaToolbar_db_data[GIGYATOOLBAR_RSS_URL]);       
        variable_set(GIGYATOOLBAR_STATUSMESSAGE,$gigyaToolbar_db_data[GIGYATOOLBAR_STATUSMESSAGE]);
        variable_set(GIGYATOOLBAR_SUBJECT,$gigyaToolbar_db_data[GIGYATOOLBAR_SUBJECT]);
        variable_set(GIGYATOOLBAR_BODY,$gigyaToolbar_db_data[GIGYATOOLBAR_BODY]);
    }
?>

   You have the settings in a database table, and then you copy them in Drupal variables. Why? Or the code uses a database table, or it uses Drupal variables.

6. The module's settings are saved in Drupal variables, but I also need the saved settings to be available, whenever the server is restarted and that's why I need to save them in the database.
   The gigyaToolbar table contains ONLY ONE record and will always contain EXACTLY one record.

   Drupal variables are saved when the server is restarted; why do you think you need to save the settings in the database? Drupal variables are saved in the database too; they are not getting lost when the server is restarted.

   You can check Drupal code, and you will notice that Drupal never creates a database table that is supposed to contain just a row.

7. Our module works well and we do not need to develop it more at the time.

   When you commit code in Drupal.org, you don't change it only when you need to change it for your own use, but you could get feature request from other people. In such cases, you cannot reply them I am not going to change the module because we don't need that feature; if that is what you think, then you can keep your module in your own server, as it is not a must to deploy a module on Drupal.org CVS.

8. <?php
function gigyaToolbar_menu_access_user_debug() {
  global $user;
  if (arg(1) == $user->uid)
    return user_access('access devel information');
}
?>

   That is a debug function that is not even called from module code; it should then be removed, first because it is debug code, and second because it is not used from the module.

Please check.

I added the module_load_include, so that gigyaToolbar_footer function will be able to use the constants defined in gigyaToolbar.admin.inc ( A scope issue ).

Thanks a lot again.