[D7] GPA Calculator

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/gpa_calculator.inc
---------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------
6 | ERROR | [ ] Doc comment short description must be on a single line,
| | further text should be a separate paragraph
47 | ERROR | [x] Each index in a multi-line array must be on a new line
47 | ERROR | [x] Each index in a multi-line array must be on a new line
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

Manual Review

Individual user account

Yes.

No duplication

Yes: While https://www.drupal.org/project/cgpa_calculator is an existing D6 module, it is minimally supported and there is no such module for Drupal 7+.

Master Branch

Yes

Licensing

Yes

3rd party code

Yes

README.txt/README.md

Yes.

Code long/complex enough for review

Yes

Secure code

Yes.

Coding style & Drupal API usage

1. gpa_calculator.admin.inc (line 56): $grades_description should probably have a check_plain() around it. Not required since the variable is built from hard coded strings.
2. No @file block in the .js files. I'm not sure if that's actually required or not.
3. Comments in css file should be capitalized with a full stop.
4. Check coding standards on .js files. Hint: drush drupalcs sites/all/modules/custom/gpa_calculator/ --extensions=js
5. Can you add a hook_install: https://www.drupal.org/node/161085#recommended_practices
6. "variable_get(): all variables that your module defines must be removed in hook_uninstall().

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Hi hurley, thanks for the work.

Here is the list of issues which needs to be fixed.

1) Is there exists any special need to add js and css to "gpa_calculator_settings_form" via form_alter?
If not you can add the js and css in the form constructor function itself.

function gpa_calculator_form_alter(&$form, &$form_state, $form_id) {
  switch ($form_id) {
    case 'gpa_calculator_settings_form':
      drupal_add_css(drupal_get_path('module', 'gpa_calculator') . '/css/gpa_calculator.admin.css');
      drupal_add_js(drupal_get_path('module', 'gpa_calculator') . '/js/gpa_calculator.admin.js', array('scope' => 'footer'));
      break;
  }
}

2) Keep consistency when providing parameters to the form constructor functions. you have created two forms each with different signatures i;e

/**
 * GPA Calculator settings form.
 */
function gpa_calculator_settings_form() {

and

/**
 * GPA Calculator form.
 */
function gpa_calculator_form($form, &$form_state) {

3) You can use $form['#attached']['js']/$form['#attached']['css'] to add js/css to your form. It takes care of loading of js/css also when form is served form cache. Its a good practice to have.

4) There a various place where unnecessary variables have been used.
for eg

       $gpa_calculator_school_name = variable_get('gpa_calculator_school_name', '');
  $gpa_calculator_instructions = variable_get('gpa_calculator_instructions', '');
  $gpa_calculator_grades = variable_get('gpa_calculator_grades', '');

  $form['gpa_calculator'] = array(
    '#type' => 'fieldset',
    '#title' => t('Settings for GPA Calculator'),
    '#collapsible' => TRUE,
    '#collapsed' => FALSE,
  );

  $form['gpa_calculator']['gpa_calculator_school_name'] = array(
    '#type' => 'textfield',
    '#title' => t('School'),
    '#description' => t('Enter you school\'s name here. If blank, block subject will read as "GPA Calculator."'),
    '#default_value' => $gpa_calculator_school_name,
  );

  $form['gpa_calculator']['gpa_calculator_instructions'] = array(
    '#type' => 'textarea',
    '#title' => t('Instructions'),
    '#description' => t('Provide instructions or a description for your GPA calculator.'),
    '#default_value' => $gpa_calculator_instructions,
  );

  $grades_options_example = ' 4.0|A ';
  $grades_options_example .= '3.67|A- ';
  $grades_options_example .= '3.33|B+ ';
  $grades_options_example .= '3.0|B ';
  $grades_options_example .= '2.67|C+ ';
  $grades_options_example .= '2.33|C ';
  $grades_options_example .= '2.0|C- ';
  $grades_options_example .= '1.67|D+ ';
  $grades_options_example .= '1.33|D ';
  $grades_options_example .= '1.0|D- ';
  $grades_options_example .= '0.0|F';

  $grades_description = t('Enter grade options for the select box values.  Key-value pairs must be entered separated by pipes. i.e. safe_key|Some readable option on separate lines.  If blank, default values will be:') . $grades_options_example;

  $form['gpa_calculator']['gpa_calculator_grades'] = array(
    '#type' => 'textarea',
    '#title' => t('Grades'),
    '#description' => check_plain($grades_description),
    '#default_value' => $gpa_calculator_grades,
  );

  $form = system_settings_form($form);
  return $form;
    

$gpa_table_head = '<div id="grades_table">';
  $gpa_table_head .= '<div class="gpa-table-thead">';
  $gpa_table_head .= '<div class="gpa-table-cell gpa-th">#</div>';
  $gpa_table_head .= '<div class="gpa-table-cell gpa-th">' . t('Class/Course Name') . '</div>';
  $gpa_table_head .= '<div class="gpa-table-cell gpa-th">' . t('Grade') . '</div>';
  $gpa_table_head .= '<div class="gpa-table-cell gpa-th">' . t('Credits Earned') . '</div>';
  $gpa_table_head .= '</div>';
  $gpa_table_head .= '<div class="gpa-table-body"></div>';

  $form['gpa_table_head'] = array(
    '#markup' => $gpa_table_head,
  );

  $gpa_table_end = '</div>';

  $form['gpa_table_end'] = array(
    '#markup' => $gpa_table_end,
  );

  module_load_include('inc', 'gpa_calculator', 'gpa_calculator');

      $gpa_calculator_school_name = variable_get('gpa_calculator_school_name', '') . ' ';

      $gpa_calculator = drupal_get_form('gpa_calculator_form');
      $blocks['subject'] = $gpa_calculator_school_name . t('GPA Calculator');
      $blocks['content'] = $gpa_calculator;
  

There are few other examples also but i think you got the point

One example of correct usage may be

$form['gpa_calculator']['gpa_calculator_school_name'] = array(
    '#type' => 'textfield',
    '#title' => t('School'),
    '#description' => t('Enter you school\'s name here. If blank, block subject will read as "GPA Calculator."'),
    '#default_value' => variable_get('gpa_calculator_school_name', ''),
  );

Declaring variables should be minimized until and unless really needed since every variable takes some memory space.

5) Instead of using $(document).ready(function() {}) in your javascript file you should use

Drupal.behaviors.myModuleBehavior = function (context) {
  //Do some fancy stuff
};

see drupal javascript api for more details

File gpa_calculator, function gpa_calculator_block_view()
Switch cases, though not complusory, should always have a default case.

File gpa_calculator.js, gpa_calculator.admin
Most common mistake in writing .js file is the use of $(document).ready function, which should be avoided. Drupal provide Drupal.behaviors functionality which should be used always instead of $(document).ready as it help in attaining same results.
The variable from Drupal.settings can easily be used from this.

Drupal.behaviors.behavior_name = {
  attach: function(context) {
    if(Drupal.settings.behavior_name){
(function ($) {
        $.each(Drupal.settings.behavior_name, function(id) {

In this way you can access the variable added in Drupal.settings array.

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxhurley2309905git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Verified that the issues above were fixed.

Those were just minor UI changes. Back to RTBC.

@hurley, nice module! Thank you for your contribution.

I installed this module, it worked as intended (+1 from my side).

Here are some minor suggestions for you.
1. In Readme.txt file, Configuration contains some special characters that need to be fix.
» People » Permissions:
2. In gpa_calculator_settings_form, you can directly return the system_settings_form($form) instead of following:

$form = system_settings_form($form);
return $form;

3. You can provide GPA Calculator Settings page URL (admin/config/gpa-calculator/gpa-calculator-settings) in .info to make it direct access from module page.
You can add following line into .info file.
configure = admin/config/gpa-calculator/gpa-calculator-settings

Other than this I have assigned "GPA Calculator" block on two pages, in one scenario when no Grades is there, this form get submitted, once I click on "Calculate" button as I have reviewed your code there is no submit handler found for gpa_calculator_form form. I think either you completely prevent this form submission or provide some message.

Rest looks good to me, it is ready to go :)

Thankyou for your hard work!

Minor, but easy enough that the changes might as well be made.

Changes look good.

removed copies of automated review.

Review of the 7.x-1.x branch (commit 0f392fe):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. what are the differences to the existing project at https://www.drupal.org/project/cgpa_calculator ? Are they somehow related? Please add that to the project page.
2. gpa_calculator_block_view(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as school name in the admin settings form then I will get a nasty javascript popup when the block is displayed. You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984 again. In your case you should not concatenate the variable to the translatable strings but rather use the automatically sanitized "@" variable placeholder with t(). And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
3. gpa_calculator_settings_form(): The check_plain() is wrong here. There is no user provided text involved, so there is nothing untrusted to be sanitized.
4. gpa_calculator_form(): do not use drupal_add_js() here, rather use #attached to add the JS file to the form render array. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
5. gpa_calculator_form(): this is also vulnerable to XSS. The $gpa_instructions variable comes form user provided input, so you need to sanitize it.

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

All changes looks good to me after a manual review. I have also tested functionality of this module w.r.t XSS & other possibilities and it works fine as intended.

There are some special characters found in gpa_calculator_help() function that need to be fix.

Project page and Readme.txt looks good to me. Inline comments and docs looks good, JS file also very well organised.

Not seeing any further blockers... so moving to RTBC.

Great Job hurley!

Hello @hurley

A quick review of the 7.x-1.x branch (commit 62723d8).

1. gpa_calculator.admin.inc:38 Redundant check_plain(). The text is already sanitized by t().
2. Avoid use of multiple consecutive spaces (including tabs, line breaks, etc) in translatable strings (in couple places of module's code).
3. gpa_calculator.module:83 Block info element should be a translated string (hook_block_info()).

4. gpa_calculator_form(): this is also vulnerable to XSS. The $gpa_instructions variable comes form user provided input, so you need to sanitize it.

   - Still not fixed:

   • The module does not know what filters are used by 'filtered_html' text format and it does not check if user has an access to this format. See check_markup().
   • gpa_calculator_grades variable must be sanitized too. Maybe take a look at the check_plain() function, not a check_markup().
5. gpa_calculator.inc:20 I guess here should be if ($grade != '') {.
6. gpa_calculator.inc:38 (and other places) If setting an HTML element ID attribute, make sure it won't duplicate other IDs on page. Maybe prefix them with a module's name.
7. gpa_calculator_form(): Instead of building markup here maybe you should create a template file.
8. It seems that all form submission logic (both end-user's and admin's) is done in JS. Have you thought of people who do not have JS in their text-only browsers?

The vulnerability is mitigated by the fact that:

• filtered_html text format must have filter allowing user to insert executable PHP code or JS <script>s;
• an attacker must have a role with permission to 'administer gpa calculator'.

Hi
At the time of module configuration, i am getting following error.

Fatal error: Can't use function return value in write context in /opt/lampp/htdocs/drupal730/sites/all/modules/gpa_calculator/gpa_calculator.admin.inc on line 31

Please fix it!!

There are also some coding issue, mentioned on http://pareview.sh/.
http://pareview.sh/pareview/httpgitdrupalorgsandboxhurley2309905git

Thanks!!

Hey Hurley, I LOVE this idea. I have a daughter in 5th grade and would love to share this with her principal.

That said, this module is not working for me at all and I cannot wait till it becomes a full project, because this is a needed module for sure among school educators.

Issues (Or let me know what I am doing wrong as an end user:)

1. According to the readme.txt file under Configuration says Grant the Administer GPA Calculator permission to any...
   After I enable the module, the role does not appear for GPA Calculator and does not show up on the permissions page.
2. The Readme.txt file also says Customize the GPA Calculator in Administration > Configuration >
   GPA Calculator Administration > GPA Calculator settings.. When I click that link in my Config settings, I get a PHP error: Fatal error: Can't use function return value in write context in C:\wamp\www\drupal7a\sites\all\modules\gpa_calculator\gpa_calculator.admin.inc on line 42

See the attached screenshot for the error called gpa-calc-error.jpg. I am using Wampserver for module testing and development, so I am not sure if that makes a difference.

Possible Solution:
I looked at line 42 in the admin include see that you are using variable_get(), but when I use this function, I never pass it array elements. Instead, I define it in my configuration form in hook_form_alter() and then just call it in my implementing function. Just a suggestion.

Looking forward to seeing this module go live! Thank you, hurley, for this contribution.
Bruce

Hurley, thanks for your quick response, I see the GPA permission now and set it, but I have another error after pulling from GIT:

Parse error: syntax error, unexpected '[' in C:\wamp\www\drupal7a\sites\all\modules\gpa_calculator\gpa_calculator.admin.inc on line 42

Please revise,
Bruce

Ok, I will pull it from GIT again and we can eliminate the fatal errors together.

Keep up the great work on this module. It is very useful and I look forward to seeing it become a full project!

Bruce

hurley, I get a similar error, but now on line 44:

Parse error: syntax error, unexpected '[' in C:\wamp\www\drupal7a\sites\all\modules\gpa_calculator\gpa_calculator.admin.inc on line 44

Let's keep knocking these out. I will pull your fixes and report back to you...

Bruce

manual review:

• gpa_calculator.inc: will still be broken on PHP 5.3, "variable_get('gpa_calculator_instructions', '')['format']" will not work, right? See http://3v4l.org/3KbEJ

review of the 8.x-1.x branch:

1. gpa_calculator_form_alter(): why do you need to alter your own form? Please add a comment. You are already adding the CSS in your Form class?
2. Fatal error: Declaration of Drupal\gpa_calculator\Form\GpaCalculatorSettingsForm::buildForm() must be compatible with Drupal\Core\Form\FormInterface::buildForm(array $form, Drupal\Core\Form\FormStateInterface $form_state)
3. I think the XSS problems are still present in the 8.x-1.x, but since Drupal 8 has no stable release yet anyway this should not be a blocker.

Thanks for your contribution, hurley!

I updated your account so you can 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 stay 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.