[D7] Onki

Hi,
I do automated project review, and there's many errors of coding style, you should check it in ventral.org.
And after I install it, there something collapsed. See screenshots.

Sorry I just review for layout, and for module work I'm still looking up.
( I'm also beginner in drupal.)

I did a line-by-line visual and test review with teelmo @ drupalcon today.

The utility this provides is useful, appropriate, and leverages and provides a useful service. It's a good thing to have.

Suggested that exactly *what* it does and why you'd want it should be added to the top of the project page above the technical details. (screenshot or diagram?)

No direct conflict with other modules, but do compare with (and add a link to cross-reference to) web_taxonomy project which has a number of similarities thought the method and architecture is different.

* minor indentation cleanups recommended - todo
* an amount of redundant unused code (functions from earlier attempts & SOAP stuff) to remove (hook_install & the URI funcs).
* old 'features' inc files to remove.

* Configuration settings page - very good and clear.
* Proper use of Drupal APIs throughout : 90% (some DB queries could be DBTNG, but they are not wrong)
* No apparent security issues
* well structured, well commented code.

However
* didn't actually work on a fresh install. That should be fixed offline.

Make it *work* and get a clean report from Ventral, and I'd say the *code* review is done and it should be RTBC.

.dan.

i think you should remove lines that you make them comment in development
like this one in line 141 in .module file

        // '#required' => TRUE,

and also in line 191

      // $matches["<create>$tags"] = t($string, array('%term' => $tags));

it's better that you user @return to explain about return type and object of a function
for example in lines 526 to 531

/**
 * Helper function for testing Onki service.
 *
 * Helper function to test if ONKI service is up and runnig or not.
 * Return TRUE if ONKI is running and FALSE if a timeout occurs.
 */

if you fix theese your code is great

thank you ,
Mohsen

Improved instructions

Don't RTBC your own issues, see http://drupal.org/node/532400

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 I'll take a look at your project right away :-)

There are still some issues with the automated PAReview:
http://ventral.org/pareview/httpgitdrupalorgsandboxteelmo1596454git

Manual review:

• How does Drupal handle adding a different version of the jQuery UI? In my experience it gives a lot of strange errors.
• It is recommended to use db_select instead of db_query on line 287 in onki.module.
• In your CSS I see some CSS3 prefixes, maybe you can make a notice somewhere which browsers are supported.

Hi,

I see that you have a 'onki.features.field.inc' file in your repo. What's the purpose of this file? I don't see any references to the features module or usage of this file.
Why do you set a status message after uninstalling your module? When the uninstallation process doesn't throw any errors, it is very obvious that it is correctly uninstalled.
If db_query is better than db_select is a point of discussion, but for now I do not see any real issues here after reviewing the query.

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.

Reopened after fixing the following:

* Unnecessary file removed
* Code cleaned to be compliant with ventral.org
* Drupal_set_message removed from uninstall hook
* Spelling errors fixed and comments cleaned

Reviewed this with jaysire at Drupalcon.

manual review:

1. onki_enable(): that can be removed because yxou can use default values with variable_get().
2. "'access arguments' => array('create article content'),": you should probably use your own permission instead?
3. "'#autocomplete_path' => 'onki/autocomplete/1',": where does the "1" come from? Should still work without it?
4. "if (module_exists('arpa')) {": that seems like an internal module, so you should add a comment.
5. $form['onki_dialog']['onki_dialog_btn']: do not use custom markup, use the submit type of the form API. See http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.ht...
6. onki_form_after_build(): docs are wrong, this is not a hook. See http://drupal.org/node/1354#functions
7. onki_form_after_build(): i think you don't need that function, just use #attached for JS and CSS in the form building function.
8. onki_autocomplete(): you are loading user supplied text from the term table, so you need to sanitize it before printing to avoid XSS. Please read http://drupal.org/node/28984
9. onki_search(): most function docs do not tell me anything, you should improve that.
10. "htmlspecialchars($r['label'], ENT_COMPAT, 'UTF-8');": use check_plain() instead.
11. onki_search(): you should not sanitize here, but in the function that actually prints the data to the user (in this case it is onki_autocomplete()).
12. onki_json_concepttree(): I'm not sure where the tree descriptions are sanitized? You can do that either in Javascript or in PHP before delivering the ajax results.
13. onki_taxonomy_term_presave(): When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database. See http://drupal.org/node/28984 . So I guess you can remove this function as a whole.
14. "'#title' => onki_get_description($term->name),": that also looks suspicious, make sure that the description has been sanitized somewhere.
15. "'fi' => 'Finnish',": all user facing text should run through t(). Please check all your #options values.
16. throbber.gif: is that the standard drupal throbber? I think you can remove that file.

Thanks for the help @ Drupalcon in Portland, Klausi! I've now addressed all the issues and cleaned up the module considerably. It should be ready for community testing now.

In onki_menu() you have 'access arguments' => array('create article content') which doesn't really match up to your use case. Since Onki is a tags taxonomy, perhaps you could check to see if the user has permission to use/view that vocabulary instead? Or maybe create your own permission like klausi suggested?

There's also a fair number of code style issues found at: http://ventral.org/pareview/httpgitdrupalorgsandboxteelmo1596454git.

The syntax issues are minor, but setting to needs work for the access issue. Otherwise this is RTBC from me.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Thanks for looking at the module, kscheirer!

What is the best practice concerning custom permissions? Would it be ok to automatically grant these permissions to administrators from the install hook of the module or should it always only be the responsibility of the module user to decide who gets what permissions?

I have addressed all of the coding style issues (except some comments split on several lines that still get warnings).

I've also created a custom permission "use ONKI" instead of using built in ones. The "use ONKI" permission is granted to administrators automatically in the module install hook. Sorry if this is against best practice.

Thanks! Using a custom permission and adding it to the admin role seems fine to me. There is a variable called user_admin_role that will give you the rid of the admin role - that's a little safer than querying for "administrator".

----
Top Shelf Modules - Crafted, Curated, Contributed.

I'll look at this now in the Project applications sprint

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

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

  
  FILE: /home/klausi/pareview_temp/onki.module
  --------------------------------------------------------------------------------
  FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
   168 | ERROR | Function comment short description must be on a single line
   213 | ERROR | Function comment short description must be on a single line
   235 | ERROR | Function comment short description must be on a single line
   247 | ERROR | Function comment short description must be on a single line
  --------------------------------------------------------------------------------
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/onki.admin.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
    87 | WARNING | Unused variable $ip_address.
   143 | WARNING | Unused variable $node_type.
  --------------------------------------------------------------------------------
  

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.

But otherwise looks good, so ...

Thanks for your contribution, jaysire!

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.

The full project for Onki (ontology service now called Finto) can be found at https://drupal.org/project/onki