[D7] transferuj.pl

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

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.

Corrected Git Clone URL.

@Luke_Nuke, thank you for your work.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Few minor issue.

FILE: /var/www/drupal-7-pareview/pareview_temp/transferuj_pl.module
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
182 | WARNING | Line exceeds 80 characters; contains 112 characters
252 | WARNING | Line exceeds 80 characters; contains 100 characters
--------------------------------------------------------------------------------

FILE: ...pareview/pareview_temp/includes/TransferujPlPaymentMethodController.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------------
16 | ERROR | Class property
| | $payment_method_configuration_form_elements_callback should use
| | lowerCamel naming without underscores
45 | WARNING | Line exceeds 80 characters; contains 85 characters
--------------------------------------------------------------------------------

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

(*)May be: Does not cause module duplication and fragmentation. Can you follow the steps in the link and confirm that the functionality contained in this module should stay separate from Commerce Transferuj.pl?

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: Doesn't follows the guidelines for in-project documentation and the README Template. There is no info on how to use or confgure this, or how it works.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

1. (+) In transferuj_pl_form_redirect function you are directly using <script> tag.
   

     $form['js'] = array(
       '#type' => 'markup',
       '#markup' => '<script type="text/javascript">document.getElementById(\'transferuj-pl-form-redirect\').submit();</script>',
     );
   

   This is not a recommended approach to use <script> tag directly in your code, do it in another way, make sure to read https://www.drupal.org/node/756722 again.

2. In following code, you are accessing $_POST array directly that is completely unsecured to use.
   

    function transferuj_pl_verify() {
     $pid = explode(':', $_POST['tr_crc'])[0];
     $payment = entity_load_single('payment', $pid);
   
     $payment->method->controller->processFeedback($_POST, $payment);
     echo "TRUE";
   }
   

3. In transferuj_pl_verify_access function again using $_POST directly and also using md5() that should not be used for anything security related. You should use drupal_hmac_base64() instead. Can you just use drupal_get_token() instead?

Coding style & Drupal API usage

1. In class TransferujPlPaymentMethodController contains some constant values like IP and redirect URL, can you add some comment on these. Can;t we manage this information through admin?
2. Function public function validate(Payment $payment, PaymentMethod $payment_method, $strict) { don't contains any body, if it require mention this in To Do list or remove it.
3. In TransferujPlPaymentMethodController.inc file, as mentions above, better to avoid md5() and use other function.
4. (*) This file transferuj_pl.pages.inc code need to be improved. Like you can use #attached to add your js code. Sanitize $_POST array before using in your code, make sure to read https://www.drupal.org/node/28984 again.
5. hook_help() is missing in your module.

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.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!