[D6] Project Estimator

welcome,

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow tips for a great project page. Also create a README.txt that follows the guidelines for in-project documentation.

Major things you should fix:

• The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
  

  ./includes/estimator.class.php
  

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

  function _display_project_statistic() {
  function project_statistic_admin_settings() {
  function file_default_extensions() {
  function file_extension_explode($delimeter, $extension) {
  function file_extension_format_right($value) {
  

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxankitchauhan1536580git

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

• Bad line endings were found, always use unix style terminator: This should be fixed by saving the file in unix format (File>Save as>format=unix)
• You can replace str_replace('\\', '/', $dir_name); with drupal_str_replace('\\', '/', $dir_name);
• trigger_error(filter_xss('Cannot find ' . $dir_name . ' directory.'), E_USER_ERROR); - why not using t() in these cases? -> trigger_error(t('Cannot find @directory directory.', array('@directory' => $dir_name)), E_USER_ERROR);

Rest of the errors are only minor and can be ignored, you don't have to satisfy the nitpicking automated review by 100%, just make sure major issues are cleared.

Just reading the code (i didnt install), here are a couple of small things.

Line 77: variables should be prefixed with your module name. So, something like "project_estimator_file_extension" instead of a very generic "file_extension".

     '#default_value' => variable_get('file_extension', project_estimator_file_default_extensions()),

Also, you use "administer site configuration" for your access checks in project_estimator_menu(). A better practice would be to implement hook_perm(), and define your own permissions.

Thank you for your reviews. When finishing your review comments also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

manual revew:

1. what is the use case of this module? Please add that toy our project page, see also http://drupal.org/node/997024
2. 'description' => 'Configure your player settings for each video extension.',: eh, what?
3. "require_once 'includes/project_estimator.php';": be careful with such relative file inclusions, better use something like require_once dirname(__FILE__) . '/mymodule.inc';.
4. project_estimator_display_project_statistic(): doc block "@return string a string" does not tell me much. Be more descriptive like "Themed HTML output of bla blu".
5. "variable_get('file_extension', project_estimator_file_default_extensions()),": all variables should be prefixed with your module's name to avoid name collisions.
6. "define('MAX_DEPTH', 100);": all constants must be prefixed with your module's name to avoid name clashes.
7. ProjectEstimator::getInfo(): duplicate of display-project-statistic.tpl.php?
8. "filter_xss('No root directory specified.')": no user provided data involved, so filter_xss() is useless here.
9. "print $fname['file_name'];": file names should be considered user provided input and therefore need to be sanitized before printing.

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

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1556812
Project 2: http://drupal.org/node/1539536

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.