subhrajit [subhrajit]

@subhrajit: You were supposed to change status after uploading the archive containing the proposed module.

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.

• 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. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
2. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted; how Drupal variables, global variables, constants and functions defined from the module should be named; which Unicode functions the code should use.
   Check also the line ending used for the files; the coding standards suggest you use the Unix line ending characters.
   Check you are not using tabs to indent the code.
3. Drupal variables are not initialized in hook_install().

4. 	$nfa_association_finder[0]['description'] = "Files uploaded using upload module";
   	$nfa_association_finder[0]['type'] = "filepath";
   	$nfa_association_finder[0]['querystring'] = "SELECT f.filepath AS filepath FROM {files} f, {upload} u WHERE f.fid = u.fid AND u.nid=\$nid";
   
   

   The description is non translated.

5. 	include_once("node_file_access.lib.inc");
   
   

   include_once() is trying to load the file from the current directory, which is the Drupal root directory, and not the directory where the file is located.
   For such cases, there is a Drupal function that should be used.

6. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

7. Permissions follow the schema <verb> <object>, where the verb is written in lowercase.
8. Strings used in the user interface should be translated.
9. In a settings form, the default value should be taken from Drupal variables, or from a database table; in this way, users would retrieve the value they used the last time.

10. 		'#description' => t( "separate multiple file paths by newlines"),
    
    

    The description of a form field starts with a word in capital case, and ends with a period.

11. Why isn't the code using system_settings_form()?

12. 				drupal_set_message(t('Node File Access Security Alert: The permission to folder '.$this_folder['path'].' is not set properly.'), 'warning');
    
    

    The first argument of t() is a literal string, not a concatenation of strings. The script used to create the translation template is not able to handle any dynamic value, even in the case of code similar to t($variable); this means that if the argument of the function is not a literal string, it will not appear in the translation template.
    Use t()-placeholders.

13. 	$r = db_result(db_query("SELECT COUNT(*) FROM {nfa_association} WHERE nid=%d AND BINARY file='%s'", $nid, $fileQuery));
    
    

    I think BINARY is not used from all the database engines.

14. 					$filepathList = eval($this_ass_finder['querystring']);
    					if (is_array($filepathList))
    						foreach ($filepathList as $thisfilepath)
    						{
    							$thisfilepath = _nfa_correct_path($thisfilepath);
    							$allFoundFiles[] = $thisfilepath;
    							_nfa_set_association($thisfilepath, $nid);
    						}
    
    

    If the module is using eval() with users input, then it should also expose a permission like use PHP for <motivation>.

15. 	$fpath = _nfa_correct_path($fpath);
    	$handle = opendir($fpath);
    	if ($handle)
    		while (FALSE !== ($thisfile = readdir($handle)))
    		{
    			if ($thisfile=="." || $thisfile=="..")
    				continue;
    			$thisfile = $fpath.$thisfile;
    			if (is_file($thisfile))
    				$fileList[] = $thisfile;
    			elseif (is_dir($thisfile))
    				$fileList = array_merge($fileList, _nfa_get_files_from_folder_recursive($thisfile));
    		}
    
    

    Why isn't the code using file_scan_directory()?

16. 	$form['progress_iframe'] = array(
    				'#value' => "<div id='progress_div' style='border: 1px solid #888888; margin:5px; padding: 5px; font-weight: bold; font-size: 12pt;display:none;'>Building associations. Please wait.
    							<iframe id='progress_iframe' src='http://subhasree.net/drupal/index.php?q=get_progress&onlyval=0' style='width:350px; height:20px; margin-left:10px; overflow:hidden;' marginheight='0' marginwidth='0' frameborder='0' scrolling='no'>
    								<div style='border: 1px solid #888888; background-color: #aaaaaa; margin:5px; padding: 0px; width:350px; height:20px; position:relative;'><div id='nfa_progress_bar_progress' style='border: 0px; background-color: #44ee22; margin:0px; padding: 0px; width:3px; height:20px; position:absolute; left:0px; top:0px;'></div></div>
    							</iframe></div>"
    			);
    
    

    What is the purpose of the iframe?

17. 		header('Content-type: text/plain');
    		print("".$theProgress);
    
    

    The function to use is drupal_set_header().
    print transform already its argument in a string; there is no need to use "" . $theProgress.

18. The function _get_nfa_progress() is outputting too much HTML; you should use a theme function.
    There is a Drupal function to add JavaScript code to a page.

This is a partial review.

1. Fixed almost all issues. I have used "nfa", which is shorter and easier for readability, as the prefix for function names, instead of the longer "node_file_access".

   In that case, you should rename the module to nfa.module (and rename the other files too); if the module name is example.module, then example_ must be used as prefix for all the functions, constants, global variables, and Drupal variables it defines. To notice that constant names are written using all uppercase characters.

2. I think I have initialized all the variables using "variable_set" function in hook_install. Is there something else that you are referring to?

   I meant that a module doesn't initialize its Drupal variables in hook_install(); if you look at Drupal core code, there is no module that does that.

3. The code format still doesn't follow what reported in Drupal coding standards.

4.         if (!$res)
             drupal_set_message(t('WARNING: Could not change permission of folder ' . $this_folder['path'] . '.'), 'error');
         }
   
   

   Use t()-placeholders.
   There is no need to prefix the string with 'WARNING'; the type of message is a parameter of drupal_set_message().

5.       print("<br/>".$path." -  ".$perm."<br/>");
   
   

   Remove any debugging code.

6.       { drupal_access_denied(); exit(); }
   
   

   Before exit() the code should call module_invoke_all('exit').

7. function _nfa_force_update_associations_all($forceUpdate = TRUE) {
   
   

   In the coding standards there is also a part about the names to use for PHP variables.

8.   while ($thisrow = db_fetch_array($result)) {
       _nfa_update_associations(intval($thisrow['nid']), $forceUpdate);
       $count++;
       variable_set("nfa_progress", intval(100*$count/$totalNIDcount));
       //sleep(1);
     }
     for ($nnid=-1; $nnid<=0; $nnid++) {
       _nfa_update_associations($nnid, $forceUpdate);
       $count++;
       variable_set("nfa_progress", intval(100*$count/$totalNIDcount));
       //sleep(1);
     }
   
   

   Is there a reason to use a Drupal variable instead of a PHP variable?

9.           $querystr = str_replace("\$nid", "" . $nid, $this_ass_finder['querystring']);
             $result = db_query($querystr);
   
   

   db_query() supports placeholders.

10.     if (variable_get("nfa_auto_remove_unlocked_associations", TRUE))
          $ass_to_remove = array('-1','0');
    
    

    That is a funny name to give to a PHP variable. :-)

11.     drupal_set_message(t('The path ' . $orgFilepath . ' is not a valid file.'), 'warning');
        return(FALSE);
    
    

    The first argument of t() is a literal string, not a concatenation of strings. The script used to create the translation template is not able to handle any dynamic value, even in the case of code similar to t($variable); this means that if the argument of the function is not a literal string, it will not appear in the translation template.
    return is not a function; the parentheses are not necessary.

12.         drupal_set_message(t('Incorrect syntax in a Pattern Rule for folder ' . $this_folder . '.'), 'error');
            continue;
    
    

    Sentences used in messages are written in sentence case. (This is a sentence written in sentence case.)

13. function _nfa_add_js_progressbar (&$form) {
      
      $form['progress_iframe'] = array(
            '#value' => "<div id='progress_div' style='border: 1px solid #888888; margin:5px; padding: 5px; font-weight: bold; font-size: 12pt;display:none;'>Building associations. Please wait.
                  <iframe id='progress_iframe' src='" . url('get_progress', array('query' => 'onlyval=0')) . "' style='width:350px; height:20px; margin-left:10px; overflow:hidden;' marginheight='0' marginwidth='0' frameborder='0' scrolling='no'>
                    <div style='border: 1px solid #888888; background-color: #aaaaaa; margin:5px; padding: 0px; width:350px; height:20px; position:relative;'><div id='nfa_progress_bar_progress' style='border: 0px; background-color: #44ee22; margin:0px; padding: 0px; width:3px; height:20px; position:absolute; left:0px; top:0px;'></div></div>
                  </iframe></div>"
          );
    
      $ProgressBarJS = "
                function SubmitAndShowProgress(theElem) {
                  
                  var branchLevel = 0;
                  var myElems = new Array();
                  myElems[0] = theElem;
                  while (myElems[branchLevel].tagName.toUpperCase() != 'FORM') {
                    branchLevel++;
                    myElems[branchLevel] = myElems[branchLevel-1].parentNode;
                  }
                  theform = myElems[branchLevel];
                  
                  document.getElementById('progress_div').style.display = 'block';
                  var inpElms = theform.getElementsByTagName('input');
                  for (i=0; i<inpElms.length; i++)
                    if (inpElms[i].getAttribute('type').toUpperCase() == 'SUBMIT')
                      inpElms[i].style.display = 'none';
                  
                  // AJAX progress display
                  setTimeout('recursiveUpdateWidth()', 0);
                  
                  // Submit the form
                  if (branchLevel==0) {
                    theform.submit();
                    return false;
                  }
                  else
                    return true;
                }
                
                
                function recursiveUpdateWidth()  // This is used only if the iframe fails
                {
                  if (window.XMLHttpRequest)
                    var oRequest = new XMLHttpRequest();
                  else
                    var oRequest = new ActiveXObject('Microsoft.XMLHTTP');
    
                  oRequest.open('GET','index.php?q=get_progress&onlyval=1',true);
                  
                  oRequest.onreadystatechange = function() { 
                    if(oRequest.readyState == 4) {
                      var progress = parseInt(oRequest.responseText);
                      document.getElementById('nfa_progress_bar_progress').style.width = parseInt(progress*3.5) + 'px';
                      if (isNaN(progress) || progress < 100)
                        setTimeout('recursiveUpdateWidth()', 250);
                    }
                  };
                  oRequest.send(null);
                } ";
                
      drupal_add_js($ProgressBarJS, 'inline');
    
    }
    
    

    The function is outputting HTML without using a theme function.
    The JavaScript code (which is not using jQuery) should be placed in an external file.

14. Menu descriptions and titles, as well as schema descriptions, are not passed to t().
15. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted; how constant names are written; how Drupal variables, global variables, constants, and functions defined from the module should be named.
    Check also the line ending character used in the files; the module file is not using the correct one.

I am closing this issue due to lack of replies.

Please read the following links as this is very important information about CVS applications.

• Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for these applications.
• Please read Migrating from CVS Applications to (Git) Full Project Applications and Applying for permission to opt into security advisory coverage on how this affects and benefits you and the application process. In short, every user has now the permissions necessary to create new projects, but they need to apply for opt into security advisory coverage. Without applying, the projects will have a warning on projects that says:
  

  This project is not covered by Drupal’s security advisory policy.