[D7] Vocabulary image

You only need one project reviewed to be granted full project access (and it cuts down on the Project Applications queue). So please pick one you'd like reviewed, then close the other(s).

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:

• ./vocabulary_image.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function vocab_validate_image() {
  

• 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. You have to get a review bonus to get a review from me.

Manual Review:
1) vocab_image_settings_form() please make sure that each user facing string should run through t(). Example: '#description' => 'Separate extensions with a space or comma and do not include the leading dot.', should be like '#description' => t('Separate extensions with a space or comma and do not include the leading dot.').
2) Remove extra comments from vocab_image_settings_form(). Example: //'#title' => t('Maximum image resolution').
3) vocab_image_settings_page_validate() funtion should use t(). Example: form_set_error('vocab_max_image_resolution_width', t('Enter valid width for "Maximum image resolution".')). Please do this for all the form_set_error() functions.

1. You can use db_add_field to add only column to database, instead of new table.

   db_add_field('taxonomy_vocabulary', 'fid', array(
     'type' => 'int',
     'not null' => TRUE,
     'default' => 0,
     'unsigned' => TRUE,
     'description' => 'fid if this particular source is attached to a vocabulary.',
   ));

   And db_drop_field to uninstall. See Schema API functions.

2. Please check the strings on the transferability. And don't use HTML tags inside t(), place '<br/>' outside it.
   vocabulary_image.admin.inc 24 - '#description' without t().
3. Try to not use queries like db_query("SELECT fid FROM {taxonomy_vocabulary_image} WHERE vid = " . $vid . "")->fetchField();. Use
   

   db_select('taxonomy_vocabulary_image', 't')
     ->fields('t', array('fid'))
     ->condition('vid ', $vid)
     ->execute()
     ->fetchField();

Hi brajendrasingh,
thank you for your module.

1) Manual code review. vocabulary_image.info: "Other" is bad package name, may be it's better to rename package to "Taxonomy" or something similar?
2) Manual code review. vocabulary_image.admin.inc: lines 127-144 may be it's better to loop through a cycle on array('vocab_image_extension', 'vocab_max_image_resolution_width', ...) and execute variable_set() instruction inside it?
3) Manual code review. vocabulary_image.module: I don't see any theme functions and template references. How vocabulary image will be appeared on the pages by default?

Thank you for hot fixes. But it still needs review.

Overall:
Very simple, to the point. Solves a very specific need, but does so adequately.

Automated Review:
No Issues, excellent =D

Manual Review:

• vocabulary_image.module
  • Line 26 and 29 - contain misspellings in the comments. Not a biggie, but might as well fix them.
  • Line 118 - $vocab_id does not need to be instantiated here, as it is instantiated inside the following if conditional and is not used outside of this conditional.
  • Line 141 - even if a default_vocab_image is returned from variable_get, $fid will be overwitten by its reinstantiation on line 142.
  • Line 163 - $imagepath is not guaranteed to be defined, as it is instantiated in an if conditional which is not guaranteed to be true.
• vocabulary_image.admin.inc
  • Line 139 - $fid is reinstantiated on line 141 and will override the value. $fid is not used elsewhere so setting $fid = 0 doesn't accomplish anything.

Manual Review:

• vocabulary_image.module
  • Line 118 - Remove commented-out variable.
  • Line 141 - Remove commented-out variable.
  • Line 163 - $imagepath should be defined inside the foreach loop or emptied after each iteration. Alternatively, this function can actually be simplified to the following:
    

    function vocabulary_image_taxonomy_vocabulary_load($vocabularies) {
      foreach ($vocabularies as $vocabulary) {
        if ($vocabulary->vid) {
          $vocabulary->imagepath = vocabulary_image_get_vocab_image_path($vocabulary->vid);
        }
      }
    }
    

  • Line 103 - vocabulary_image_custom_image_submit() contains if conditionals testing for the presense of certain submitted values. The test is done by looking at the effective boolean value of the variable, which if a value is not set, results in a PHP Notice.
    Notice: Undefined index: vocab_image in vocabulary_image_custom_image_submit() (line 104 of /www/vhosts/sandbox.localhost/sites/all/modules/testing/vocabulary_image/vocabulary_image.module).
    See line 104:

    if ($form_state['values']['vocab_image']) {
    

    Should be:

    if (isset($form_state['values']['vocab_image'])) {
    

    You can optionally check its value if it is set, as isset() will not test for whether the value is an empty string. This function can be simplified further, eliminating the two if conditionals checking the presense of the same variable:

    function vocabulary_image_custom_image_submit(&$form, &$form_state) {
      $fid = 0;
      if (isset($form_state['values']['vocab_image'])) {
        // Load the file via file.fid.
        $file = file_load($form_state['values']['vocab_image']);
        // Change status to permanent.
        $file->status = FILE_STATUS_PERMANENT;
        // Save.
        file_save($file);
        file_usage_add($file, 'custom_vocab_alter', 'custom_vocab_alter', $file->fid);
        $fid = $form_state['values']['vocab_image'];
      }
    
      // $vocab_id = '';
      if (isset($form_state['values']['machine_name']) && ($fid)) {
        $vocab_id = db_query("SELECT vid FROM {taxonomy_vocabulary} WHERE machine_name = '" . $form_state['values']['machine_name'] . "'")->fetchField();
        if ($vocab_id) {
          db_delete('taxonomy_vocabulary_image')
          ->condition('vid', $vocab_id, '=')
          ->execute();
    
          db_insert('taxonomy_vocabulary_image')
          ->fields(array(
            'vid' => $vocab_id,
            'fid' => $fid,
          ))
          ->execute();
        }
      }
    }
    

• vocabulary_image.admin.inc
  • Line 139 - Remove commented-out variable.

Manual Review:

1) Remove unnecessary comments from the vocabulary_image.admin.inc file.
example: // '#upload_validators' => vocabulary_image_custom_validate_image(),
on line no 84.
2) Use placeholders instead of concating variables in query(http://drupal.org/writing-secure-code).
example: $vocab_id = db_query("SELECT vid FROM {taxonomy_vocabulary} WHERE machine_name = '" . $form_state['values']['machine_name'] . "'")->fetchField();

Forgot to change the status.

Manual Review:

1. vocabulary_image_form_alter(): This function is vulnerable to XSS. You need to sanitize strings or urls before printing.
example: $image->uri should be like check_url($image->uri).
2. vocabulary_image_custom_validate_image(): This function is also vulnerable to XSS. Please use proper santization function to santize the strings. see http://api.drupal.org/api/drupal/includes!common.inc/group/sanitization/7

There are two articles which are related to writing secure code and you must read these articles
i) http://drupal.org/node/28984
ii) http://api.drupal.org/api/drupal/includes!common.inc/group/sanitization/7

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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/vocabulary_image.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   140 | ERROR | Incorrect spacing between argument "$vid" and equals sign;
       |       | expected 1 but found 0
   140 | ERROR | Incorrect spacing between default value and equals sign for
       |       | argument "$vid"; expected 1 but found 0
  --------------------------------------------------------------------------------
  

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. "Implements of hook_menu()." should be "Implements hook_menu().".
2. "variable_get('vocab_max_image_resolution_width')": all variables provided by your module need to be prefixed with your module's name to avoid name collisions.
3. vocabulary_image_taxonomy_vocabulary_load(): why do you check the the vid? Of course all loaded vocabularies will have a vid, right?
4. You don't need the check_url() calls in your case, because the file URI has already been munged by the managed file system. So this is not user provided input and therefore the check_url() calls are not needed.

Otherwise I think this is nearly ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Automatic Review:
All good.

Manual Review:
You have a spelling mistake "addtional" in your comment, vocabulary_image:line 26
And again "incase", vocabulary_image:line 29
Instead of your "hook_form_alter" implementation have you consider using: hook_form_FORM_ID_alter

General comment:
I do not see the value in this module, it can be implemented by a site builder: adding an Image field to the taxonomy.
Adding the field to the taxonomy via the existing functionality provides more features and keep the image upload inline with other image fields for UI, implementation and most importantly security updates.

Automatic Review

Sorry, found some ventral.org issues at http://ventral.org/pareview/httpgitdrupalorgsandboxbrajendrasingh1386558git

Manual Review

Is there a reason why you named the validate and submit functions for the admin settings form as vocabulary_image_settings_page_validate() and vocabulary_image_settings_page_submit()? If you were to name them the same as your settings form e.g vocabulary_image_settings_validate() and vocabulary_image_settings_submit() then you would not need to add $form['#validate'][] = 'vocabulary_image_settings_page_validate'; and $form['#submit'][] = 'vocabulary_image_settings_page_submit'; to the form. They will get called automatically.

Apart from these observations, I can't find anything other issues. Nice work!

Hi brajendrasingh,

No issues while doing automated review in ventral.org
-------
Found some major issues while doing manual review.

• I just visited admin/structure/taxonomy/tags/edit page after I enabled your module.
• I submitted without uploading image and got the below error.

Strict warning: Creating default object from empty value in vocabulary_image_custom_image_submit() (line 106 of C:\xampp\htdocs\projects\drupal719\sites\all\modules\contrib\1386558-db8b80a\vocabulary_image.module).
Notice: Undefined property: stdClass::$uri in file_save() (line 570 of C:\xampp\htdocs\projects\drupal719\includes\file.inc).

------

• Added image for particular vocabulary and saved. I can see preview of image below 'Upload image' field.
• Clicked on remove button then preview will remains.
• Then click on save I get the same error as above.

------

• Again I visited the same edit page of vocabulary. I still see uploaded image.
• Just submitted again without doing anything. Got massive error (Attached error file).

-------
And other minor issues found in manual review.
-----------
File : vocabulary_image.admin.inc
Give the single line after every $form in function vocabulary_image_settings_form or just follow the same indentation for all forms.

Line 101: Expected single line.
Line 106: Use t() for error description.
Line 109: Use t() for error description.
Line 112: Use t() for error description.
Line 116: Use t() for error description.
Line 119: Use t() for error description.
Line 122: Expected single line.
--------
File: vocabulary_image.install

Line 34: Expected single line.
--------
File: vocabulary_image.module

Line 22: Expected single line.
Line 56: Expected single line.
Line 98: Expected single line.
Line 135: Expected single line.
Line 154: Expected single line.
------
Final conclusion: I hope you need to spend some more time to fix these issues.

Thanks and Regards,
D. Veera Prasad.
www.drup-all.com

Consider the following:
- adding admin screenshots to the project page: http://drupal.org/sandbox/brajendrasingh/1386558
- justifying tabs and indents in vocabulary_image.module for arrays (for example, for the array on line 11)

Brajendra,

No issues were found from PAReview.

Manual Review:

1. Your Project Page could use some work. You've got some of the good basics (features, an example case & overview) however some additional sections like Requirements, Similar Projects, and Documentation links would go a long way. A great read on this is at: http://drupal.org/node/997024

2. A Documentation Page would help a lot. This helps new users know how to use the module and can include any other useful information you might have for someone trying Vocabulary image. See #5 on this link: http://drupal.org/node/7765

3. There is one security concern: your files have group write permissions. If you develop on linux you can fix this by using the command: "chmod 644 /path/to/file" on all files.

4. Coding standards:
vocabulary_image.admin.inc - line 24 - the description should be wrapped with t();
vocabulary_image.module - line 83 - I don't see a use for this line. The documentation for file_validate_size does not accept the user id as an argument, and also calls it's own instance of global $user.

5. Module functionality:
As far as I can tell the size and resolution restrictions do not work. I tried setting the minimum and maximum both to 10x10 and uploaded a much larger image successfully. The file extension restriction did appear to work properly.

Otherwise the module seemed to work properly.

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

manual review:

1. vocabulary_image_uninstall(): doc block is wrong.
2. vocabulary_image_form_taxonomy_form_vocabulary_alter(): doc block is wrong, this is hook_form_FORM_ID_alter()?
3. I think the README.txt should contain usage instructions? Where does the image appear? Do I have to (over-)write theme templates? Which ones?

But otherwise I think this is RTBC.

Assigning to mitchell as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, brajendrasingh!

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.