[D6] Agrovocfield

thanks for this module

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

Just a short note, since you were already using git, you could have just added the new repo on d.o as a new remote and push it. The advantage would be that the history wouldn't be lost and it would show all your existing commits separate, making it visible that you are continously working on this module and it's not a one-off thing :)

I am not sure if this is still possible because you already pushed now, but maybe next time...

Hello,

Here is my quick review :

You should remove LICENSE.txt, it will be added automatically by d.o.

All $Id$ lines are not necessary anymore since CVS is not used anymore.

In your agrovocfield.module file :

    case 'validate':
     break;
    case 'sanitize':
      break;

Why do you use this instead of "default" ?

function agrovocfield_get_term_by_name($name) {
  $db_result = db_query("SELECT t.tid, t.* FROM {term_data} t WHERE LOWER(t.name) = LOWER('%s')", trim($name));

Nothing big, but why t.tid AND t.* ? t.* should be enough, should it not?

Also, I'm not sure, but I think it is more performance wise to describe the fields one by one if you know what you want (I'm kinda itchy at this star :p).

Found some trailing whitespaces in your agrovocfield_autocomplete.module file.

Except for these little things, the code looks really great! Havent tested it though.

Hi,
Thanks for your contribution and sorry about the long wait!

I did a code review on your module, and overall I think it looks well done. The module follows coding standards well, though there were a few indentation issues and that's it. The module has a README file, no LICENSE files, etc. I'm prepared to mark this RTBC once a few issues are resolved. Looking at your code and your recent posts, I think you have demonstrated your abilities as far as I'm concerned. But a few things to note:

Comments and names should use US English spelling (e.g., "color" not "colour").

http://drupal.org/coding-standards
Drupal coding and commenting standards dictate that the standard language is US english. So, all of the

/**
 * Implementation de hook_field().
 */

type comments should be

/**
 * Implementation of hook_field().
 */

Also, if properly formatted, comments should look like this (from line 201 of agrovocfield.module

/**
 * Adapted from taxonomy_get_term_by_name function.
 *
 * @param string $name
 *    The term that was searched.
 * @param integer $vid
 *    A vocabulary ID.
 *
 * @return
 *    An array of objects for each term found.
 */

Additionally, comments should follow sentence structure, starting with a space, capital letter, and end with a period, and inline // comments are generally discouraged (like unset($obj->labels); //reseting container).

These are pretty simple fixes that shouldn't take long. Your code demonstrates an understanding of PHP. the Drupal API, and makes good use of the Content API.

Lastly, you might want to include some instructions on using the module in your README.txt file rather than only describing what it does.

Hi,

Actually, "Implementation of..." should be "Implements...".

Indeed that be the new format, though I've gotten mixed signals on whether D6 modules must use this format. All of Drupal core in D6 still uses Implementation of, as do most D6 modules, and many modules complete this review process with the same. Indeed, I've seen it expressly mentioned in this queue that using this format is okay for D6 modules. But perhaps the safest bet is to use the new format, be bold (while I get clarification from the rest of the review team)!

Agh. Once you correct any issues the application needs to be set back to needs review. That's why it got lost in the sauce!

Elevating priority to get this checked out again. I am busy at the moment but will check it out again soon if nobody beats me to it.

I hope this module gets reviewed soon and is released as an official module, as we strongly need something like this.

The existing Agrovoc module only uses taxonomy, while this one also stores the URI in the CCK field, which is essential for developing Linked Data-enabled websites. Having been developed in very strict collaboration with the group in the Food and Agriculture Organization of the United Nations (FAO) that manages the Agrovoc thesaurus, this module has the necessary functionalities in order to fully integrate Agrovoc indexing and .linking to the Agrovoc URIs published as Linked Data.

We are already using the module, but it would help our community (http://aims.fao.org/community/group/agridrupal) to have it published on drupal.org so that it can be easily shared and upgrades can be monitored.

Just to add more noise, a committed maintainer that is working six months to get his module pushed is (at least for me) the best prove that this is a project and not just a pastebin. I encourage the approval of this module.

Regarding module duplication, why not merge functionality with the agrovoc module? Especially since you say you are in contact with that person to coordinate development.

Adam, I'm so sorry this is taking so long. We are severely short on volunteers. I'm looking over the code right now, and will post comments tonight or tomorrow. Definitely don't add more comments to bump up the issue. Reviewers can actually skip it over because of that. They see it being commented on, so they think it is being reviewed. Thank you for your patience.

Thanks for your README. It is very helpful.

I get a lot of these errors:

Fatal error: Class 'SoapClient' not found in C:\wamp\www\d6\sites\all\modules\agrovoc\agrovoc_api.module on line 30

I know it is not from your module, but it is preventing the autocomplete from working, and the node from saving. It does not show any autocomplete suggestions, but it only fails when typing something that should autocomplete, like "chicken". I am doing this on Windows. I will try Linux tomorrow. Otherwise, I cannot get it to work, so I'll have to review the code only.

Coder does hit on this:

Line 272: Potential problem: "SELECT FROM {node}" statements should probably be wrapped in db_rewrite_sql() and with the alias for {node} table defined (e.g. {node} n)

I'm looking at the code now to see if it necessary in this use case, but I don't immediately see a reason not to use db_rewrite_sql(). http://drupal.org/writing-secure-code

Still looking. Regardless of whether I can get it working, I just want to make sure there are no obvious security issues. I should be finished reading it all by tomorrow.

Thanks for the information. I set this up on Ubuntu this evening and it is working without any errors. I will try to finish the review very soon.

Use t() for the label and description in hook_field_info(). The agrovocfield_autocomplete_widgets_settings() function seems to have no purpose. Other than that, nothing jumps out at me. Add the t() function and I'll mark it reviewed, as long as no one else has any comments.

Congratulations and thanks for your patience. Based on davidhernandez' review I've granted you the git vetted user role.

Indeed the db_rewrite_sql was necessary for security reasons. Adding a tag to track that.