IBIS (First Data Latvia) | Ubercart Payment

Review of the 6.x-1.x branch:

• Remove the translations folder, translations are done on http://localize.drupal.org
• The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
  

  ./includes/config.php
  ./includes/Merchant.php
  

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

  function uc_payment_method_ibis($op, &$arg1) {
  

• ./includes/Merchant.php: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  51-    */
  66- */
  123-*/
  146-*/
  170-*/
  192-*/
  211-*/
  228-*/
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  translations/lv.po:1:# $Id$
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./uc_ibis.install ./includes/config.php ./TODO.txt
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

please be patient, the reviewers do this voluntary and in their sparetime
Most project applications take two weeks until their accepted.

You can help us moving this process by trying to review other applications yourself, you will definitely learn something ;-)

Still some formatting issues, see http://ventral.org/pareview/httpgitdrupalorgsandboxrudins1352366git

Switched back to needs review, so in-depth reviews won't be blocked by coding standart issues.

Review of the 6.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• "return MENU_ACCESS_DENIED;": I think you should use drupal_access_denied() here instead.
• uc_ibis_failed(): drupal_goto() already calls exit(), so remove that after it.
• "drupal_set_message(check_plain(t('Tehnical error occurred! Please contact merchant! <br><br> !message')...": check_plain() will break your tags, so remove them.
• uc_ibis_process_sms(): indentation errors in the if/else statements, always use 2 spaces for indentation.
• your module file is quite long, you should put page callbacks into separate include file(s).
• "'#title' => check_plain(t('Transaction id')),": why do you call check_plain() here? No user provided data is involved, so no need to use it.
• uc_ibis_completion_access(): you don't need that function, just use TRUE on the access callback in hook_menu().
• "uc_payment_method_ibis($op, &$arg1)": $arg1 is a horrible variable name, what is it? a order, a user, a node?

The PAReview.sh reviews scripts flagging number of issues, see http://ventral.org/pareview/httpgitdrupalorgsandboxrudins1352366git-6x-1x

Find attachment for more details

I'm sorry for your desperation with the process, for SURE we want new contributors!

You can see most issues flaged by these automated scripts as suggestions, you don't have to make everything perfect.
We just want to make sure that you have an understanding about the right use of Drupals API.

For sure you can review other applications ! Please do it!
By reviewing other applications you can enhance the speed of your's (see http://drupal.org/node/1410826)

Found a few issue flagged by ventral, if you feel that none of these are intended to be solved or cant be solved in your context, then please explain. I would like to state that the these issue are different from the once mentioned in the earlier review. Ignoring the minor, but I am bit more worried about the ERROR flags.

sites/all/modules/pareview_temp/./test_candidate/uc_ibis.sms.inc:
+32: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
+33: [minor] in most cases, replace the string function with the drupal_ equivalent string functions

sites/all/modules/pareview_temp/./test_candidate/uc_ibis.pages.inc:
+32: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
+35: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
+39: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
+251: [minor] in most cases, replace the string function with the drupal_ equivalent string functions

sites/all/modules/pareview_temp/./test_candidate/uc_ibis.dms.inc:
+32: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
+33: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
+72: [minor] in most cases, replace the string function with the drupal_ equivalent string functions

Status Messages:
Coder found 4 projects, 4 files, 9 minor warnings, 0 warnings were flagged to be ignored

FILE: ...l-7-pareview/sites/all/modules/pareview_temp/test_candidate/uc_ibis.css
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
3 | ERROR | Multiple CSS properties should be listed in alphabetical order
--------------------------------------------------------------------------------

FILE: ...pareview/sites/all/modules/pareview_temp/test_candidate/uc_ibis.dms.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
19 | ERROR | Use the function ip_address() instead of $_SERVER['REMOTE_ADDR']
--------------------------------------------------------------------------------

FILE: ...-pareview/sites/all/modules/pareview_temp/test_candidate/uc_ibis.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
173 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
--------------------------------------------------------------------------------

FILE: ...review/sites/all/modules/pareview_temp/test_candidate/uc_ibis.pages.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
29 | ERROR | Use the function ip_address() instead of $_SERVER['REMOTE_ADDR']
--------------------------------------------------------------------------------

FILE: ...pareview/sites/all/modules/pareview_temp/test_candidate/uc_ibis.sms.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
19 | ERROR | Use the function ip_address() instead of $_SERVER['REMOTE_ADDR']
--------------------------------------------------------------------------------

Other review comments:

1. File: uc_ibis.sms.inc, Line: 35, 41 - Please refrain from using INSERT for adding record to the DB instead use drupal_write_record
2. File: uc_ibis.sms.inc, Line: 28 - Better to fetch the value from variable_get to a variable before using it in the constructor
3. File: uc_ibis.pages.inc, Line: 36, 37, 40, 41- Indentation issue 4 spaces needed, more given.
4. File: uc_ibis.pages.inc, Line: 84,96,160, 273, 284 - Use drupal_write_record instead of using UPDATE/INSERT directly in the query
5. File: uc_ibis.pages.inc, Line: 122 - Two code lines are placed in the same line, please split on ';' to form two line
6. File: uc_ibis.pages.inc, Line: 127 - fetch value from variable_get in a variable before using it
7. File: uc_ibis.pages.inc, Line: 219 - check_plain(t()), not sure what you want achieve here. t() should be sufficient
8. File: uc_ibis.module, Line: 382, 388 - check_plain(t()), not sure what you want achieve here. t() should be sufficient
9. File: uc_ibis.module, Line: 487 - Use drupal_write_record instead of using UPDATE/INSERT directly in the query
10. File: uc_ibis.install - There is no index defined for the tables being created. Consider having indexes to quicken the queries.
11. File: uc_ibis.install - Leave a single line space between each array (table).
12. File: uc_ibis.dms.inc, Line: 35, 41, 74, 81 - Use drupal_write_record instead of using UPDATE/INSERT directly in the query
13. File: uc_ibis.dms.inc, Line: 76, 84 - Translation for the dynamic text is always not possible. Consider splitting the string into two parts: static and dynamic (from variable)

The review comments above are not limited to line numbers and files mentioned, please make sure that similar issues found elsewhere should as well be fixed.
Also I observed use of $_POST, though check_plain has been used, still like to recommend use of $form_state if forms are involved in fetching this data.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Almost there. A few smaller things:

http://ventral.org/pareview/httpgitdrupalorgsandboxrudins1352366git-6x-1x
I realize you're not changing the function name, that's fine - just ignore that.
The function issues are that substr should be drupal_substr.
Then a couple of indent errors.

Manual Review
- Remove example directory
- Blank lines are a bit inconsistent. Sometimes you have a blank line after a new function name, sometimes not. Sometimes there's a blank line after an if() statement, sometimes not, switch statements, etc. I'd recommend being consistent. Definitely most common in Drupal is no blank line.
- uc_ibis.pages.inc line 121 exit() is not needed.
- uc_ibis.sms.inc Use drupal_goto instead of header. You can pass in query string paramaters with the second parameter. Same issue on uc_ibis.dms.inc
- Based off comparison of the code for uc_ibis_process_dms and uc_ibis_process_sms, it would make sense to make a generic function that handled this. While the functions are almost identical, in one version you use watchdog, in the other you don't. In one you give a message to the user, in the other you don't. If it was a generic function, then it would be the same regardless of how the user is paying.
- I'd recommend implementing hook_requirements to make sure the user has downloaded an correctly placed the merchant file.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)