Taxo Faceted Navigation

The review was too large to be pasted in here.
I'll attach it on the next comment.

Here it goes:

Please try fixing these coding standart issues by using http://ventral.org/pareview.

Switched back to needs review, so in-depth reviews won't be blocked by coding standart issues.

added bit about drupal 7

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• taxo_faceted_navigation_block_info(): you are repeating yourself. Use a loop to generate the blocks.
• taxo_faceted_navigation_block_view(): same here.
• "@return unknown_type": you don't know what you return?
• "'access arguments' => array('access administration pages'),": do not use this generic permission, create your own.
• taxo_faceted_navigation_menu(): if title and description are empty, you do not have to provide them at all.
• "// drupal_set_message('url_arr'.print_r($url_arr,1));": remove all debug statements from your code.

The response time for a review is now approaching 4 weeks. Get a review bonus and we will come back to your application sooner.

i installed your module, and the permissions showed up just fine. Not sure if you fixed that in the mean time though... On to some other stuff:

Here is what i did:

1. Created 3 vocabs
2. Included each vocab in the taxo facet navigation, and set weights of 1, 2, and 3 for vocabs 1, 2, and 3.
3. created 3 terms in each vocab
4. Set url term path pattern to [term:name]
5. Made sure all paths were updated.
6. Set first argument to 'article' and left the checkboxes unchecked, and set 3 blocks
7. Put the three filter blocks into the left side bar, arranged by weight.
8. Created 3 term reference fields on the article content type, 1 for each vocab.
9. Created a bunch of content tagged with terms from each vocab.

When i view the page /articles, i get this error 6 times.:

Notice: Undefined variable: url in taxo_faceted_navigation_get_term_url_alias_from_tid() (line 512 of /Users/Chris/drupal7/sites/all/modules/taxo_faceted_navigation/taxo_faceted_navigation.module).

In addition, the links for the terms in vocabs 2 and 3 are set to '/article'.

I followed the configuration directions included in the README.txt, but apparently something is wrong. If i did something incorrectly, please update the readme, and let me know.

There are also a LOT of typos throughout all of your code. Not huge deal for comments, but things displayed to the user should probably be spelled correctly (a couple things in the settings form).

Also, it would probably be helpful if you named your blocks a little better. Instead of:

 $blocks["filter$i"] = array(
         'info' => t("Filter$i"),
         'cache' => DRUPAL_CACHE_PER_PAGE,
       );

Maybe:

 $blocks["filter$i"] = array(
         'info' => t('Taxo Facet Filter :i', array(':i' => $i)), // You shouldn't pass variables directly into t(),
         'cache' => DRUPAL_CACHE_PER_PAGE,
       );

I think this is a great idea for a module, and definitely see some potential uses.

Hi Darko

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732. In other words your master branch should be empty.

There some Drupal coding standards, check http://ventral.org/pareview/httpgitdrupalorgsandboxdarkokantic1334080git....

Please check correct indentation and punctuation in your code and comments.

Good work! Your master is clean :)

However, add some full-stops in your comments, eg. taxo_faceted_navigation.module lines 4 and 6. Always begin comments uppercase and end with fullstop. There are also some grammatical issues, but nothing serious.

Keep going!

Hi,

You must add a .install file to delete variables when uninstalling with variable_del()

You should never use t() to translate variables
Ex : $block_arr['title'] = t(taxonomy_vocabulary_load($vocabs[$arr_index_no]['vid'])->name);

There are encoding errors in README.txt
107 Home � Administration � Structure � Taxonomy � Computer Hardware

Only applications with a status of needs review that have been awaiting response from a reviewer for 4+ weeks can have the "critical" status. See http://drupal.org/node/894256

You have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698

I suppose this will become a very interesting module especially for e-commerce sites.

Ventral did not find any errors, and at a first glance I did not see any severe problems.
I found many spelling errors (even I did not search for them), and I find some of your comments are uneeded and better function names would be beneficial.

Here my review:

1. You should remove INSTALL.TXT as it does not contain any information.

2. You should place your template files into a "templates" directory

3. Check typos:

 * Theme your own "remove filter" <strong>sighn</strong> here.
 * <strong>Wrups</strong> up the block.
 *  For each vocabulary id that is passed as argument output <strong>meny</strong> tree. Array
 *  of <strong>meny</strong> tree is passed trough theme function at the end, so themed
 *  output is produced.
 *   Taxonomy tree, but only one level, either first level if <strong>coled</strong> first time,
 *   or first level children of the current level.
      // Do this check only if item is last leaf
      // and if <strong>not</strong> filter applied.

4. I'm not sure if shouting at the user is a good idea:
'#title' => t('Do NOT display link if empty'),

5. I suppose there should be no \n and unnessary spaces in t() functions.

    '#description' => t('Do not display link if there are no nodes under particular link, but also taking into 
     account current filter selection. I.e if there are no nodes for current filter combination. Works well only for flat hierarchies, i.e single level hierarhy'),
  );

... also I find it difficult to read some of your phrases, it would be better making them short and clear.

6. I would add a manula page break between the phrases, for readability.

  * Provides block for progressively filtering content. There is no limit on
 * number of blocks it can provide.

7. No reachable code:

function taxo_faceted_navigation_theme() {
    return array(
      'taxo_faceted_navigation_menu_template' => array(
        'variables' => array('taxo_menu_item' => NULL),
        'template' => 'taxo_faceted_navigation_menu-template',
      ),
      'taxo_faceted_navigation_ul_wrapper_template' => array(
        'variables' => array('ul_sub_menu' => NULL),
        'template' => 'taxo_faceted_navigation_ul_wrapper-template',
      ),
      'taxo_faceted_navigation_removefilter_template' => array(
        'variables' => array('ul_sub_menu' => NULL),
        'template' => 'taxo_faceted_navigation_removefilter-template',
      ),
    );
  // Will raise fatal error if void.
  <strong><em>return array();</em></strong>
}

8. You could save some of your inline comments, by putting better variable names:

  // We need this so we know when we hit last item.
  $no_of_items = count($tree);

9. Comment does not give any additional information:

    // Check if current term in this loop has any children.
    $children = array();
    $children = taxonomy_get_children($term->tid, $vid);

10. I suppose this function could be used more generic as it's name implies:
$term_has_nodes = taxo_faceted_navigation_check_if_term_has_nodes_underneath($vid, $term->tid);
I'll propose: taxo_faceted_navigation_get_subnodes
or: taxo_faceted_navigation_has_subnodes

11. You call twice the same function:

   <strong> $term_has_nodes = <code>taxo_faceted_navigation_check_if_term_has_nodes_underneath($vid, $term->tid);</strong>

    // Work out if we will display this item or not.
    $display_item = TRUE;
    // User preference is to NOT display link if empty, i.e no nodes underneath.
    if ($do_not_display_if_empty) {
      $has_nodes = <strong>taxo_faceted_navigation_check_if_term_has_nodes_underneath($vid, $term->tid);</strong>

if (!$has_nodes) {
$display_item = FALSE;
}
}

12. I do think that many of your inline comments are not necessary. Example:

function taxo_faceted_navigation_taxonomy_term_get_siblings($tid, $vid = NULL) {
  // Get term parent, if no parent it means its first level,
  // in which case just get first level terms for vocabulary.
  $parent = taxo_faceted_navigation_taxonomy_term_get_parent($tid);

  if ($parent) {
    // There is parent, so get all children of a parent.
    $siblings = taxonomy_get_children($parent, $vid = 0);
  }
  else {
    // No parent so it means its first level, get first level children,
    // i.e siblings.
    $siblings = taxonomy_get_tree($vid, 0, 1, FALSE);
  }
  return $siblings;
}

The code should be self-explained (especially if variables and functions are named correclty), and most of your comments only slow down the reading of the code.

13. Writing "the problem" assumes, people know all the problems you got during development. Would be better writing "a problem" ...

 * Preprocess node url and append argument to it.
 * This function is used to fix the problem where menu tree collapses
 * if you go into the node.

Removed PAReview: review bonus

You may add 3 new reviews and put it back.

You could consider starting by reviewing the following module if you had some time to offer: http://drupal.org/node/1302786

sorry I ment line break ^^

@luxpaparazzi: Never tell people to remove comments from code! Comments can be more clear or variable names can be improved, but code is *never* fully self explaining and comments help a lot to understand what is done. There is no such thing as self explaining code.

Thanks for your reviews, just make sure that you pick applications that did not get a review in a long time.

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

manual review:

1. taxo_faceted_navigation_form_alter(): All user facing text must run through t() for translation ("Vocabulary", "Vocabulary order"). And the dynamic variables must be passed with placeholders to t(). Also this is vulnerable to XSS exploits right now if the vocabulary name contains malicious script tags.
2. taxo_faceted_navigation_get_vocabulary_name(): Is that function really needed? I can just use taxonomy_vocabulary_load() and access the name property from the returned object.
3. taxo_faceted_navigation.module: "include_once 'taxo_faceted_navigation.taxoadmin.inc';" why do you have to include that globally? It is a best practice to keep all hook implementations in the module file, so you would not need that then.
4. taxo_faceted_navigation_block_info(): indentation errors, always use 2 spaces per level.
5. taxo_faceted_navigation_block_view(): indentation error on the return statement.
6. taxo_faceted_navigation_get_blocks(): this is vulnerable to XSS exploits, the vocabulary name is not sanitized. See http://drupal.org/node/28984
7. taxo_faceted_navigation_theme(): indentation errors, always use 2 spaces per level, not more.
8. "'access arguments' => array('access administration pages'),": The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations. Or you could of course provide your own permission.
9. taxo_faceted_navigation_get_selected_filters(): indentation errors in the nested if clauses.
10. taxo_faceted_navigation_removefilter-template.tpl.php: all user facing text must run through t() for translation.
11. taxo_faceted_navigation_removefilter-template.tpl.php: where do you sanitize the term name before printing it? I could not find it, maybe I missed it.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Forgot attachment.

Some typos / grammar issues:
- eaxample => example

- the order of Filters => the order of filters

- persistant => persistEnt

- t('Used for ordering terms in the urls, the lightest vocabulary will go to top (or beginning of the url), the heaviest will sink to the bottom (or end of url). Used for ordering of terms in the urls only, to order filters on the site go to the blocks admin page, Administration » Structure >> Blocks .')
I suppose there should already be some translation for "Administration", "Structure" and "Blocks", so I prefer using those translations, and concatanating the different t() blocks.
Also use the same separator "»" is different from ">>"

-

// Save variable.
// Get whats there first so we can add to it.
$filters = array();
$filters = variable_get('taxo_faceted_navigation_taxonomies', $filters);

I don't get the 2nd comment-line.

-

// Filter is NOT already there so add it.

If it's not there it's not there "already" does not make any sense here. (Maybe use "yet").

- Using "is not set" would also be better than using "is not there"

- Instead of urldecode, you should use drupal_encode_path: http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_enc...

function taxo_faceted_navigation_get_nodes_based_on_intersect_of_terms($selected_filters, $node_types = NULL, $text_compare = NULL, $text_compare_middle = NULL) {
  $tids = array();
  foreach ($selected_filters as $filter) {
    $tids[] = $filter['tid'];
  }
  $joins = ' ';
  $wheres = 'WHERE n.status = 1 ';
  if ($node_types) {
    $wheres = ' AND n.type in (' . $node_types . ')';
  }

  foreach ($tids as $key => $value) {
    $joins .= 'INNER JOIN {taxonomy_index} ti' . $key . ' ON n.nid = ti' . $key . ' .nid ';
    $wheres .= ' AND ti' . $key . ' .tid = ' . $value . ' ';
  }

  if ($text_compare) {
    $wheres .= 'AND n.title LIKE \'' . $text_compare . '%\'';
  }

  if ($text_compare_middle) {
    $wheres .= 'AND n.title LIKE \'%' . $text_compare_middle . '%\'';
  }
  // $wheres .= ' AND status = 1 ';
  $order = 'n.sticky DESC, n.changed DESC';

  $sql = 'SELECT  n.nid 
          FROM {node} n ' . $joins . '   
          ' . $wheres .
          ' ORDER BY ' . $order;
  // $sql = db_rewrite_sql($sql);
  $result = db_query($sql);
  // Convert array of objects to array.
  $arr_result = array();
  foreach ($result as $record) {
    $arr_result[] = $record->nid;
  }
  return  $arr_result;
}

You should really use correct form building functions, see http://api.drupal.org/api/drupal/includes!database!database.inc/function...
This is a security issue! You did it correctly at other places.

added new rewievs

>> Instead of urldecode, you should use drupal_encode_path:
> Where in the code is this relevant
I suppose I found "urldecode" somewhere in your code ... don't remember exactly.

>> You should really use correct form building functions, see ....
> You should really use correct form building functions, see ....
The example is beneath my comment.

you did not add it, you removed it ;)
please leave it for statistical purposes

Just wanted to point out that there is some duplicate effort here, which I am definitely not opposed to but wanted to throw out there.

The Facet API module works with Apache Solr Search Integration, Search API, and core Search. It also has a Facet API Pretty Paths module that can be used to construct clean URLs.

Again, I am not opposed to duplicate effort because I think that it is healthy for projects to push one another, but I just want to make sure that the search solutions we are creating don't further fragment the community. We have done a lot of work to start re-using code across the various search projects specifically in the area of faceted navigation, and I wouldn't want to go the other way unless absolutely necessary.

Thanks,
Chris

The git repository seems to be unavailable.
Where can I get this so I can write a review?

Regards

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/DarkoKantic/1334080 taxo_faceted_navigation

works without any problems for me

There has been ongoing code review for this module, so I'm trying to focus on items here that have not already been addressed.

1. Automated code review is still finding indentation errors. Darko, I think you misunderstood Klausi's comment. Example:

function taxo_faceted_navigation_theme() {
  return array(
    'taxo_faceted_navigation_menu_template' => array(
    'variables' => array('taxo_menu_item' => NULL),

The fourth line should be indented two more spaces.

2. I reviewed the duplication referred to in http://drupal.org/node/1359516#comment-5942340.

These modules seem to do similar things, but they do them in very different ways and for different reasons. I applied the test "could reasonable contributions to another module produce the same functionality?" My answer was no. This module is valuable on its own.

3. USER EXPERIENCE (not code): I found the blocks difficult to manage. These aren't like menu_blocks where there might just be a few on the site - I could easily imagine dozens of them with enough vocabularies, especially on an e-Commerce or media/publishing site with extensive tagging. Once you have a few dozen it gets hard to keep track of them and differentiate when you move them around.

I was also confused about where to go to configure the blocks once I added them. (I expected to be able to configure them in the blocks page, when configuring each block.) Also, it was even more confusing if I changed which vocabularies mapped to these blocks. Removing a vocabulary from the list and then adding it back or adding a different one didn't behave as I had expected.

Your comments in the code don't make this much clearer:

 *  I.e. if user selected vocabs 3,7 and 1
 *  to be used as taxo facets we print menus for these 3 vocabs, in this order.
 *  So for argument $arr_index_no =  1 we print menu for vocab 3,
 *  $arr_index_no =  1 we print menu for vocab 7 etc.

You might want to consider an alternate mechanism similar to what menu_blocks does. Instead of asking the user to specify a block count and then configure a vocabulary to automatically appear in the next available block, in your admin screen provide an "Add Block" function that adds a new block. Then implement hook_block_configure()/hook_block_save() to give the user a simple setup form where they can pick the taxonomy. This would be a lot easier to understand and act more like other blocks. (You could also ask for a title so it didn't have to match the taxonomy vocab title.)

hook_block_info() should also show more than "Taxo Facet Filter 4". What is filter 4?

4. In hook_block_info() your delta pattern is "filter$i" and then in _view() you have to do extra work to get it back. Delta values are unique to your module. You can just use $i to save some code.

This is important for performance reasons because in block_view() your current logic requires you to do a for() loop across all possible block numbers. What if I have 50 blocks? On average this must for() through half the count, so if all 50 were displayed you would on average do the string build and comparison below it 25*25 times, when you could just use the value from $delta directly.

5. taxo_faceted_navigation.module 89-105: For repeated actions in hooks you should make calls like drupal_get_path just once and store it in a variable.

6. I can see some security and behavior issues with your hook_menu() implementation. First, if the user (not understanding what this is for) specifies something like 'user' or 'node' this could cause a lot of trouble. Second, of all the defaults in the world to choose I think 'articles' is the worst because it seems like a really common potential conflict and also not really where this module's "biggest" target audience is.

I am guessing you did not want to force the user into a specific initial prefix. That's your choice, but unless you want to change that decision you need to do sanity-checking here, at the very least looking for the most common Drupal special paths, or better still, querying menu_router for conflicting paths that don't match your page callback.

I couldn't find anything else. I think this module is a little rough in terms of user experience but the guidelines are a little slim on how to review those and opinions vary on "good UX". On the basis of code review, once the final items above are addressed (1, 4, 5, and 6) I feel this project can go to RTBC.

Oops, didn't see the PAReview security comment, adding back.

After following the all the steps in the README.txt file, I read I already have the taxonomy URL alias set up as recommended in Installation step 3.

When I click "food" taxonomy term, the browser replies with "Not Found
The requested URL /articles/subjects/food was not found on this server."

The blocks did not generate valid links, event though I regenerated (+3000 Bulk alias) the path alias match [term:name] token. They all fail as described in the previous paragraph.

The subjects is the vocabulary name, and "food" is only a flat term. I am only using flat vocabulary.

Perhaps is the view to emulate Drupal core's handling of taxonomy/term?
I am using this since it is a part of my navigation system and I really need the view.

Sounds promising though.
Good luck.

The issues in http://drupal.org/node/1359516#comment-5945876 have not yet been addressed. This issue is not ready to be reviewed.

Ok, this looks like RTBC for me.

manual review:

1. "$blocks[$delta]['info'] = 'Taxo Faceted Filter: ' . $taxonomy_name;": all user facing text should run through t() for tranlsation.
2. taxo_faceted_navigation_get_selected_filters(): why do you need check_url() here, you are not printing anything to the user?
3. taxo_faceted_navigation_get_nodes_based_on_intersect_of_terms(): this is vulnerable to SQL injection, always use placeholders in DB queries and never concatenate dynamic variables directly into it. See http://drupal.org/writing-secure-code
4. The module is still vulnerable to XSS exploits. If I have a term <script>alert('XSS');</script> and add a taxo facted search block I will get a nasty javascript popup. Terms are user provided input and need to be sanitized before printing. Please read http://drupal.org/node/28984 again.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished sooner.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

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

  FILE: ...all/modules/pareview_temp/test_candidate/taxo_faceted_navigation.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   545 | ERROR | Language constructs must be followed by a single space; expected
       |       | 1 space but found 2
   886 | ERROR | Language constructs must be followed by a single space; expected
       |       | 1 space but found 2
  --------------------------------------------------------------------------------
  

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.

manual review:

1. "module_invoke_all('taxo_faceted_navigation_blocks')": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php
2. taxo_faceted_navigation_get_selected_filters(): why do you need check_url() here, you are not printing anything to the user?

But otherwise looks RTBC to me.

no objection in more than a week, so ...

Thanks for your contribution, Darko Kantic!

I updated your account to let you 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 get 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.