tutumlum [tutumlum]

Just on an initial quick review, the profile_role.module has this in it:

/**
 * @file
 * Define profile categories by role.
 *
 * Copyright 2008 by Jimmy Berry ("boombatower", http://drupal.org/user/214218)
 */

So this isn't even your module? Turns out it's an existing project http://drupal.org/project/profile_role so I don't know what you're hoping to accomplish by submitting an already existing project with your CVS application. The only thing I can think of is to make yourself look better and hope we don't notice?

You do not need a CVS account to help port a module to D7. All you need to do is post patches to the module's issue queue. Boombatower is quite active and isn't exactly abandoning his modules. I'm sure he would accept your porting patches. I am sorry I blamed you, but nothing in your application said "I wrote these modules, but I also helped port some existing D6 modules to D7."

No, I didn't submit it as my module, I convert it to D7 because it is required module.

Still, it doesn't make sense.
You are supposed to report here your code, not the code developed from somebody else that you converted to a newer version of Drupal. If your module depends on a module that has not been converted to the Drupal version used by your module, you simply report your code adding a note that it depends on a module that needs to be ported.

In school_administration.forms.php (the extension should be ".inc"):

1. There are many functions that are marked as implementations of hook_form() when they are not (the correct definition of such hook is hook_form($node, $form_state), and it's implemented by modules that handle nodes).
2. Also there is some code that is outside a function.

   $job_types = array(
   t('Organization'),
   t('Company'),
   t('Government'));
   $jobs = array(
   "--",
   t("Assistant Teacher"),
   t("Canteen"),
   t("Cashier"),
   t("Cleaner"),
   t("Club Teacher"),
   t("Cook"),
   t("Cook Assistant"),
   t("Counselor"),
   t("Director"),
   t("Driver"),
   t("Full Time Teacher"),
   t("Guard"),
   t("IT Designer"),
   t("Librarian"),
   t("MPA Guard"),
   t("Nurse"),
   t("Part Time Teacher"),
   t("Receptionist"),
   t("Secretary"),
   t("Security"),
   t("Supervisor"),
   t("Teacher"));
   $relations = array(
   "--",
   t("Aunt"),
   t("Brother"),
   t("Brother in Law"),
   t("Father"),
   t("Grandfather"),
   t("Grandmother"),
   t("Husband"),
   t("Mother"),
   t("Own"),
   t("Sister"),
   t("Sister in Law"),
   t("Step Brother"),
   t("Step Father"),
   t("Step Mother"),
   t("Step Sister"),
   t("Uncle"),
   t("Wife"));
   $withdraw_reasons = array(
   "--",
   t("Academic difficulty"),
   t("Change in career plans"),
   t("Death"),
   t("Discipline problems"),
   t("Dissatisfied with school"),
   t("Employment"),
   t("Family problems"),
   t("Financial problems"),
   t("Goals undefined"),
   t("Graduation"),
   t("Health problems"),
   t("Housing problems"),
   t("Marriage"),
   t("Moving"),
   t("No reason specified"),
   t("Personal problems"),t("Temporary travel"),
   t("Transfer to another school"),
   t("Transportation Problems"),
   t("With family wish"));
   $gender = array(
   1 => t('Male'),
   2 => t('Female'));
   

In school_administration.install:

1. There are some functions that are not prefixed by the name of the module; in the specific case, all the function names must start with school_administration_.
2. The indentation used is not two spaces as suggested by the Drupal coding standards.
3. It seems there is code that has been copied from another module, and pasted in the proposed module (see the comment that says "insert filed roles (profile_role module)"); it has not been even modified to change the function names, and the proposed module is likely to create a conflict with the module from which it took the code.
4. insert_field_roles() contains the following code (which should not even exist):

     echo 'field roles are recorded<br/>';
   

If you call functions that are present in other modules, then the name of the function is as defined in that module. If with "using those functions in other modules" you mean that you copy the file containing the function definitions in your module, then that is not allowed.
A module uses the functions given from third-party modules, but it doesn't incorporate them inside itself. If you need some functions present in a module, then your module must declare its dependency from the other module.

Waiting for new upload of corrected code.

What I was referring is this code (school_admin.install):

/** 
 * Implementation of hook_schema()
 */
function school_administration_schema() {
  $cyear = variable_get("c_year", '');
  foreach(array($cyear . '_', ($cyear + 1) . '_') AS $tbl_pf) {
    $schema['2009_student_parents'] = array(
        'description' => 'Stores parents and elder realtives of students.',
        'fields' => array(
             'id' => array('type' => 'serial', 'not null' => TRUE, 'disp-width' => '11'),
             'school_no' => array('type' => 'int', 'not null' => TRUE, 'disp-width' => '11'),
             'relation' => array('type' => 'varchar', 'length' => '32', 'not null' => TRUE),
             'uid' => array('type' => 'int', 'not null' => TRUE, 'disp-width' => '11'),
             'guardian' => array('type' => 'int', 'size' => 'tiny', 'not null' => TRUE, 'default' => 0, 'disp-width' => '1')),
        'primary key' => array('id'),
    );
  }

The indentation passes from 4 characters to 8, and from 8 to 13; it is supposed to pass from 4 to 6, and from 6 to 8 characters.

@KiamLaLuno, if you are wanting the applicant to make changes please set the status to "needs work". Thanks :)

1. In your module's .info files, do not specify the version line. That is supposed to be added by the drupal.org packaging script. See http://drupal.org/node/152819 and http://drupal.org/node/231036.
2. Arrays need to have commas after their last element. See http://drupal.org/coding-standards#array.
   Your code:

     $gender = array(
       1 => t('Male'),
       2 => t('Female')
     );
   

   Should be:

     $gender = array(
       1 => t('Male'),
       2 => t('Female'), <--- added comma is here
     );
   

3. It appears that you are creating/using several variables that could potentially cause conflicts with other modules since your variables are not prefixed with your module's name.
   

   function school_administration_uninstall() {
     drupal_uninstall_schema('school_administration');
     variable_del('c_year');
     variable_del('std_id_len');
     variable_del('num_range');
     variable_del('register_to');
     variable_del('password_inc');
     variable_del('st_id_conf');
     variable_del('school_admin_mod_relations');
     variable_del('school_admin_mod_jobs');
     variable_del('school_admin_mod_withd');
     variable_del('s_year');
   }
   

   If I were to create a password.module and use my own password_inc variable and someone uninstalls your module, it will unexpectedly mess up with an unrelated module. Likewise, your table names should also be prefixed with your module's name as to avoid collision. For instance, you have a table named 'withdrawal'. [Warning: silly example ahead] What if someone wanted to create an ATM module that allowed people to withdraw things? We always need to be sure that we take care with our module's functions, variables, and table names to use the module's namespace and avoid these potential conflicts.

4. In school_administration_add_roles_and_permissions() you are creating roles based on the assumption that the next rid starts with 3. What if another module has been installed before yours and the next available rid is 10? This is going to fail. You should run db_insert() on the role table, which should return the rid that was inserted. Then use that value to insert into the role_permission table.
   

     //add roles and role permissions
     $rid = 3;
     if (db_query("SELECT COUNT(*) FROM {role}")->fetchField() <= $rid) {
       $query1 = db_insert('role')->fields(array('rid', 'name'));
       $query2 = db_insert('role_permission')->fields(array('rid', 'permission'));
       foreach ($role_permissions AS $role => $permissions) {
         $rid++;
         $query1->values(array($rid, $role));
         foreach ($permissions AS $perm) {
           $query2->values(array($rid, $perm));
         }
       }
       $query1->execute();
       $query2->execute();
       drupal_set_message('new roles are recorded');
     }
   

5. The array definitions that are longer than 80 characters, specifically in school_administration_schema(), should use the format detailed in the coding standards. See http://drupal.org/coding-standards#array.

That was without going through the .module file. While it's clear you have a starting grasp of how to write modules, it's clear to me that there is still a lot of work to be done.

school_administration.module:

1. If you don't you the second argument of t(), it's perfectly useless you pass it.

       $replace = array();
       return '<p>' . t($txt, $replace) . '</p>';
   

2. The indentation is still wrong; this happens in more than one part.
3. school_administration_access() should always return a value, not just when it is TRUE.

4.   $items['school_administration/browse/%/%/%'] = array(
       'title' => 'Browse Persons',
       'description' => 'Add Portrait Photo',
       'page callback' => 'school_administration_identity',
       'page arguments' => array(4),
       'access callback' => 'school_administration_access',
       'access arguments' => array(1),
       'weight' => 0,
       'type' => MENU_CALLBACK,
     );
   

   The menu uses three '%', but only one of those values are not used; in that case, the menu should be $items['school_administration/browse/%'], and the rest of the definition changed. I am not sure if 'access arguments' => array(1) is correct, or you meant 'access arguments' => array(2).

5.       $cyear = SCHOOL_ADMINISTRATION_C_YEAR;
         $result = t("@acd ACADEMIC YEAR", array('@acd' => ($cyear - 1) . '-' . $cyear));
   

   There is not need to use a variable that you don't change to contain the value of a constant.

6. substr(), and strlen() should be replaced by Drupal functions.
7. 'school_administration/browse/' . $group . '/' . $sub_group . '/' . $member can be rewritten "school_administration/browse/$group/$sub_group/$member" (and so do similar strings).
8. Use a placeholder in the SQL queries every time the parameter is not a constant.
9. The functions that the comment says to be implementations of hook_validate() are really not; the same is true for the functions the comment says to be implementations of hook_submit().
10. t() doesn't understand placeholders that doesn't start with !, @, %.
11. There are many callbacks that return complex HTML output; it would be better to rewrite them to use theme functions.

1. There are some constants that are not prefixed by the module name.
2. Some of the SQL queries are built concatenating not constant strings; even in the case you use the constant SCHOOL_ADMINISTRATION_C_YEAR (which is not truly a constant, as its value is created by the result of a call to variable_get()) it should be better to use a placeholder (as you already do in other cases).
3. The callbacks I was referring to are the menu callbacks, or the page callbacks; many of them output complex HTML text, and it would be better to rewrite them using theme functions.

1. include_once('school_administration.forms.inc') is not necessary in Drupal 7 because, giving the right information in the .info file, Drupal will include the necessary files; that is also true for Drupal core files, and functions defined in those files.
2. t($txt) should be avoided because it is not possible to extract the string that needs to be translated; it's better to use t('Sentence to translate, which will be catched').

3.   $items['photo/%'] = array(
       'title' => 'photo',
       'description' => 'Administer Your School',
       'page callback' => 'school_administration_portrait_photo',
       'page arguments' => array(1),
       'access callback' => 'school_administration_access',
       'weight' => -2,
     );
   

   The menu relative URL doesn't contain a string that is specific for your module, and it could cause a conflict with another module. It is still possible for somebody else to create a module named photo.module, and your module would not allow to use it; who creates a module is not forced to check all the menu callbacks defined by third-party modules, and see if they are defined as they are supposed to be.

4. Check that the functions you are using inside SQL queries are not specific for a database engine; CONCAT_WS() is one of them.
5. Some of the code that you placed in a submit function should be placed in a validation function. This is valid for the code that verify if the user given the right name of a directory (and if it's really a directory).
6. Always use a function Drupal makes available; using a different function just causes a duplication of code which should be avoided.
7. There are still functions that are declared as hooks, which are not hooks.

I already signaled a problem, but the code still have that problem.

Rather than rushing to change the problems reported in the exact code reported, it would be better to check all the code and see if a similar problem exists in other parts of the modules. Just to make an example, the problem of the namespace not being respected has been reported by Dave, but then I reported the same problem twice; the code was different all the times, but when you are said that every Drupal variable names, and all the function names must be prefixed with the module name, you should check that every functions respect that.

You could wait to attach a new copy of the files, and check the code to see if it still needs to be modified as suggested here.

1.   drupal_set_header("Content-type:", "image/pjpeg");
   

   The content type is not the one usually used for JPEG pictures.

2. The code initializes a variable with a constant, but then the variable is not changed at all; in other words, the variable is used as a constant. In that case, why does not the code use the constant directly, then?
3. There are again SQL queries that don't use a placeholder when they should (especially because the value they use is not a constant value). There isn't a rush to attach here a new version of the code; check the code trice, wait 6 hours, re-check it, and then attach the new version.
   We are reviewing the code, but that doesn't mean we are going to do the work for you. ;-)

4. /**
    * To limit access to menu callbacks
    */
   function classes_and_lessons_access($op = 'view') {
     return true;
     GLOBAL $user;
     switch ($op) {
       case 'view':
         if (isset($user->access) && $user->access && $user->status && (
           user_access('access user profiles') || 
           school_administration_student($user->uid) || 
           school_administration_student_parent($user->uid) || 
           school_administration_school_admin($user->uid) ||
           classes_and_lessons_teaching($user->uid) || 
           classes_and_lessons_supervisor($user->uid) ||
           classes_and_lessons_hr_teacher($user->uid)
         ))
         return TRUE;
       default:
         return FALSE;
       //false returns access denied
     }
   }
   

   This function will always return TRUE. Without any comment present, I guess that is not what it is supposed to do.

5. function classes_and_lessons_delete_class_form($form_state) {
     $form['name'] = array(
       '#title' => 'Class',
     );
     return $form;
   }
   

   The form doesn't define any visible element; what is its purpose?

6. function classes_and_lessons_student_list_for_parent($parent_id) {
     //used only in student_marks_student.inc on line 25 
     //check that it might be neddless
     $students = array();
     // ...
     foreach ($queryst AS $st) {
      $students[$st->school_no] = l("$st->school_no $st->name", "student_marks/student//$st->school_no/view");
     }
     return $students;
   }
   

   "student_marks/student//$st->school_no/view" should maybe be "student_marks/student/$st->school_no/view".

1.       switch ($group) {
           // ...
           case 'students' || 'parents' :
             $list = (module_exists('classes_and_lessons')) ? array()
             : school_administration_student_list($sub_group);
             break;
   
           default :
             $list = array();
         }
   

   I am not sure it works as you thought. Trying to execute the following code

   $a = 'test';
   
   switch ($a) {
     case 'a' || 'b' :
       print "AAA";
       break;
     case 'test' || 'tset' :
       print "BBB";
       break;
   }
   

   will print "AAA", differently from what you would suppose.

2. The code uses PHP string functions when there are Drupal functions for that purpose; the reason the Drupal equivalent functions are preferable is because they can handle UTF8 characters even when the multi byte functions are not available.
3. Some SQL queries are not using placeholders, and the code is creating the query by concatenating strings.

Re #36:
1. Should be

case 'students':
case 'parents':
  // do stuff here

1.     return t('<p>First step of administrating a school is '
         . 'administering registration of students, teachers and non-academic staff. '
         . 'This module is a base module of school administration system, and it is '
         . 'responsible from keeping data of people.</p>');
   

   When suppyling a fixed string to t() you should not concat it like this. The correct method is:-

       return t('<p>First step of administrating a school is administering registration of students, teachers and non-academic staff. This module is a base module of school administration system, and it is responsible from keeping data of people.</p>');
   

   Yes, I know your code looks a lot more readable and neat but the reason is the extractor.php program translators use doesn't execute the code, it uses PHP's token_get_all(). The outcome of this is errors are thrown and translators cannot extract the fixed string correctly.

2. function school_administration_access($op = 'main') {
     GLOBAL $user;
     if (school_administration_school_admin($user->uid) || user_access('access administration pages')) :
         return TRUE;
     elseif (school_administration_school_admin($user->uid) || user_access('access administration pages')) :
         return TRUE;
     endif;
     return FALSE;
   }
   

   What is the elseif doing? It looks the same as the if. Also, I may be wrong but the if() : ... endif; format is used by themes. I thought module coding standards were to use curly braces? See http://drupal.org/coding-standards#controlstruct

Some queries are still built concatenating strings.

  switch ($op) {
    case 'new' :
      $condition = "school_no > " . (SCHOOL_ADMINISTRATION_C_YEAR - 2001)*1000;
      break;

    case 'old' :
      $condition = "school_no < " . (SCHOOL_ADMINISTRATION_C_YEAR - 2001)*1000;
      break;