Yandex Site search Pinger

Could you, please, specify correct Git link in the issue: http://git.drupal.org/sandbox/yandex-plugins/1937972.git
and provide a Project link (http://drupal.org/sandbox/yandex-plugins/1937972)

Results of the automatic review: http://ventral.org/pareview/httpgitdrupalorgsandboxyandex-plugins1937972git
Take a look at them.

Manual review:
1) yandex_pinger.install
- Line 52. The following

if ($settings = init_settings()) {
    variable_set($prefix . 'key', $settings['key']);
    variable_set($prefix . 'login', $settings['login']);
    variable_set($prefix . 'searchId', $settings['searchId']);
    variable_set($prefix . 'message', $settings['message']);
    variable_set($prefix . 'pluginId', $settings['pluginId']);
  }

can be replaced with the foreach () through $settings.

2) yandex_pinger.module
- Looks like this plugin has all the messages in Russian and is not supposed to be translated into other languages. What about mentioning it on the Project Page: http://drupal.org/sandbox/yandex-plugins/1937972
Otherwise you should write all the messages in English and use t() function to translate them.
- Consider justifying tabs and indents of the arrays (Line 70, 229 and so on)

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and put yourself on the PAReview: review bonus high priority list. Then I'll take a look at your project right away :-)

Hi,

You should follow http://ventral.org, because Coder module is not PAReview.sh, which is required by klausi.

Manual code review:

README.txt is not in right format.

yandex_pinger.info:
These two lines used in wrong way, install and module files includes by default, files array in info should include only additional, so you don't have to do module_load_include
files[]= yandex_pinger.install
files[]= yandex_pinger.module

yandex_pinger.install

1. function _yandex_pinger_init_settings:
   • using settings.ini in module folder for user settings - is not the way you should
     provide configuration, because in most cases they are using repositories and
     files in contributed modules can not be modified at all. I suggest you to implement
     configuration page, after module installation. - In case if this is not possible: provide
     users instructions, so they can set all required variables in settings.php using $conf.
   • It is not recommended to use "throw new Exception" in module install. Use standard drupal error handling in module install.
2. Why do you need to create custom menu with name "Development" (which is already exists in most cases) @line73?

yandex_pinger.module

1. In function _yandex_pinger_doping@57, which is called each time any node is inserted or updated: you use "throw new Exception("unable to write");", "throw new Exception("unable to create socket");", etc... , in case if there is any problem with connection to the server. - it will prevent user from update and make them confused. Use drupal_set_message and watchdog.
2. Is there any reason to use fsockopen function instead of curl to get a response from third-party service?

3.    $postdata = array(
       'key' => variable_get($prefix . 'key', ''),
       'login' => variable_get($prefix . 'login', ''),
       'search_id' => variable_get($prefix . 'searchId', ''),
       'pluginid' => variable_get($prefix . 'pluginId', ''),
       'urls' => $url,
       'publishdate' => 0,
       'cmsver' => VERSION,
     );
   
     $tarr = array();
   
     foreach ($postdata as $key => $val) {
       $tarr[] = $key . '=' . $val;
     }
   
     $postdata = implode('&', $postdata);
     unset($tarr);
   

   Can be revised with this:

   $postdata = array(
       'key' => variable_get($prefix . 'key', ''),
       'login' => variable_get($prefix . 'login', ''),
       'search_id' => variable_get($prefix . 'searchId', ''),
       'pluginid' => variable_get($prefix . 'pluginId', ''),
       'urls' => $url,
       'publishdate' => 0,
       'cmsver' => VERSION,
     );
     $postdata = http_build_query($postdata);
   

   http://www.php.net/http_build_query

Also I'd recommend to add ability for user to specify which content type of nodes they would ping in yandex.

Good luck!

Hi again,

Regarding curl or sockets - you can use http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_htt... function, it uses sockets I believe.

I'm just a reviewer, who went thru Bonus Review process, like others, and do not have permission to grant access for promoting full projects :)
If you want to be reviewed by responsible person, you would like to read this article http://drupal.org/node/1410826

• First of all - do not RTBC your own applications.
• Furthermore, why don't you make use of drupal_http_request() for doing the ping? There seems to be nothing special about it.
• "Initializing" settings is a bad practice in Drupal 6 and 7, please don't do that.
• Do not change your modules weight by altering the system table in drupal 7 - use hook_module_implements_alter()
• Don't overwrite the existing settings when the module is re-enabled by calling the install functions - that makes no sense.

I understand that you try to make the module easily maintainable over all drupal core versions by using similar API's and code. BUT that's not the way drupal works.

Drupal is driven by innovation. If there are new and easier API functions ->>> USE them.

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

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

Fix repo path, add links to reviewed projects