Commerce Eurobank Redirect

Hi drupalulis,

Thanks for submitting a project application for review. To help you track the status of your application you might want to enable email notifications for changes to your application:

http://drupal.org/project/issues/subscribe-mail/projectapplications

You might also want to participate in the review bonus program - this should help ensure your application is review quickly:

http://drupal.org/node/1410826

The PAReview tool identified a few issues which need addressing:

http://ventral.org/pareview/httpgitdrupalorgsandboxncharalampidis1364916git

• It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
   
• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). Please see the above link for further information.
   

Please check/fix these issues before changing the status back to "needs review"

Hi,

here are my suggestions after doing manual review:-

1) First of all code is not properly formatted. Please follow proper indentation of code as mentioned here.

There is still a master branch in repo. Please remove it.

Please make sure to close your ventral issues here : http://ventral.org/pareview/httpgitdrupalorgsandboxncharalampidis1364916git

Thanks
Parwan

Apologies - whilst I can see you do have a 7.x-1.x branch it looks like your recent changes have been made in the master branch. You might want to merge them to 7.x-1.x and remove the master branch.

Adding PAReview: Commerce. Got removed after my comment.

Dear drupalulis, there seems to be a lot of coding errors with your module. I run the project through the online automated project review system. Here are the things you need to fix:

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 1 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | Line exceeds 80 characters; contains 85 characters
3 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...ll/modules/pareview_temp/test_candidate/commerce_eurobank_redirect.info
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | "description" property is missing in the info file
1 | ERROR | "core" property is missing in the info file
--------------------------------------------------------------------------------

FILE: .../modules/pareview_temp/test_candidate/commerce_eurobank_redirect.module
--------------------------------------------------------------------------------
FOUND 129 ERROR(S) AND 2 WARNING(S) AFFECTING 79 LINE(S)
--------------------------------------------------------------------------------
8 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
12 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
16 | ERROR | Inline comments must start with a capital letter
16 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
22 | ERROR | Missing function doc comment
27 | ERROR | Spaces must be used to indent lines; tabs are not allowed
27 | ERROR | Array indentation error, expected 6 spaces but found 3
28 | ERROR | Spaces must be used to indent lines; tabs are not allowed
28 | ERROR | Array indentation error, expected 6 spaces but found 3
29 | WARNING | A comma should follow the last multiline array item. Found: )
66 | ERROR | Whitespace found at end of line
71 | ERROR | More than 2 empty lines are not allowed
84 | WARNING | A comma should follow the last multiline array item. Found: )
90 | ERROR | Whitespace found at end of line
90 | ERROR | The open comment tag must be the only content on the line
91 | ERROR | Whitespace found at end of line
91 | ERROR | Function comment short description must be on a single line
109 | ERROR | Spaces must be used to indent lines; tabs are not allowed
109 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
109 | ERROR | Array indentation error, expected 4 spaces but found 1
122 | ERROR | Whitespace found at end of line
139 | ERROR | Wrong function doc comment end; expected "*/", found "**/"
148 | ERROR | Whitespace found at end of line
152 | ERROR | Concat operator must be surrounded by spaces
152 | ERROR | Concat operator must be surrounded by spaces
155 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
155 | ERROR | No space found after comma in function call
163 | ERROR | Expected 1 space before "+"; 0 found
163 | ERROR | Expected 1 space after "+"; 0 found
167 | ERROR | Function comment short description must end with a full stop
173 | ERROR | Whitespace found at end of line
175 | ERROR | Concat operator must be surrounded by spaces
175 | ERROR | Concat operator must be surrounded by spaces
180 | ERROR | Spaces must be used to indent lines; tabs are not allowed
180 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
180 | ERROR | Array indentation error, expected 4 spaces but found 1
181 | ERROR | Spaces must be used to indent lines; tabs are not allowed
181 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
181 | ERROR | Array indentation error, expected 4 spaces but found 1
182 | ERROR | Spaces must be used to indent lines; tabs are not allowed
182 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
182 | ERROR | Array indentation error, expected 4 spaces but found 1
183 | ERROR | Spaces must be used to indent lines; tabs are not allowed
183 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
183 | ERROR | Array indentation error, expected 4 spaces but found 1
184 | ERROR | Spaces must be used to indent lines; tabs are not allowed
184 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
184 | ERROR | Array indentation error, expected 4 spaces but found 1
185 | ERROR | Spaces must be used to indent lines; tabs are not allowed
185 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
185 | ERROR | Array indentation error, expected 4 spaces but found 1
186 | ERROR | Spaces must be used to indent lines; tabs are not allowed
186 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
186 | ERROR | Array indentation error, expected 4 spaces but found 1
189 | ERROR | Inline comments must start with a capital letter
193 | ERROR | BREAK statements must be followed by a single blank line
193 | ERROR | break statement indented incorrectly; expected 6 spaces, found
| | 3
194 | ERROR | Line indented incorrectly; expected 4 spaces, found 3
196 | ERROR | break statement indented incorrectly; expected 5 spaces, found
| | 3
215 | ERROR | Missing function doc comment
215 | ERROR | Expected 1 space between the closing parenthesis and the
| | opening brace; found 0
216 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
221 | ERROR | A cast statement must be followed by a single space
222 | ERROR | No space found after comma in function call
225 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
232 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
234 | ERROR | Spaces must be used to indent lines; tabs are not allowed
234 | ERROR | Line indented incorrectly; expected 4 spaces, found 1
237 | ERROR | Inline comments must start with a capital letter
237 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
238 | ERROR | No space found after comma in function call
241 | ERROR | Inline comments must start with a capital letter
241 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
242 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
247 | ERROR | Inline comments must start with a capital letter
247 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
251 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
256 | ERROR | Concat operator must be surrounded by spaces
256 | ERROR | Concat operator must be surrounded by spaces
259 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
268 | ERROR | Missing function doc comment
268 | ERROR | Expected 1 space between the closing parenthesis and the
| | opening brace; found 0
269 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
271 | ERROR | Whitespace found at end of line
274 | ERROR | A cast statement must be followed by a single space
275 | ERROR | No space found after comma in function call
279 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
281 | ERROR | Spaces must be used to indent lines; tabs are not allowed
281 | ERROR | Line indented incorrectly; expected 4 spaces, found 1
284 | ERROR | Inline comments must start with a capital letter
284 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
287 | ERROR | Inline comments must start with a capital letter
287 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
288 | ERROR | No space found after comma in function call
291 | ERROR | Inline comments must start with a capital letter
291 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
302 | ERROR | Multi-line assignments must have the equal sign on the second
| | line
302 | ERROR | Expected 1 space after "="; 2 found
302 | ERROR | Whitespace found at end of line
304 | ERROR | Whitespace found at end of line
314 | ERROR | Missing function doc comment
314 | ERROR | Expected 1 space between the closing parenthesis and the
| | opening brace; found 0
315 | ERROR | Concat operator must be surrounded by spaces
315 | ERROR | Concat operator must be surrounded by spaces
318 | ERROR | Missing function doc comment
318 | ERROR | Expected 1 space between the closing parenthesis and the
| | opening brace; found 0
319 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
321 | ERROR | Whitespace found at end of line
324 | ERROR | A cast statement must be followed by a single space
325 | ERROR | No space found after comma in function call
329 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
330 | ERROR | Spaces must be used to indent lines; tabs are not allowed
330 | ERROR | Line indented incorrectly; expected 4 spaces, found 1
331 | ERROR | Spaces must be used to indent lines; tabs are not allowed
331 | ERROR | Line indented incorrectly; expected 4 spaces, found 1
331 | ERROR | Concat operator must be surrounded by spaces
331 | ERROR | Concat operator must be surrounded by spaces
334 | ERROR | Inline comments must start with a capital letter
334 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
337 | ERROR | Inline comments must start with a capital letter
337 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
338 | ERROR | No space found after comma in function call
341 | ERROR | Inline comments must start with a capital letter
341 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
352 | ERROR | Multi-line assignments must have the equal sign on the second
| | line
352 | ERROR | Expected 1 space after "="; 2 found
352 | ERROR | Whitespace found at end of line
354 | ERROR | Whitespace found at end of line
361 | ERROR | Concat operator must be surrounded by spaces
361 | ERROR | Concat operator must be surrounded by spaces
362 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Hi,

There are a couple of minor touch ups at the PAReview Tool link:
http://ventral.org/pareview/httpgitdrupalorgsandboxncharalampidis1364916git

From looking through the module a suggestion I have is to use drupal's Render Arrays suffixes, prefixes and classes for html for more flexibility for themers who might want to change these elements.

We have it live and working at www.parachute.gr.

Testing on my site and now the credit card payments work like a charm. Thanks drupalulis!

Changed the branch name from master to 7.x-1.x

manual review:

1. "define('EUROBANK_REDIRECTION_MODE_TEST', 0);": all constants defined by your module need to be prefixed with your module's name to avoid name collisions with others.
2. "drupal_alter('commerce_eurobank_redirect_configuration', $conf);": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php . Also elsewhere.
3. "Implements hook_redirect_form().": hook_redirect_form() does not exist, or is that a commerce module hook which is not prefixed correctly?
4. commerce_eurobank_redirect_order_confirm(): where is the security in this page callback? How do you know that the request is coming from the payment service and not from an attacker that forges the request? Shouldn't you verify a token or something that only the payment gateway can know? I can provide all needed data for that function by simply looking at the hidden form values in the checkout form. There is no access callback for that page callback, so anyone could spoof a request that completes an order that they previously created. Maybe I'm missing something?

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

Hello,

1.

// Wait 2 seconds so as the order manage to change status. 
sleep(3);

Change to Wait 3 seconds.

2. I guess you're getting this particular module reviewed out of the selection you have to be able to make full projects but I did wonder why this can't be a sub-module of the existing Commerce Eurobank module developed by your company?

Apart from this in terms of the module in itself I would say it RTBC especially after the security vulnerability being fixed.

I see, just wanted to get clear that up as I wasn't entirely sure, but I do understand.

manual review:

• "watchdog("commerce_eurobank_redirect", '<strong>VALIDATE: Empty Post Parameters</strong><br/><pre>' . print_r($post, TRUE) . '</pre>');": this is vulnerable to XSS exploits. An attacker can submit arbitrary POST data to the commerce_eurobank_redirect_order_validate() callback which is written unsanitized to the log. As soon as an admin visits the logs admin page Javascript and other nasty stuff could be executed. Make sure to use the appropriate placeholders with watchdog() instead and make sure to read http://drupal.org/node/28984 again. Also elsewhere.

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

manual review:

• why did you remove all the watchdog() calls instead of fixing them with "@" placeholders?

But otherwise looks RTBC to me now!

Assigning to mitchell as he might have time to finally approve this.

Thanks for your contribution, nchar!

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 stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other project applications that are pending review.

Thanks to the dedicated reviewer(s) as well.