[D7] Taxonomy Extends

Hi xylustc,

Ran the code through the online coder check and there are some formatting issues, please see http://ventral.org/pareview/httpgitdrupalorgsandboxxylustc1881890git

Once you have sorted the issues out I will review again.

Please move your code to version branch and delete master branch - http://drupal.org/empty-git-master.
There should also be hook_uninstall implement where you should delete variable(s) your module creates.

change display

Automatic Review

Found 1 error using PAReview at http://ventral.org/pareview/httpgitdrupalorgsandboxxylustc1881890git

Manual Review

• You create an alter called "te_node_order_right" on Line 838 of the .module file. Would this be better to name it the same as your module e.g taxonomy_extends_node_order_right() ?

Other than the above, I didn't find any major issues.

Sorry, still get an error on http://ventral.org/pareview/httpgitdrupalorgsandboxxylustc1881890git

Hi,

All good for errors on ventral.org and everything else looks fine to me now.

Cheers.

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 :-)

Hi, xylustc.

Manual review:

1 You have multiple places where static text not wrapped in t() function.
2 README.txt have not wright structure. See example here http://drupal.org/node/1271064.
3 Looks like path 'admin/taxonomy_extends' is main admin panel of this module, but in not appear in Management menu, you must specify it on your hook_menu. Also you not specified any permission and it not clear who can manage this settings.

Please fix all this issues.

Hi,

Good module, thanks for contribution !
Here is my review of it :

- in taxonomy_extends_tokens you are foreaching all tokens to get only the 'hierarchypath' one ! I think its better to access the information you need simply by accessing the 'hierarchypath' key (if(isset($tokens['hierarchypath'])) {//do stuff}). this will avoid unecessary $tokens looping.

- $replacements[$original] = $sanitize ? ($cascadename) : ($cascadename);
i didn't understand why we are testing $sanitize here and having the same value anyway.
$replacements[$original] = $cascadename will be clear i guess

- it's a commun Drupal best practice to use the l() function to generate links instead of all those hard coded '<a href='*, please try to use l().

- For performance concerns, it's a good practice in Drupal to move all the form declaration and handler to a separate file. so that the .module remains clean with minimal needed code.
i suggest to move like this :
Whenever you have a menu item that has a 'page callback' => 'drupal_get_form', you can add a 'file' => 'taxonomy_extends.forms.inc' and move the formbuilder and handlers from the .module to the .forms.inc

- You can use theme('table', $variables) instead of the hard coded '<table><tr><th>'*

- are all those '@' really necessary ?

 $nid = @$values["nid"];
 $tid = @$values["tid"];
 $sticky_pos = @$values["sticky_pos"];
 $node_pos = @$values["node_pos"];
 $sticky_pos_old = @$values['sticky_pos_old'];
 $node_pos_old = @$values['node_pos_old'];

- For performance needs an dbest practices, i suggest to add only a hook_views_api in your .module and move the hook_views_data to a taxonomy_extends.views.inc for example

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.

The name of your module is not very descriptive, and it depends on tokens, how about something like "Taxonomy Token Extension" or "More Taxonomy Tokens" instead?

In your README, name should be a human-readable string like "Taxonomy Extends". The same applies to the description, and you can remove the commented lines.

You don't need taxonomy_extends_install() if it's just to set a default value. And use single quotes unless you need variable substitution.

For admin/config/system/taxonomy_extends you have no access arguments - I think it will still inherit from the parent menu item, but you might want to be sure.

Just a partial review.

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

You have some misspellings - "voculaburyname" should be "vocabularyname". Your branch should be named "7.x-1.x" instead of "7.x-1.x-dev".

In taxonomy_extends_term_list() you have drupal_set_title($term->name); which is a security risk - please use check_plain() before outputting the tern name.

I'm not really sure what this module does, but the code looks ok. Setting to "needs work" for the security problem, otherwise this looks RTBC. Some more documentation or examples of what the module does would be muchly appreciated.

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

change to branch 7.x-1.x-dev