Commerce CM-CIC

welcome,

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also create a README.txt that follows the guidelines for in-project documentation.

These points have to be fixed for now:

• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
  

  ./kit/CMCIC_Tpe.inc.php
  

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxolmetadavid1550282git

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

Your sandbox project is empty! I need this payment method for Drupal Commerce so I can contrib to this module. Can you post a version of your module in your sandbox repository?

Ok, It's better with the right branch! I check the module and I give you my feedback.

First: the "package" key in info files is not required and it is perfectly fine if modules live in the "Other" caegory. And please be more verbose in your reviews, just posting that the package is missing is not enough. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

manual review:

1. "die("Erreur paramétre " . $constants[$i] . " indèfini");": do not use die(), you should stop the execution gracefully and log errors. Also in English please.
2. commerce_cmcic_schema(): dates are normally stored as unix timestamps in drupal, are you sure you want to store strings?
3. "watchdog('commerce_cmcic', sprintf('The data received from bank server are %s', var_export($payment_data, TRUE)));": do not use sprintf() here, just pass the dynamic value as third parameterin an array to watchdog(). Also elsewhere.
4. "require_once 'includes/commerce_cmcic.inc';": does that work? This is a relative path, better use require_once dirname(__FILE__) . '/mymodule.inc';
5. commerce_cmcic_response(): currently anyone can POST arbitrary data to that function which is logged in the database. As soon as an administrator accesses the log via the web interface malicious scripts could be executed leading to XSS vulnerabilities. You should use watchdog() as I already said with the "@" or "%" placeholder to sanitize the dynamic POST data. And you should check for a valid order early and deny access if necessary.

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

According to post #9, #10, #11, #12, I think status of the issue has been forgot to change to "need review".
So I switch it back.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (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. You have to get a review bonus to get a review from me.

manual review:

1. "define("CMCIC_CLE", $settings['security_key']);": do not use global constants to pass things to your class. Use constructor parameters to do that, or one parameter that is the settings array. Not sure if you provided the class and are able to change that?
2. commerce_cmcic_response(): the "@" placeholder will run check_plain() for you, so no need to do that extra check_plain() in the foreach loop beforehand. Double escaping is bad.
3. "exit(0);": do not use exit(), use drupal_exit() instead.

Is CmcicTpe.inc.php third party provided code or did you write that yourself? If the former, is it licensed under the GPL or GPL-compatible?

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

Please add all your reviews to the issue summary so that we can track them.

manual review:

1. commerce_cmcic.info: dependencies to commerce modules are missing?
2. theme_cmcic_icons_title(): all user facing text must run through t() for translation.
3. "module_load_include('inc', 'commerce_cmcic', 'includes/commerce_cmcic');": no need to use module_load_include as you are including files from your own module which you already know where they are. Use something like require_once 'mymodule.inc';. Also elsewhere.
4. License.txt: Remove that, all code on drupal.org is GPLv2 only and a license file will be automatically added for module downlaods.
5. "exit(0);": do not use exit, use drupal_exit() instead.
6. commerce_cmcic_history_load(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075

The license file is a blocker, otherwise looks nearly ready to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I think the best option to integrate the required CM/CIC classes would be to use the Libraries API and require the user to put that code somewhere under sites/all/libraries.

@garphy Your request make sense in case of other bank can have a different structure but it's look like the class provided is enough to work well for both banks targeted by the project.

I saw you put your request and patch in issue queue of the project so these is just a suggest and not a blocker to a full project review.

Also all comments (and the license files blocker point) looks to have been corrected.

Then I've seen a good comment in the issue queue #1898832: Order status should not be updated when payment is completed which says "I have been using the module in production for a few months and all works well, thanks a lot for your contribution! I think it is time you turn it into a full project."

I also think it's time for these module after 9 months!

I saw you put your request and patch in issue queue of the project so these is just a suggest and not a blocker to a full project review.

This is obviously a suggest and by no means a blocker.
I've mostly completed the review in both cases. I just have to review the callback invocation from the bank.

I thought externalizing that piece of code would somehow make the licensing issues easier to manage.

I'll be more than happy to see that module officially promoted as a project !

Thanks for your contribution, olmeta.david!

I updated your account to let you 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 get 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.