OpenSearchServer

I think there is no README.txt file in your repository

Please find the latest commit with README in
http://drupalcode.org/sandbox/ekeller/1128202.git/commit/d2d54d0edbc1bd9...

Please remove the LICENSE.txt it is added automatically by the drupal packaging script.
after you remove it set your project back to needs review.

and please,
read this here http://drupal.org/node/1015226
and name your git branches accordingly.
the way you have it now it is very confusing.also you should point which branch a reviewer should check

Updated the branches according to the naming conventions of Drupal and please review the branch 6.x-1.x

Ok thanks for the branch renaming!
I checked the module a bit more, but soon find out a lot issues.

i suggest running your module on Coder on minor level, also reading this
http://drupal.org/coding-standards and since you mostly use OOP be sure to read this too as well http://drupal.org/node/608152

I am sorry that i force you to rewrite a large part of your module, but it is required if you want to get full project status

When your module fully complies with Drupal coding standars set this back to needs reviews!
(it makes reviewing easier when code is consistent)
Thank you !

ThankYou for your suggestions.We have updated the code regarding the drupal coding standards.
Please review the tag 6.x-1.1-rc1.

- Put a space between the (type) and the $variable in a cast: (int) $mynumber.
- All binary operators (operators that come between two values), such as +, -, =, !=, ==, >, etc. should have a space before and after the operator
eg this

$this->channelTitle    = (string)$xml->channel->title;

should be

$this->channelTitle = (string) $xml->channel->title;

- You are strongly encouraged to always use curly braces even in situations where they are technically optional

    foreach ($items as $item)
      $this->append(new NewsFeedParser_RSS_Entry($item));

to

    foreach ($items as $item) {
      $this->append(new NewsFeedParser_RSS_Entry($item));
    }

or

    if (isset($xml->channel->item[0]))
      return new NewsFeedParser_RSS($xml);
    // Atom
    elseif (isset($xml->entry[0]))
      return new NewsFeedParser_Atom($xml);

to

    if (isset($xml->channel->item[0])) {
      return new NewsFeedParser_RSS($xml);
    }
    // Atom
    elseif (isset($xml->entry[0])) {
      return new NewsFeedParser_Atom($xml);
    }

- Comments should be always wrapped to 80 spaces.

- Do not put licensing information in your module anywhere. Anything that is put in the Drupal repository is automatically licensed at GPLv2.

- Remove ; $Id$.They are not needed now that drupal moved to git from CVS

- Indentation: everything is indented to 2 spaces:

'fields' => array(
 'serverurl' => array(
 'description' => t('The Server Url of opensearchserver.'),
 'type' => 'varchar',
 'length' => '255',
 'not null' => TRUE,
 'not null' => TRUE),

should be

'fields' => array(
  'serverurl' => array(
    'description' => t('The Server Url of opensearchserver.'),
    'type' => 'varchar',
    'length' => '255',
    'not null' => TRUE,
    'not null' => TRUE
  ),

- Functions should be separated with a new line, and documented if possible.Also separate dots with spaces:
eg

}
function opensearchserver_validate($form, &$form_state) {
  if (empty($form_state['values']['keys'])) {
    form_set_error('keywords', t('Please enter An Search Term.'));
  }
}
function opensearchserver_submit($form, &$form_state) {

  if ($form_state['values']['search_theme_form']) {
      $form_state['redirect'] = 'opensearchserver/search/'. $form_state['values']['search_theme_form'];
  }
  elseif ($form_state['values']['search_block_form']) {
    $form_state['redirect'] = 'opensearchserver/search/'. $form_state['values']['search_block_form'];
  }
  else {
    $form_state['redirect'] = 'opensearchserver/search/'. $form_state['values']['keys'];
  }
}

}

/*
 * Documentation.
 */
function opensearchserver_validate($form, &$form_state) {
  if (empty($form_state['values']['keys'])) {
    form_set_error('keywords', t('Please enter An Search Term.'));
  }
}

/*
 * Documentation.
 */
function opensearchserver_submit($form, &$form_state) {

  if ($form_state['values']['search_theme_form']) {
      $form_state['redirect'] = 'opensearchserver/search/' . $form_state['values']['search_theme_form'];
  }
  elseif ($form_state['values']['search_block_form']) {
    $form_state['redirect'] = 'opensearchserver/search/' . $form_state['values']['search_block_form'];
  }
  else {
    $form_state['redirect'] = 'opensearchserver/search/' . $form_state['values']['keys'];
  }
}

- Use check_plain() when printing unsafe values to avoid XSS

  $form['details']['indexname'] = array(
    '#type'  => 'textfield',
    '#title'  => t('IndexName '),
    '#description'  => t('Name of the OpenSearchServer Index '),
    '#default_value'  => $sdetails->indexname,
  );

  $form['details']['username'] = array(
    '#type'  => 'textfield',
    '#title'  => t('UserName'),
    '#description'  => t('The username of OpenSearchServer for authentication '),
    '#default_value'  => $sdetails->username,
  );

  $form['details']['key'] = array(
    '#type'  => 'textfield',
    '#title'  => t('Key'),
    '#description'  => t('The Key of OpenSearchServer for authentication '),
    '#default_value'  => $sdetails->key,
  );

to

  $form['details']['indexname'] = array(
    '#type'  => 'textfield',
    '#title'  => t('IndexName '),
    '#description'  => t('Name of the OpenSearchServer Index '),
    '#default_value'  => check_plain($sdetails->indexname),
  );

  $form['details']['username'] = array(
    '#type'  => 'textfield',
    '#title'  => t('UserName'),
    '#description'  => t('The username of OpenSearchServer for authentication '),
    '#default_value'  => check_plain($sdetails->username),
  );

  $form['details']['key'] = array(
    '#type'  => 'textfield',
    '#title'  => t('Key'),
    '#description'  => t('The Key of OpenSearchServer for authentication '),
    '#default_value'  => check_plain($sdetails->key),
  );

- Use indentation on sql queries too.This is ver ugly eg

db_query("INSERT INTO `{opensearchserver}` (`serverurl`,  `indexname`,  `username`,  `key`, `last_indexed`, `delay`, `filter_enabled`, `signature`, `title_snippet`, `content_snippet`, `url_snippet`, `date`, `date_filter`, `log_enable`) VALUES ('%s',  '%s',  '%s',  '%s',  '%s',  '%s',  '%s', '%s', '%s', '%s', '%s', '%s', '%s', '%s')",  $form_state['values']['serverurl'],  $form_state['values']['indexname'],  $form_state['values']['username'],  $form_state['values']['key'], '', $form_state['values']['delay'], $form_state['values']['filters'], $form_state['values']['signature'], $form_state['values']['title_snippet'], $form_state['values']['content_snippet'], $form_state['values'

check this: http://drupal.org/node/2497

- Creating classes directly is discouraged. Instead, use a factory function that creates the appropriate object and returns it
- Classes and Interfaces should use UpperCamel naming.
- Functions and variables should be named using lowercase, and words should be separated with an underscore. Functions should in addition have the grouping/module name as a prefix, to avoid name collisions between modules.

     // this should actually be: $oss_api = new OssApi($server_url);
      $ossAPI = new OSS_API($serverurl);
      $ossAPI->credential($username, $key);

could be

      $oss_api = opensearch_server_create_oss_api($server_url)->credential($username, $key);

also on the

- opensearch.module lines 564 and below.You should use a template for that amount of HTML.this way that is hardcoded, a themer can't
chenge it without actually hacking the module
- Is there a reason that annotate_schema() isn't opensearchserver_schema()?

and a lot more..actually there is no point continuing.just realized.Please set this back to needs review, after everything is as close as possible to Drupal Coding Standars and run against the Coder module set to minor. (the links i posted above)

Thanks for your patience

The others (starting with "OSS_...") come from our PHP client library.

Then you shouldnt include that in your module and request users to download separately:
http://drupal.org/node/422996

That would save you from having to rewrite a large portion of them to agree with drupal coding standards.

We have updated the code to Drupal Coding Standards and tested under the
coder module with settings minor and found no warnings.

- And we have implemented your suggestion regarding the template file.
and added a opensearchserver.tpl.php file to display the search result.

- Add our php library has also been ported to drupal coding standards.

Please review our module with tag 6.x-1.1-rc1

Thankyou for you help and support.

Sorry i dont have time these days.
here is a quick patch.check it and commit it please

It definitely needs work.
i will come back with more info,once i find time.

Unless someone else volunteers to complete this meanwhile

I still dont have time.I am sorry:(

It seems this module should be a plugin for http://drupal.org/project/search_api Any reason why not?

Just a note ... I noticed a .gitignore file in your repository. Was this intentional?

A few other comments ...

from opensearchserver_form_alter (line 106):

$form['#token'] = FALSE;
unset($form['#token']);

You assign a value, then remove it on the very next line ... unset() alone is sufficient. From my quick look, I don't understand why you're clearing $form['token'] in the first place; but admittedly, I didn't look very hard.

opensearchserver_admin_index_form_submit (line 515)

opensearchserver_populate_db(&$form_state,

This is going to cause a 'pass by reference' error on recent php versions.

opensearchserver_cron (line 542)

if ($delay > $serverDetails->delay) {
db_query("
UPDATE `{opensearchserver}`
SET
`last_indexed` = " . date('YmdHis', time()) . "
WHERE
`serverurl` = " . "'". $serverDetails->serverurl . "'"
);

Numerous SQL statements do not make proper use of db_placeholders, which could result in a vulnerability to SQL injection. Even if you feel the variables are safe, do not use direct string concatenation to include them in your queries.

I would also be worried about XSS attacks ... while I couldn't pinpoint a direct example, I had an uneasy feeling throughout the entire review that some of the values entered in one location could find their way to output without being run through a sanitation check such as check_plain() or filter_xss(). With the number of sql concatenation issues as identified above, I'm not going to continue the review at this point in time ... but I'd recommend taking something like $block_tool_tip->block_text, entering <script>alert('xss vulnerability');</script> as the value, and then browse around the site and make sure you never get the javascript popup.

As per the review given we have improved the code with drupal coding standards ,

-Every printing statements has been checked with check_plain and check_url to avoid XSS.
-The SQL statements has been improved with indentation.
-Db place holders has been used and removed the concatenation in SQL statements.
-And we have checked the module under coder module with option set to "minor (most)" which returned "No Problems Found"

Please review the 6.x-1.x branch and give us feedback.

Thank you.

lib: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Thank you for feedback and review Klaus Purer.For your information the lib folder contains our own php library and its not an 3rd party library and the php library is developed and maintained by OpenSearchServer.And the code has been released under GPL.

From the (http://drupal.org/node/422996) "This policy does not apply to original code written by a project maintainer. For example, if you write an integration library to connect a Drupal module to another API, you may include it in a repository (licensed under the GPL), since this will be the original version of the library".

we haven't violated any terms of drupal.

Thankyou.

But the library is not developed exclusively on drupal.org, right? It is developed here http://sourceforge.net/projects/opensearchserve/ . So it is not the original version of the library. What version of the library is included in your module? Did you modify it?

Next problem: if another module on your site also needs that library and includes it the correct way via the Libraries API, you will get PHP errors because Drupal will include files with the same class names.

So yes, you did not violate any license, but you violate best practices.

@Emmanuel Keller 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

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256#abandonedtwoweekscontact

Thank you for your patience and review . As per the suggestion made by Mr.Klausi, we have removed our own PHP library.
The OpenSearchServer PHP library can be included using Libraries API. The code has been reviewed using the coder module
with minor level.

Thank you once again for your time.

i see you got a 7.x branch as well:)
which branch should i review?

EDIT:
i see you are working on master branch:
http://drupal.org/node/1128202/commits

you should empty it and add an README like this:
http://drupalcode.org/project/newsletter.git/tree/refs/heads/master

and then continue development to 6.x and/or 7.x

Thank you for your feedback. As you suggested we have updated the master branch with the README.txt file.
And updated the 6.x-1.x branch please review the 6.x-1.x branch.

Please don't assign the issue to your self, only the current reviewer should do this.

Ok,this still needs quite a bit more work.

opensearchserever.js
The functions should be namespaced.I strongly suggest converting everything to jQuery:) Mostly the AJAX calls
i suggest reading this http://drupal.org/node/171213

opensearchservewr.install
a lot of indentation issues.
eg:

  $schema['opensearchserver_type'] = array(
    'description' => t('The base table for saved articles.'),
    'fields' => array(
      'type' => array(
        'type' => 'varchar',
        'length' => '255',
        'not null' => TRUE,
      ),
  'name' => array(
        'description' => t('The name of the fields.'),
        'type' => 'varchar',
        'length' => '255',
        'not null' => TRUE,
      ),
      'enabled' => array(
       'type' => 'int',
        'size' => 'tiny',
        'not null' => TRUE,
      )
    ),
  );

should be

  $schema['opensearchserver_type'] = array(
    'description' => t('The base table for saved articles.'),
    'fields' => array(
      'type' => array(
        'type' => 'varchar',
        'length' => '255',
        'not null' => TRUE,
      ),
      'name' => array(
        'description' => t('The name of the fields.'),
        'type' => 'varchar',
        'length' => '255',
        'not null' => TRUE,
      ),
      'enabled' => array(
        'type' => 'int',
        'size' => 'tiny',
        'not null' => TRUE,
      )
    ),
  );

take extra care to 'name' and 'type'

opensearchserver.module

again indentation issues..eg hook_menu() implementation indentation varies..i can see indentation with 2 spaces,4 spaces and 6 spaces within the same function

opensearchserver.tpl.php

There should be no logic inside templates.Please move php logic outside the template and use php just to print values.
Also use t() for strings so that can be translated.

opensearchserver.admin.inc

Again indentation issues.
function insert_database_values : Dont use check_plain when inserting data in the database.Only when you print them.

Use spaces before and after operators.
eg this:
$user_url=$base_url . '/?q=user/';
should be
$user_url = $base_url . '/?q=user/';

Also use ternaries for better readability.

eg this function:

function get_user_url($clean) {
  global $base_url;
  if ($clean) {
    $user_url=$base_url . '/user/';
  }
  else {
    $user_url=$base_url . '/?q=user/';
  }
  return $user_url;
}

should be

function get_user_url($clean) {
  global $base_url;
  return $clean ? $base_url . '/user/' : $base_url . '/?q=user/';
}

Much better:)
But you could actually dont need this function at all..Use url function.
so instead of get_user_url you just do url('user');

rest files: again indentation issues. use everywhere 2 spaces

removing tag

Thanks for the review Mr.Rootarwc
As per the suggestion made by you the following things where changed.

All the files has been updated with 2 space indentation.

The logic has been removed from opensearchserver.tpl.php .
It just iterates over the search result and prints it.

opensearchserver.js has been changed to namespaced and the
AJAX calls has been made through jQuery.

The check_plain has been removed from the SQL insert statement.

The module has been checked with coder module with option set to "minor (most)" which returned "No Problems Found". And please review the 6.x-1.x branch.

Did again a quick check and still see indentation issue in .install file at least.
Also in the tpl.php i still see more php than html.
variable assignement should happen outside the tpl:)

Thanks for the review Mr.Rootarwc
We have checked the .install file and did not find any issues in that.
Can u please explain what is the indentation issue in the .install file?
In the .tpl.php file the logic and the variable assignment. has been removed

Please check the 6.x-1.x branch and give us feedback.

Thankyou once again for your support.

lines 132-134 and 138-140 two extra spaces

We have removed the two extra space in .install file.
Please review the 6.x-1.x and give us feedback.

Thankyou once again for your support.

Sorry, busy again.
We really need more hands in the application queue and highly recommend to get a review bonus to get this done sooner ;)

Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure.

Also, at the moment the 3rd party files are still contained in 7.x-1.x. Please make sure that also the d7 version of this module uses libraries correctly. It would definitely make sense to implement hook requirements and check whether the necessary library files are available / tell a short introduction where to download and where to put it to in the status report page. (usage and installation instructions should also be contained on the project page which is currently pretty short)

An automated review (drupalcs not coder in this case) of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.
You can test it yourself on http://ventral.org/pareview

D7 has class lazyloading, you can add these 3rd party files to drupal's registry and they'll be included automatically when needed and not on every bootstrap. Same with your d6 version; I'd recommend depending on the autoload module which provides the same functionality so you don't have to include these files by hook_init() every time.

$msg= 'The OpenSearchServer library could not be found Or The Library API not found.';
drupal_set_message(check_plain($msg), $type = 'error');

Always (except within hook_menu(), hook_schema() and watchdog()) encapsulate human readable string with t().
Also it makes no sense here to use check_plain on $msg as it is a static string and can't contain any user input. I'd suggest you to have a look at the writing secure code chapter.
Your also doing $type = 'error' what makes not very much sense and probably happened by c&p from the api site.

the .info of your d7 version says core = 6.x

there's no hook_perm in d7, it's hook_permission now and has changed a little

seriously do not use t() within hook_menu()

$form['#token'] = FALSE;
unset($form['#token']);

what's this for?

Your documenting all your functions with

/**
 * Implements form validation.
 */

The "Implements ..." is only for hooks! See documentation: https://drupal.org/node/1354#hookimpl

well; don't get me wrong, but this looks a little messed up, IMHO I'd really suggest you to take a weekend and rework both versions of this module completely before we continue here :/

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