jimkont [jimkont]

You should apply for a CVS account because you have a module to add in Drupal.org repository, not because you would like to create a module.
The CVS application needs to review your code, which needs to be almost complete, when you apply for a CVS account.

I think this would be a useful contribution if it were compatible with Drupal 6 / Ubercart 2.

My concern is that I don't think you should be starting a new project to host a Drupal 5 / Ubercart 1 module, as it is almost obsolete right now. A big problem on drupal.org is an abundance of outdated and unsupported modules that no longer work with the current version of Drupal - will this module be just another one of those right from the start?

I think if you ported it to Drupal 6 / Ubercart 2 *before* you asked for a CVS account that would demonstrate that you want to actively maintain this module.

Please complete the code before to submit it to review; the module file contains 69 lines of TODO.

Initial review of code and files without installing or trying to use:

1. Overall, if you plan on adding support for other Greek payment methods, it would be wise to rename the module something more generic.
2. You have image files, what are the licensing on these files?
3. There is a Thumbs.db file in there.
4. readme.txt shoudl be README.txt
5. There is a _notes directories that has an XML file. Is this needed?
6. In install file, you should need docblocks for all functions. See http://drupal.org/coding-standards
7. Ensure that you are using two spaces instead of tabs.
8. In install file, this:
   

     drupal_set_message(st("ProxyPay3 Eurobank settings are available under !link",
       array( '!link' => l('Administer > Store administration › Configuration › Payment settings ',  'admin/store/settings/payment/edit/methods' ) )
   

   you need to make the text translatable in l() with st(). See http://api.drupal.org/api/function/l

9. In install file, the schema needs descriptions filled in.
10. You uninstall hook is not right. Should be:
    

    /**
     * Implementation of hook_uninstall().
     */
    function uc_proxypay3_eurobank_uninstall() {
      drupal_uninstall_schema('uc_proxypay3_eurobank');
    
      // Get global variable array
      global $conf;
      foreach (array_keys($conf) as $key) {
        // Find variables that have the module prefix
        if (strpos($key, 'uc_proxypay3_eurobank_') === 0);
          variable_del($key);
        }
      }
    }
    

    The uninstall schema will drop the table. You should utilize variable_del() function. See this article for a good way to do this: http://zzolo.org/thoughts/tip-managing-variables-drupal-module

11. Your module does not implement a hook_help().
12. Your menu items should have descriptions.
13. Your form alter injext form items into the checkout review form. Doesn't Ubercart offer a hook for this?

14. $title .= '<br /><img src="'. $path .'/cards.gif" style="position: relative; left: 2.5em;">';
    

    This should utilize theme_image().

15.   if ( variable_get('uc_proxypay3_eurobank_merchant_password', '') != $_POST['Password']) {
        print '[NOTOK]';
        watchdog('proxypay3_eurobank', 'Confirmation Error: Wrong password ' . $_POST['Password'] );
        exit();
      }
    

    If this is a response from a Drupal form, you should not be using POST directly. Why are you directly printing to screen? watchdog uses t() directly and you should use variables in your text like that. You should do a module_invoke_all('exit') before exiting.