Yandex Share

I just took a quick look at your project.

Licensing: no problems noted.
Module duplication: No duplication found.

Your .info file is missing a few items. Please check the module developers guide http://drupal.org/node/231036 and update the file. Make sure to change your project status to 'needs review' after you have made the changes.

This project still needs technical and security review.

The waiting time for a full review can be as long as 4-6 weeks. You can help to reduce the wait by contributing to the code review process. Please see http://groups.drupal.org/code-review for details on how to participate.

* Remove all old CVS $Id tags from your files, not needed anymore
* Don't mix "Implements hook_xxx()" and "Implementation of hook_xxx()" in your doc blocks. Choose one.
* Are you sure you want to implement hook_init()? It is invoked on literally every page even if your module has nothing to do there.
* yandex_share_theme(): indentation errors, use 2 spaces everywhere.
* yandex_share_admin_settings(): doc block formatting is not correct, see http://drupal.org/node/1354#functions
* Remove the translations folder, translation is done on http://localize.drupal.org

• It appears you are working in the "master" branch in git. You should really be working in a version specific 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.
• CHAMGELOG.txt: wrong line endings ('\r' but should be unix style '\n'). Run coder to check your code style http://drupal.org/project/coder
• "//Get array for content types": comment formatting: there should be a space after "//" and a "." at the end
• yandex_share_admin_settings(): indentation error on a nested #options array
• "$button_img: use theme('image', ...) instead.
• theme_yandex_share_auto(): there should be a new line between the previous function and the doc block. Also elsewhere.
• theme_yandex_share_auto(): you should sanitize variables before embedding them into markup to avoid XSS.

Hi Plazik,

Please address these issues and set the status back to "needs review":

• drupal_add_js("new Ya.share({\n element: 'ya_share" . $id . "',\n l10n: '" . $language . "',\n elementStyle: {" . $text . $type_style . $border_style . $link_underline . $link_icon . "\n 'quickServices': ['" . $services_standart . "']\n },\n link: '" . $url . "',\n title: '" . $title . '\',' . $image . $description . "\n popupStyle: {\n blocks: {\n '" . $popup_text . "': ['" . $services_standart_popup . "'],\n }" . $copy_paste_field . $vdirection . "," . $code_for_blog . "\n },\n});", 'inline', 'footer');

  This appears to include a JSON-formatted object. Rather than writing JSON out manually, which risks doing something wrong and also makes the code very difficult to read, you should put it in an object and use drupal_json() to format it as JSON. This will also allow you to take out a lot of the code before this, which is also formatting variables as JSON.

• $output = '<div class="yashare-auto-init" data-yashareL10n="' . $language . '" data-yashareType="' . $type . '" data-yashareQuickServices="' . $services . '" data-yashareTitle="' . $title . '" data-yashareLink="' . $url .'"></div>';

  You should be wrapping all these variables in check_plain().

• $output = '<a href="' . $url . '" title="' . check_plain($title) . '" target="_blank">' . check_plain($title) . '</a>\'';

  You should create this link HTML with the l() function.

• Sanitization should always happen as close as possible to the actual output, i.e. it should happen in your theme function. Example: http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_image/6
• theme_yandex_share_buttons(): use theme('image', ...) here

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/yandex_share.module:
   +10: [minor] Comment should be read "Implements hook_foo()."
   +35: [minor] Comment should be read "Implements hook_foo()."
   +139: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +193: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 2 files, 1 normal warnings, 3 minor warnings, 0 warnings were flagged to be ignored
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:
"'#options' => $node_types,": you must sanitize the node type labels here. a node type with the label <script>alert('XSS');</script> would result in a nasty javascript popup on your form. See also http://drupal.org/node/28984 the section about checkboxes. Sorry that I did not spot that earlier.

Checked the code and tested functionality; found nothing to mention.

Just a general point: Your module name refers to yandex, but actually your plugin really enables sharing with a lot of SN. IMO, you should rethink naming of your module and change the project description as well. With very few changes you would provide a useful and universal social share module.

May I also suggest that you implement hook_block, now or later?

Then, last point: I did not check that myself now and I read comment #1, but are you really sure there is no duplication issue (since some other "social share" projects already exist)?

Just to clarify, I find your code tiny and easy to set up.

Ah, okay, so all the SN buttons are generated by this yandex api then? That confused me since I could simply click "tweet" without ever having registered with yandex. Ok.

I just mentioned the "block" feature because I have built a SN sharing module myself (unreleased), and it provides a simple "social share" block to put wherever it fits into a theme. Advantage is that you could add it anywhere and not just for nodes, e.g. on a views result page or so. I thought I'd mention it because you might receive such a feature request sooner or later anyway ;)

Regression reveals a little bit more new coding-style flaws:

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• Remove all old CVS $Id tags, they are not needed anymore.
  

  CHANGELOG.txt:34:- Remove old CVS $Id tags
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/yandex_share.module:
   +10: [minor] Comment should be read "Implements hook_foo()."
   +120: [minor] Comment should be read "Implements hook_foo()."
   +160: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +217: [minor] Comment should be read "Implements hook_foo()."
   +251: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 1 files, 1 normal warnings, 4 minor warnings, 0 warnings were flagged to be ignored
  

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

  
  FILE: ...709/sites/all/modules/pareview_temp/test_candidate/yandex_share.install
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   9 | WARNING | Format should be * Implements hook_foo().
  --------------------------------------------------------------------------------
  
  
  FILE: ...p709/sites/all/modules/pareview_temp/test_candidate/yandex_share.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AND 5 WARNING(S) AFFECTING 7 LINE(S)
  --------------------------------------------------------------------------------
     6 | WARNING | Line exceeds 80 characters; contains 83 characters
    10 | WARNING | Format should be * Implements hook_foo().
   120 | WARNING | Format should be * Implements hook_foo().
   153 | ERROR   | Closing brace indented incorrectly; expected 2 spaces, found 4
   160 | ERROR   | Concat operator must be surrounded by spaces
   217 | WARNING | Format should be * Implements hook_foo().
   251 | WARNING | Format should be * Implements hook_foo().
  --------------------------------------------------------------------------------
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./yandex_share.install ./yandex_share.admin.inc ./yandex_share.info ./README.txt ./yandex_share.module ./CHANGELOG.txt
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual additions:

• Regarding those results and your recent comment, are you sure that you pushed your changes?
• What exactly is meant by "Two types of initsializaatsii (automatic and standard)", is it meant to be "types of installation"?
• There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.

HTH, dave

coding style looks quite good so far

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

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

  
  FILE: ...view/sites/all/modules/pareview_temp/test_candidate/yandex_share.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
    6 | ERROR | Whitespace found at end of line
   31 | ERROR | Comma required after last value in array declaration
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

Hi wanted to have a deeper look at your .module, but this is really hard because there are only a few comments and it's all very compressed.

It would be much easer if you split big code blocks into smaller ones (depending on the functionality of the segment) and describing a little more in detail what you do and especially why your doing it.
Rather too much comments then too few ;-)

the variable_del function does the following:
db_query("DELETE FROM {variable} WHERE name = '%s'", $name);
So you have to specifiy the deletion of each variable used in your code!
This variable_del('yandex_share'); is not enough.

@Plazik has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

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