[D6] Taxonomy Node Filter

I think you need to attach an archive of the actual code for your finished module...

You might want to read http://drupal.org/node/539608
Especially the bit about "Here is a list of common motivation messages that, more often than not, get declined/rejected. If your motivation message resembles one of these then please, think before applying.

[....]
* I am planning to create modules / themes. Who applies for a CVS account should have already a module / theme. The purpose of a CVS application is to review code, and verify the degree of understanding about how a Drupal module is written; without a module there would not be a review, and there is not CVS application. For the same motivation reported in the previous point, future CVS applications reporting only this motivation will be declined. "

There have not been replies in the last week. I am marking this application as won't fix.

1. See http://drupal.org/coding-standards to understand how a module should be written; in particular, see how the code should be formatted.
2. Submission functions don't use drupal_goto(), in Drupal 6.
3. The first argument of t() must be a literal string; differently, the script that extracts the string to translate to create the translation template will not be able to extract the string, which would not be translatable (if not in the case another module uses the same exact string, but it's rather difficult it happens, when the string is dynamically changed).
4. Strings used in the user interface should be in sentence case.

Remember to change status, when you upload new code, or nobody will notice you updated the code.

The field assigned is used to mark who will review the code for this application; who is applying for a CVS account cannot review is own code, for obvious reasons. :-)

• The points reported in this review are not in order or importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. The version line needs to be removed from the .info file.
2. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.
3. The module doesn't implement hook_uninstall().

Hi khoomy,

I'm not an expert, but I'm having a look to see if I can help this application along...

Good:
1) A README.txt hooray! You'd be surprised how many people submit modules without even that. Although it's not required, you wonder why people create modules without explaining to people how to use them. And named README.txt, hooray!
2) Coder finds 0 errors :-)
3) Install does variable_del()
4) t() used for translations
5) Every function commented
6) No SQL, so no problems there

Not so good:
1) README.txt tells me how to install, but many users (such as me!) will use this file as a primary source of information about what the module does and how to use it. I can see that you can write good docs when you want, so go for it! Though README.txt is not obligatory (for reasons I don't understand), so that's not blocking.

Nothing I see wrong with this application. For me it's RTBC.

Hi,
First, this is a really great utility :) I haven't done a survey of all the taxonomy related contributed modules, however just reviewing functionality this module provides, seems really useful.

You module provides a form that allows the user to select nodes based on multiple taxonomy terms. First a couple design comments:

• I would expand the narrative in your hook_help implementation. Currently users are restricted to constructing a logical "AND" query (when selecting from multiple vocabularies). Therefore, it would help folks if you explicitly explained the query they are building is an "AND". For example, "select all nodes that have a value of cat in vocabulary animal AND a value of women in vocabulary gender."

  At first, I thought the module was returning incorrect results. However, that's because there is no indication whether the query is a logical "AND" , or logical "OR".

  To summarize, it works fine as a logical "AND", however your documentation should spell that out to users.

• I would consider adding additional form controls that allow the user to select "AND" or "OR". That would allow the use to build a query like : "select all nodes that have a value of cat in vocabulary animal OR a value of women in vocabulary gender." . Just a thought.

Next a technical suggestion:

• In your form submit handler (taxonomy_node_filter_form_submi() ) you are using the $form['#post'] array to process user input. Please consider using the $form_state['values'] array instead, Using $form_state is the Drupal standard. For example, an alternative construct would something like:

  $terms_selected = NULL;
  for ($index = 1; $index < count($form_state['values']); $index++) {
      if (isset($form_state['values']['vocab_' . $index])) {
        if (!empty($form_state['values']['vocab_' . $index])) {
          if (is_null($terms_selected)) {
            $terms_selected .= $form_state['values']['vocab_' . $index];
          } else {
            $terms_selected .= ',' . $form_state['values']['vocab_' . $index];
          }
        }
      } 
      else {
        break;
      }
  }
  

• Also at the end of the form submit handler taxonomy_node_filter_form_submit() you should add a verification that at least one taxonomy terms has been selected before redirecting. You might consider rendering a message if no taxonomy term is selected. For example, something like :

  if (!empty($terms) ) {  
   drupal_set_mesage(......);
  } 
  else {
   $form_state['redirect'] =......
  }
  

Hope that helps.

I am changing status as per previous comment.

Hi,
I installed the version of your module posted in comment #21. I installed and tested on a Drupal 6.20 installation.

Nice job! Really close.

Here are my issues:

1. You've extended the functionality to allow folks to add multiple instances (blocks) and custom configurations per instance (block). Really nice. I would just add a small corresponding elaboration in both your hook_help() and your README.txt file.
2. In your hook_uninstall() implementation, I would explicitly perform a variable_del for each entry in the variable table (that you module set). Currently you are looping through each of values stored in the global conf variable array instance. Then deleting whatever array element contains the pattern ''taxonomy_node_filter_'. This is analogous to folks deleting from the 'variables' database table, selecting records to delete based on a pattern. From reading other reviews in this queue, that's not a preferred method. You introduce the chance that you are deleting someone else's variables.
3. In general, the variable table should be used sparingly. I don't know if there is a specific guide, but if every module makes heavy use of the variable table, not only will the table, but the global conf variable size become a performance issue.
   
   
   In your case, really nice feature to allow folks to set 1 to 10 blocks. However you are now close or at the point to where your settings should be persisted in a separate database table.
   
   
   It looks like you are storing up to 21 entries in the variable table. Is that correct?
   
   
   I'll leave it at that. Maybe other reviewers have a better grasp of what the threshold is for use of the variables table.
4. I noticed you now have a call to drupal_add_css(). However, you are calling it in a hook_form() implementation. I believe for performance reasons, drupal_add_css() should be called from within hook_init().
   
   
   If you wanted to style the form you would implement hook_theme() first. That tells the theme engine that your module is going to style (theme) some components. Next you would set a #theme element in your form which points to a new method like theme_taxonomy_node_filter_form().
5. In your method taxonomy_node_filter_block() please consider replacing the multiple elseif constructs performed on $op (operation). Please use a switch/case construct instead.
6. That brings me to my final point. Again really nice utility!! I would consider providing a theme, theme preprocess, and a taxonomy_node_filter_block.tpl.php. Together, all three of those components would allow themers to custom the style of the great blocks your module provides. Something to consider. I'm actually not sure whether this is a requirement or not. I've done it for the modules I write.

Hope that helps.
Lorin

Hi,
I reviewed the version of your module posted in comment #26. I installed you module on a Drupal 6.20 installation. Nice job, real close, just a few issues:

• Coder is still reporting some formatting issues. Please make sure you run you source code through Coder. Set Coder's options to "minor" and "Internationalization". For example, here is the output from Coder:
  

  Line 204: table names should be enclosed in {curly_brackets}
  
     db_query("DELETE FROM taxonomy_node_filter  WHERE delta = %d",$delta);
  

  So all you need to do is change that line to:
  db_query("DELETE FROM {taxonomy_node_filter} WHERE delta = %d", $delta);

• On my installation, the following raised an exception: function theme_taxonomy_node_filter_form(&$form) {, the parameter is supposed be passed by value rather than reference. Thus please consider changing to: function theme_taxonomy_node_filter_form($form).
• In your method function taxonomy_node_filter_uninstall() { you a statement global $conf;. Sorry, I couldn't figure out what purpose that line serves. I comment it out and tested you uninstall routine. Worked without the global $conf;, there I would consider removing that line.

Again, great job, really close.
Hope that helps.
Lorin

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.

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.

IMHO good to go

Congratulations, you know can promote sandbox projects to full status ones!

Nice work and when you would like further review for any module please do ask for one at http://groups.drupal.org/peer-review/requests

A little bit of cleanup/checking called for-- you call your implementation of hook_menu() "Implementation of hook_block()." ;-)

_{benjamin, agaric}