[D7] Open Graph Comments

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxgulabbisht2104985git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi gulab.bisht,

great idea for a module, it look very simple and easy to use. Unfortunately this module also does not meet the minimum 5 functions and 120 lines of code for approval. You may need to think about any other features you may be able to add to this module before it canbe approved.

In the readme file could you add link to more detail about open graph meta tags, what they are and what they do?

I enabled the module and wrote a new comment on a basic page without any links and received this error:
Notice: Undefined offset: 0 in open_graph_comments_comment_view_alter() (line 47 of /path/to/drupal/sites/all/modules/contrib/open_graph_comments/open_graph_comments.module).

There are not enough functions or lines of code to review in line with the drupal standards.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Looks fine to me except the commenting of open_graph_comments_display_comment() function

Hi.

It seems hook_form_BASE_FORM_ID_alter() is used instead of hook_form_FORM_ID_alter().
See https://api.drupal.org/comment/25048#comment-25048
If so please change comment to
Implements hook_form_BASE_FORM_ID_alter()

It appears you are working in the "7.x" branch in git. You should really be working in a 7.x-1.x branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the 7.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/open_graph_comments.module
  --------------------------------------------------------------------------------
  FOUND 10 ERRORS AFFECTING 9 LINES
  --------------------------------------------------------------------------------
    13 | ERROR | [x] Whitespace found at end of line
    14 | ERROR | [ ] Missing parameter type
    16 | ERROR | [ ] Tag cannot be grouped with paramater tags in a doc comment
    16 | ERROR | [ ] @return data type must not contain "$"
   100 | ERROR | [x] Whitespace found at end of line
   101 | ERROR | [ ] Missing parameter type
   103 | ERROR | [ ] Missing parameter type
   105 | ERROR | [ ] Missing parameter type
   107 | ERROR | [x] Whitespace found at end of line
   108 | ERROR | [ ] Return type missing for @return tag in function comment
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

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. open_graph_comments_display_comment(): why do you suppress warnings with "@" here? Please add a comment.
2. open_graph_comments_comment_view_alter(): this looks vulnerable to XSS exploits. $url, $title etc. come from an untrusted third party source and therefore they must be sanitized before printing into HTML. If some website delivers some malicious attributes that contain script tags or whatever, then there is a vulnerability. Make sure to read https://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
3. open_graph_comments_comment_view_alter(): don't call theme() here and don't add to #markup. Just add a new array entry in the build array, Drupal will render it later for you. See https://drupal.org/node/930760

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I'm confused, there is a 7.x branch and a 7.x-1.x branch, but there should only be a 7.x-1.x branch. Could you make sure that all your recent changes are on 7.x-1.x and then remove 7.x?

I am somewhat concerned about line 28 in open_graph_comments.module

@$html->loadHTML($request_data->data);

Is this a potential security hazard?

Here are the few points from my review :-

a) For each node type, open graph comments setting is stored in variables table. So I think, variables related to open graph comments should be flushed out when this module is not in use.

b) In hook_comment_update, $comment_node_type is initialised. I think , there is no need to initialize this variable.

c) Again in hook_theme, no need to create variable $themes as you can simply return array. Creating un-necessary varibale causes performance problem as they take memory.

d) In hook_comment_view_alter, you are getting comment data for LANGUAGE_NONE. It will always returns the data for only one or default language, not for other languages.

e) (My Opinion) In hook_comment_view_alter, you have used four different variables $url, $title, $desc, $img.
Instead of using separate variables for them , you can use array.

thanks

All points are looking good except (e).
In (e) what I was trying to say is that , remove $title, $desc, $img, $url and use array something like this :-
$meta_array = array()
then on change these

 if (isset($og_tags['og:url'])) {
        $url = $og_tags['og:url'];
      }

to

  if (isset($og_tags['og:url'])) {
        $meta_array = check_plain($og_tags['og:url']);
      }

and for rest of the variables.

then change

$build['ogc'] = array(
        '#theme' => 'open_graph_comments_template',
        '#og_data' => array(
          'url' => check_plain($url),
          'title' => check_plain($title),
          'img' => check_plain($img),
          'desc' => check_plain($desc),
        ),
      );

to

$build['ogc'] = array(
        '#theme' => 'open_graph_comments_template',
        '#og_data' => $meta_array,
        ),
      );

Its my opinion as I think it will make code more cleaner.

Than again on template file, you should check if variable is set, then only output them.

@joshi.rohit100

I believe those issues are purely stylistic, not required by Drupal coding standards or best practices, and offer no increase in elegance or utility. I am reverting this to 'Needs Review'

Per https://www.drupal.org/node/532400

Important note for reviewers: A 'needs work' status change should be reserved for 'major' code quality issues. Minor changes, recommendations for full API compatability, and txt file perfection are not major code quality issues. Holding up reviews on these changes can come off as unnecessary and nitpicky, especially early in the review process. This can also cause frustratingly long waits for uninitiated applicants.

However:

If he did want to increase the elegance of this block,

if (isset($og_tags) && !empty($og_tags)) {
      if (isset($og_tags['og:url'])) {
        $url = $og_tags['og:url'];
      }
      if (isset($og_tags['og:title'])) {
        $title = $og_tags['og:title'];
      }
      if (isset($og_tags['og:image'])) {
        $img = $og_tags['og:image'];
      }
      if (isset($og_tags['og:description'])) {
        $desc = $og_tags['og:description'];
      }

      $build['ogc'] = array(
        '#theme' => 'open_graph_comments_template',
        '#url' => check_plain($url),
        '#title' => check_plain($title),
        '#img' => check_plain($img),
        '#desc' => check_plain($desc),
      );
    }

He could change it to.

    $meta_tags = array();
    foreach($og_tags as $og_tag => $og_value)
    {
      list(, $og_key) = explode(':', $og_tag);
      $meta_tags[$og_key] = $og_value;
    }
    $meta_tags += array(
      'url' => '<some default value>', 
      'title' => '<some default value>', 
      'image' => '<some default value>',
      'description' => '<some default value>',
    );
   $build['ogc'] = array(
        '#theme' => 'open_graph_comments_template',
        '#og_data' => $meta_tags,
    );

And then change the tpl file to use the actual Open Graph properties.

@gwprod

Your example in #34 and what I mentioned in 32, doing same thing - making code more readable. It is not a good idea to throw un-necessasy variables as it makes code noisy that someone don't want to read. As in future, if someone looks at the module and feel uncomfortable with number of lines and unnessary variables.

@joshi.rohit100 I will reiterate that I believe a fairly minor issue of readability from your personal perspective is neither a blocking issue nor a major code quality issue.

Per https://www.drupal.org/node/532400

Important note for reviewers: A 'needs work' status change should be reserved for 'major' code quality issues. Minor changes, recommendations for full API compatability, and txt file perfection are not major code quality issues. Holding up reviews on these changes can come off as unnecessary and nitpicky, especially early in the review process. This can also cause frustratingly long waits for uninitiated applicants.

@gwprod

Ok fine, its not a major or blocking issue, but what about this ?

A 'needs work' status change should be reserved for 'major' code quality issues

How will you judge the code quality as Same thing can be done using single loop or multiple loops. Then both codes are good or same ?

@joshi.rohit100

I personally would prefer to see code that is as elegant and efficient as possible. Some times that is not practical, some times it is not necessary. I would say that a major code quality issue is when things are not done 'The Drupal Way' by and large. When formatting is incorrect. When there are security vulnerabilities.

"Needs work" is when your teacher hands you back a paper and says 'F - Revise and resubmit"

The change is #32 is not a reason to go back to Needs Work. You even mentioned in #30 that is was an opinion.

Stylistic issues are never blockers, unless there is blatant disregard for Drupal coding standards. Miss some indentation here or there? Mention it, but don't push back. Totally unformatted code or wild spaghetti code? Push back.

Reviews are not meant to hold up applications to get perfect modules. They are primarily sanity checks to get good decent modules on DO, and also to ensure that a release doesn't have third party code, licensing problems, or security issues.

manual review:

• open_graph_comments_comment_view_alter(): why can't you use valid_url(), qhy do you need $reg_exurl? Please add a comment.

But otherwise looks RTBC to me.

Assigning to mpdonadio as he might have time to take a final look at this.

Automated Review

Review of the 7.x-1.x branch:

~/tmp/open_graph_comments$ git branch -v
* 7.x-1.x eeef33c lid removed from cache variable on comment update.

• Coder Sniffer has found some issues with your code (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. You have to get a review bonus to get a review from me.

False positive about CSS.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) Your handling of the external URLs are not safe. On lines 80 and 86, replace the check_plain() with check_url(). This will ensure
that dangerous protocols (which can be overridden per site) get stripped. Poke through https://api.drupal.org/api/drupal/includes%21common.inc/function/calls/c...
to see how this gets used in core.

Coding style & Drupal API usage

You need to implement a hook_node_type_delete() to delete your variable for that content type.

Not a fan of the target=_blank in the .tpl.php files. This should be the user's choice.

It would be a little better to use the theme functions for the image and link, rather than using variables in the template. These will also take care of the
sanitization problems mentioned above.

Good use of inline comments.

open_graph_comments_display_comment() is a bad name for what the function actually does. Maybe open_graph_comments_get_og_tags()?

Eventually, you may want to consider your own cache bin.

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.

This is good to go once the (*) lines are fixed.

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

I read `git diff eeef33c..HEAD` and this this RTBC. Normal procedure for admins is to not go straight from Needs Work to Fixed. I will check on this in a few days to see if there are any objections.

Cool, I agree that all necessary issues are now addressed, so ...

Thanks for your contribution, gulab.bisht!

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.