Digg Publisher [diggpublisher]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions. Searching for Digg, I found many projects; why didn't you open a feature report for the features missing in one of the existing projects?

I know we try to avoid duplication but if the company being integrated with wants to take direct control of the integration, that doesn't seem unreasonable to me.

Michelle

These are my comments on the code as it stands. I will avoid the politics.

function _diggable_topics() {
  if (function_exists('fsockopen')) {
    $socket = @fsockopen('services.digg.com', 80);
    if (!$socket) {
      return NULL;
    }

    fwrite($socket, "GET /1.0/endpoint?method=topic.getAll&type=json HTTP/1.1\r\n");
    fwrite($socket, "Host: services.digg.com\r\n");
    fwrite($socket, "Connection: close\r\n");
    fwrite($socket, "User-Agent: PHP Digg Drupal plugin\r\n\r\n");
    $data = '';
    while (!feof($socket)) {
      $data .= fgets($socket, 128);
    }
    fclose($socket);
    $data = @json_decode(end(explode("\r\n\r\n", $data)), TRUE);
    if (!is_array($data) || !isset($data['topics'])) {
      return NULL;
    }

    // Create options array
    $options[''] = '(none)';
    foreach ($data['topics'] as $topic) {
      $options[$topic['short_name']] = $topic['name'];
    }

    return $options;
  }

  return NULL;
}

should use: http://api.drupal.org/api/function/drupal_http_request/6

And for adding external script: http://api.drupal.org/api/function/drupal_set_html_head/6 although that isn't a big deal to me. Adding external scripts in D6 isn't nice, D7 is better.

$button .= '<a class="DiggThisButton" href="http://digg.com/submit?url=' . $url . '&title=' . urlencode($node->title) . '" rel="external" rev=", ' . variable_get('diggable_button_topic', '') . '">';

could be wrapped like so.

$buttons .= l($hidden_span . $image, 'http://digg.com/submit?url=' . $url . '&title=' . urlencode($node->title), array('attributes' => array('rel' => 'external', 'rev' => variable_get('diggable_button_topic', ''), 'class' => 'DiggThisButton'), 'html' => TRUE);

There is this: # Take off the last word, Jeff style! Perl style comments are discouraged. http://drupal.org/coding-standards#comment

Wondering if instead of the hardcoded 'float: left', 'float: right' could that be in a style sheet. This allows different themes to fix it as styles applied directly to an element override the CSS files. And that would avoid having to override the theme function just for small styling changes.

Probably should add hook_content_extra_fields so that way the weight of the Button could be controlled via CCK interface

This module is still undergoing internal QA. I've attached the most recent version, which includes modifications based on the above feedback.

A couple of notes:

* The external script now needs to go in the footer, so cannot use drupal_set_html_head.
* An "unstyled" option was added to the button, which hopefully satisfies the concern about hard-coding float css.
* The remaining concerns have been addressed.

Please refer to http://about.digg.com/diggable-drupal-module for the expanded motivational message and feature details.

Please let us know if there's anything else we can do to expedite the process of receiving a cvs account. We were hoping to have the initial public release of our module hosted on drupal.org, but if we are unable to do so, we'll host it ourselves on digg.com. Public release is slated for next week.

As it's jennycita who attaches the update archives, is the application for Digg Publisher, or jennycita?

Assigning to myself. Kiamlaluno will review tomorrow.

Ok another review

variable_set('diggable_button_include_js', 1);

Should be a static cache in the diggable_footer() function and then you don't need that variable_get/set.

function diggable_footer() {
  // Workaround for lack of drupal_add_js() compatibility with external stylesheets in D6
  static $js_added = FALSE;
  if (!$js_added) {
    $js_added = TRUE;
    return '<script src="http://widgets.digg.com/buttons.js" type="text/javascript"></script>';
  }
}

Should consider: http://api.drupal.org/api/function/drupal_add_link/6 instead of printing the script tag directly. It has a disadvantage of putting the external JS in the head tag as opposed to the footer which is a performance hit. So Im not convince that would be a necessity.

drupal_add_js("
    $(function() {
      var myGenerator = new DiggWidgetGenerator($json, function() {
        $('textarea[name=diggable_widget_html]').val(myGenerator.getCode());
        $('input[name=diggable_widget_json]').val(myGenerator.getWidget());
      });
    });", 'inline', 'footer', FALSE, TRUE, FALSE);

should be

drupal_add_js("
    $(document).ready(function() {
      var myGenerator = new DiggWidgetGenerator($json, function() {
        $('textarea[name=diggable_widget_html]').val(myGenerator.getCode());
        $('input[name=diggable_widget_json]').val(myGenerator.getWidget());
      });
    });", 'inline');

Doesn't need to be a behavior because it doesn't need to be applied to new html added to the page. Though i probably can't hurt. At a min it should be a $(document).ready(). Otherwise, if I have a theme that puts JS in the footer, this previous code will cause JS error as the jQuery.js hasn't been added to the page and when the browser compiles this inline script, it will fail. And because its inline JS, $scope = 'header', $defer = FALSE, $cache = TRUE, $preprocess = TRUE are all ignored. drupal_add_js really needs a set of helper functions that make that clearer.

The block uses user entered HTML which is a security problem. I am able to add

to the widget html.

this would be my suggestion for the widget html.

$form['advanced_settings']['diggable_widget_html'] = array(
    '#title' => t('Widget code'),
    '#description' => "Note that any customizations you make here will be overwritten if you make further modifications above or resubmit this form.",
    '#type' => 'textarea',
    '#default_value' =>  $html,
    '#cols' => 70,
    '#rows' => 10,
    '#access' => user_access('Administer site configuration'),
  );

Since you need JS in this 'html', this will mitigate the hole and only show the textarea for people with an advanced permission. Taken from: http://drupal.org/security-advisory-policy

Couple of questions @Scott:

1. The idea behind the variable diggable_button_include_js was to only load the external javascript on pages where it's actually needed, that is, where the Digg button is being displayed at least once. This is any time we're showing the full content of a node type for which the Digg button is enabled (potentially including views, panels, etc). I don't think a static variable declared in the footer would work quite the same way. If I understand your snippet correctly, it prevents the js from being included on the page twice in the event that hook_footer was somehow called twice, correct? That wasn't exactly the use case I was going for...
2. $() is an alias for $(document).ready() (see api), but I can certainly switch to it if you'd like. I'm wondering, though, wouldn't either approach fail in the event that the jquery library wasn't loaded first?
3. Unfortunately, if the configuration page is ever displayed without the 'Advanced Settings' textarea, our widget generator js will break. This is where the generated widget output goes in order to later be saved in the db and displayed as a block. I was relying on the fact that permissions to access the configuration page itself are set to 'administer site configuration'... is that adequate protection?

I'll go ahead and nix the extra variables passed to drupal_add_js. Thanks for looking into this!

The idea behind the variable diggable_button_include_js was to only load the external javascript on pages where it's actually needed, that is, where the Digg button is being displayed at least once. This is any time we're showing the full content of a node type for which the Digg button is enabled (potentially including views, panels, etc). I don't think a static variable declared in the footer would work quite the same way. If I understand your snippet correctly, it prevents the js from being included on the page twice in the event that hook_footer was somehow called twice, correct? That wasn't exactly the use case I was going for...

Ahh right I forgot that is a hook. How I would accomplish this

funtion theme_diggable_button($node) {
 //.... Code here
 diggable_add_external_js();
}

function diggable_add_external_js() {
  // Workaround for lack of drupal_add_js() compatibility with external stylesheets in D6
  static $js_added = FALSE;
  if (!$js_added) {
    $js_added = TRUE;
    drupal_set_html_head('<script src="http://widgets.digg.com/buttons.js" type="text/javascript"></script>');
  }
}

Essentially, anytime you want to add JS to the page response you call diggable_add_external_js(). And that handles making sure it gets added exactly once where and when it is required.

Variable_get and sets will not accomplish what you need. It creates a race condition where user 1 requires the JS and user 2 doesn't, sometimes user 2 will get the JS. And the opposite is true too where user 1 requires it and user 2 doesn't. This is because variable_get/set isn't set per session but globally for all sessions.

@point 2, ahh interesting I wasn't aware. To remove the javascript bug with jquery being added AFTER the inline JS, it would need to be a behavior or in a separate JS file (which would then be added in the footer). Killer is I have solved this previously #623416: Don't use jQuery(document.ready())

Unfortunately, if the configuration page is ever displayed without the 'Advanced Settings' textarea, our widget generator js will break. This is where the generated widget output goes in order to later be saved in the db and displayed as a block. I was relying on the fact that permissions to access the configuration page itself are set to 'administer site configuration'... is that adequate protection?

Ugg, thats what I get for reading things too fast. Right that is proper. No security hole.

I reviewed the code, and I didn't find anything that would stop the approval of this application.

The points I am reporting here is something that should be changed, but that can be done when the code is committed in the Drupal.org repository.

1. Strings used in the user interface should be in sentence case.
2. The curly bracket in the control statements, according to the Drupal coding standards, is not optional; the instructions executed from the control statement should be on a separate line, and the control statement should be closed by a curly bracket. This is actually done only once.

3.   if ($html && !$block->status) {
       drupal_set_message('Visit the ' . l('Block Configuration Page', 'admin/build/block'). ' to place The Digg Widget into a region of your theme.', 'status', FALSE);
     }
   

   The string should be passed to t(); in that case, the call to l() needs to be removed, and the string should not be concatenated (this gives to the translator users more context, and more chances to understand how to translate the string).

4. $() is an alias for $(document).ready(); I am not aware of any problems using the first form rather than the second. More probably the code should use Drupal behaviors, but the code should work even as it is written; there are pros using the behavior, but I see the change as an optimization.

That said, I am going to approve this application.

Thank you for your contribution! I am going to update your account.
These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

I'm a bit baffled by this approval. Neither this issue, nor the page that was referred to http://about.digg.com/diggable-drupal-module, contains any explanation for why we need yet another module for Digg, which does not even provide more functionality, nor a proper/more modern configuration and implementation, nor any other integration differences. Instead, very rudimentary things about Drupal's APIs had to be explained within this very issue.

The Drupal community is a place to share, collaborate, and innovate. I'd really like to know why you believe that "working with authors of existing modules isn't a viable option."

Did you actually ask the current maintainers?

@kiamlaluno: Excellent news! We'll incorporate your suggestions and run through a final qa; the module should be uploaded to drupal.org by the end of the week.

@Scott Reynolds: Good catch re: race condition bug. I'll use your static variable approach instead.

Thanks, guys!