omllobet [omllobet]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

As per requirements, the motivation message should be expanded to contain more features of the proposed project. For themes, it should include also a screenshot of the theme, and (when possible) a link to a working demo site using the proposed theme; for modules, it should include also a comparison with the existing solutions.

• This review is only partial.
• The points reported in this review are not in order of 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. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

2. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how Drupal variables, global variables, constants, and functions defined from the module should be named.
3. The module doesn't implement hook_uninstall().

4.   if (strcmp($fieldname, check_plain($fieldname)) != 0) {
       form_set_error('paint_imagefieldname', t('The name of the field is not safe to save.'));
     }
   
   

   I am not sure I understand what the purpose of strcmp($fieldname, check_plain($fieldname)) is.

5.   $form['serverid'] = array(
       '#type' => 'hidden',
       '#required' => TRUE,
       );
   
   

   #required is not used for hidden form fields.

6. function paint_flickr_download_form() {
     $form['fid'] = array(
       '#type' => 'hidden',
       '#required' => TRUE,
       );
     $form['serverid'] = array(
       '#type' => 'hidden',
       '#required' => TRUE,
       );
     $form['id'] = array(
       '#type' => 'hidden',
       '#required' => TRUE,
       );
     $form['secret'] = array(
       '#type' => 'hidden',
       '#required' => TRUE,
       );
     $form['title'] = array(
       '#type' => 'hidden',
       '#required' => FALSE,
       );
     $form['#submit'] = array(
       '#type' => 'submit'
     );
     return $form;
   }
   
   

   What is the purpose of a form that has only hidden fields?

7.   if (!user_is_logged_in()) {
         return drupal_access_denied();
     }
   
   

   That code should be part of the access callback.

8.   if (!module_exists('flickrapi')) {
       return t('Flickr integration needs Flickr API module installed and enabled.');
     }
   
   

   Why isn't the code calling drupal_set_message()?

9.     if (fwrite($handle, $result->data) === FALSE) {
         drupal_set_message(t("We couldn't save the image."));
         exit();
       }
   
   

     if (!$comma) {
       exit(t('Error: data incorrectly formatted'));
     }
   

       else {
         drupal_set_message(t("You don't have permissions for creating an image"), 'error');
         exit('Error: incorrect permissions');
       }
     }
     exit('OK');
   
   

   The code should not call exit() in such cases.

10.   if (!empty($nid)) {
        $node = node_load($nid);
        if ($node != FALSE) { // update node
          // if it is the admin allow edit if the node is not anonymous, if its not, create a new node this way
          if ((user_access('edit own ' . $node_type . ' content') && $uid == $node->uid ) || user_access('edit any ' . $node_type . ' content'))  {
            $update = TRUE;
            $file = _register_file($filename, $imginfo[0], $imgdest, $uid);
            $node->title = check_plain($title);
            $node->field_image = _fill_field_image($file, $title);
            node_submit($node);
            node_save($node);
          }
          else {
            drupal_set_message(t("You don't have permissions for editing an image"), 'error');
            exit('Error: incorrect permissions');
          }
        }
      }
    
    

    The code should also check the node is published.

11.       $text = t('Import an image from flickr...');
          $ret = '<a href=' . url('paint/flickr') . '>' . $text . '</a><br />';
    
    

    l() is the right function to call in this case.

Hello omllobet,

Huge apologies that your module and CVS application has remained un-reviewed for so long. I can see you have made changes or addressed everything in kiamlaluno's very thorough partial review.

Another note:

1. Code comments should end in a period, including the first line of documentation blocks that introduce a function. Inline comments also should be Sentence case and end with a period.

Everything else looks good and i recommend your application for approval, you can correct comments and anything else that comes up in code committed to drupal.org proper!

Thanks for this module!

@Daniele, thanks for the application and patience. There are still a couple thing I would like to clear up before approval. I am sorry for the delay but I feel they are pretty important.

The following points are just a start and do not necessarily encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

Neccessary for approval:

1. What is the deal with the loading image? Is this your own image? If not, is it available under GPL? Why not use the one that is core?

2. function paint_uninstall() {
     variable_del('paint_typename');
     variable_del('paint_imagefield_name');
     variable_del('paint_paintweb_config');
   }
   

   This function needs to be in a .install file.

3. function paint_admin__form_validate($form, &$form_state) {
   

   This function is not named correctly and is probably not fired. (I have not actually tried to run the module so do not know). Should not have the double underscore.

4. function paint_form_alter(&$form, $form_state, $form_id) {
     if ($form_id == 'paint_flickr_download_form') {
       $form['#action'] = url('paint/flickr/download');
     }
   

   Why are you form altering your own form? This seems unnecessary.

5.   $ret .= drupal_get_form('paint_main_form', $title) . '<span class="paint-status">' . t('Loading...') . l(t('Refresh'), 'paint/' . check_plain($node_id)) . '</span><img id="PaintEditableImage" src="'. check_url($img_src) .'" alt="test"><div id="PaintWebTarget"></div>';
   

   HTML output should be in a theme function. You could also just consider putting these parts into the form definition itself with the appropriate attributes and form types.

Suggested for a better module (IMO), but not necessary for application approval:

1. Allow the path to the Google Paint library to be configurable. Consider utilizing the Libraries API module: http://drupal.org/project/libraries
2. Javascript has Drupal Coding Standards as well, please follow these.

3. /**
   * Implements hook_uninstall().
   */
   

   Should be the following (note the spaces):

   /**
    * Implements hook_uninstall().
    */
   

4. I am not sure why you have such complicated permissions going on. Both the node module and CCK deal a lot with permissions and access to this sort of thing. Consider making this simpler and using the existing systems.
5. Consider utilizing CCK and filefield better to where this would just be a widget for this field. This would mean that someone could simply just add a PaintWeb field to a node type.

6.       '#attributes' => array('onclick' =>
           'if (confirm(Drupal.t("Do you want to leave and edit the image?"))) {
               window.location.replace("' . url('paint/', $options) . arg(1) . '");
               return false;
           }'
   

   Inline JS is not cool for many reasons. Please consider moving this to your JS file and utilizing jQuery.

7.     return t("We couldn't found any photos that match your criteria");
   

   Not proper American English.

8. Consider using the "file" parameter in hook_menu() to keep the .module file size down since it is included on every Drupal page load.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article, or read the handbook page on how to review for reference.

Hi @omllobet, sorry for the name mix up above, the message was indeed intended for you.

Also, just wanted to say that you are very close and doing a great job!

Hi @omllobet, thanks for the application and patience. Congratulations, you have been approved. The following points are more recommendations for when you do put your code on drupal.org and develop it further; these are not requirements, simply suggested improvements based on my experience and knowledge of the existing Drupal community. Also, please read further about CVS access and resources.

1. .loading {
     background: url('/misc/progress.gif') no-repeat center center;
   }
   

   This CSS assumes that Drupal is in the webroot which is not a good assumption. Unfortunately there is not a really good way to do this considering how one can't use variables in CSS. I would simply copy that image to your module, as we know its GPL.

2. Concerning suggestion 5 above. Look into how to implement a CCK field. Basically a CCK field is two parts: 1) the Field which defines how its stored, such as a file in filefield. And 2) the Widget defines the interface for input (this is what imagefield is). So, you're module should simply be a widget for filefield. This would make your module a lot cleaner and leverage a lot more existing CCK infrastructure. This a good, albeit a little old, post on how to create a CCK field: http://www.lullabot.com/articles/creating-custom-cck-fields Also, looking at the imagefield module would be helpful, and there is a lot of documentation for filefield here: http://drupal.org/handbook/modules/filefield
3. On the WYSIWYG integration, that should be fine and should not conflict with the point above. A more sustainable and robust way to build this project would to abstract out any wrappers you create around the PaintWeb API into a core paint.module, then have supplemental module for paint_cck.module and paint_wysiwyg.module (maybe even paint_flikr.module. This create a lot more file, but allows people to only use the functionality they need, reducing their code base and allowing Drupal to run more efficiently.

Please read the following resources to make sure you know how to use CVS and the specifics to the Drupal CVS infrastructure, as well as how to be a good module maintainer on Drupal.org. The Drupal community is very large and dynamic; we welcome you as a module maintainer and hope that you embrace and challenge the Drupal community and continue to contribute.

• CVS reference: http://drupal.org/handbook/cvs/quickstart
• Maintaining modules: http://drupal.org/node/7765

Thanks to the following people who helped review this application, it is very appreciated:

• @kiamlaluno
• @Benjamin Melançon

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article, or read the handbook page on how to review for reference.