Banklink provides an integration to the Banklink services provided by Estonian commercial banks.
This is an abstract API level module that allows developers to integrate Banklink payments in their applications.

https://drupal.org/sandbox/picco/2176677

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/picco/2176677.git banklink
cd banklink
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

PA robot’s picture

Status: Needs review » Needs work

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

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.

picco’s picture

The issues detected by PA robot have been resolved:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpicco2176677git

picco’s picture

Status: Needs work » Needs review
pachabhaiya’s picture

Status: Needs review » Needs work

Dear picco,
I found the following issues. Please check it.

1.
After installing this module, I did some changes in admin/config/system/banklink and saved the configurations. When doing this, it inserted lots of rows with name banklink_* in the 'variable' table.

But, when I uninstalled the module, the rows inserted by this module in 'variables' table are not cleared.

I think it will be better if you delete all the variables generated by this module when uninstalling it.

2.
Link to the configuration page from module listing page is not correct.

3.
For some reason, I get the following error message in line 147 and 150 when installing this module (however I bypassed this error by commenting those two lines):

Fatal error: Call-time pass-by-reference has been removed in [drupal project folder link here]/banklink/banklink.module on line 147

picco’s picture

Thank you for your review c.pachabhaiya!

All your points are valid and I reviewed, fixed them.

1) I added .install file that implements hook_install() and cleans up the variables table.
2) Indeed, fixed
3) As the error says, passing by reference was not necessary, removed.

picco’s picture

I also ran PAReview again to make sure everything stays up to standard:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpicco2176677git

picco’s picture

Status: Needs work » Needs review
picco’s picture

Update:
1) I added .install file that implements hook_install() and cleans up the variables table.

I meant hook_uninstall() :)

mister.koz’s picture

Hey Picco,

I've had a look around your module, the code is concise and well documented, just noticed in banklink.module you reference a public and private key direcly in the code (line 508 > 547) i just noticed you don't need the quotes in the heredoc $seb_test_public_key = <<<'EOD'

Beyond that i can't see anything wrong with it.

SchnWalter’s picture

Hello picco,

I would strongly suggest to switch from using the variables table to either using a simple table with ctools exportable or to you could use Entity API and providing all the banks disabled by default.

In this way to do not overcrowd the variables table with data, I guess that most users won't need to enable all options. Also by using the Entity API you'll have custom edit pages for each of your entities. And you could also allow the creation of new entities using the UI if the user knows what to do.

Thake a look at the hook_entity_info(), entity_metadata_hook_entity_info() and ctools/help/export.html.

And the following hook_menu items 'banklink/return/%', 'banklink/cancel/%', 'banklink/reject/%' should be merge into a single item 'banklink/%/%'.

Regards,
Walter S.

picco’s picture

Thank you for your reviews!

#9 - change made
#10 - as the settings for different banks don't share the same schema, it makes sense to actually use the variable api in this case. storing data in a serialized form in one additional database table would not be a better solution
#10 - entries in the hook_menu() should remain separate, as the values of the first argument needs to be specified explicitly. Otherwise, additional error handling should be necessary.

picco’s picture

Could i ask for another review :)

ragnarkurm’s picture

Tere Ivo :)

Great job collecting all these pieces of code.

Browsed through the code and few things got my attention.

  • As it aims to be developer module - surely it needs good documentation (I refer to API functions and callbacks). Minimal usage and refering to test module is not very attractive. Consider API module, see if provided description for developers is enough. - Needs work.
  • On project page I did not notice anything about Drupal Commerce. Is the module compatible/playing with Drupal Commerce? Plans? - Not a blocker.
  • On project page I did not notice anything about Payment module. Is the module compatible/playing with Payment module? Plans? - Not a blocker.
  • Bigger concern is memory footprint. Main part of code resides in banklink.module - 20kb. I guess in standard website the module is used less than 1% of page requests. So what I suggest is to move major part of .module code to some other file, even some hooks or callbacks. But for hooks or callbacks create wrapper functions which will include the other file and call/return to/from the original function. - Not a blocker, can be polished later.
  • Suggestion for code structuring: create directory "banks" and in that each bank would have one .inc file. - Not a blocker, can be polished later.
  • Some typos here and there like "russion" and "throug". - Not a blocker.

(Can you do me a review as well?)

ragnarkurm’s picture

Status: Needs review » Needs work
kaido.toomingas’s picture

  1. I like what Ragnar said about memory footprint or actually I think this is good point.
  2. I did read the issue nr #10 and if I look it this way then I would love to see this module as submodule for Payment API rather than duplication of this functionality
  3. Still if you gonna use variable then you could optimize variable usage by storing array or object per bank settings (It will simplify code and further development)
  4. I think there is no point to add payment reason as variable if its API module
picco’s picture

Ragnar & Kaido,

Thank you for your reviews!

Required changes:
1) I added an banklink.api.php file to the project that documents the public API of the module.

Other comments:
1) The module is a standalone payment module for Estonian payment gateways. It does not interface with DC yet but the correct way to approach it anyway is update the dc_estonian_payments module to use this one as a dependancy as it's more abstract.
2) I agree that there are ways to restructure the code but it should not be a blocker for the code review.

Asking for another review.

picco’s picture

Status: Needs work » Needs review
klausson’s picture

Status: Needs review » Needs work

PAReview.sh still found two issues:

29 | ERROR | Type hint "array" missing for $payment
56 | ERROR | Type hint "array" missing for $response

I do think that implementing this as a part of https://www.drupal.org/project/payment is useful since it would give easier access for other modules like Drupal Commerce. However, I'm not sure whether this should hold up project publication. I could be convinced either way.

I actually think the implementation using the variables table does make sense given picco's explanations above.

But does the openssl dependency need to be explicitly declared somewhere? I know openssl would be expected to be present on most Unix flavors, but I don't think it is guaranteed - especially when considering Windows or Macintosh operating systems.

picco’s picture

Thank you klausson for your review.

1) Fixed the pareview.sh warning
2) Added a note about ssl to README.txt

Payment integration - I would support an approach where a Payment integration submodule would be added to this module in future. The base module should remain without any dependencies of it's own. Any integration with Payment, Commerce or any other system can be added as a next layer on top.

Other than that I hope this module is up to the standards now.

picco’s picture

Status: Needs work » Needs review
robbertv’s picture

Status: Needs review » Needs work

I reviewed the code. I see a couple of minor issues.

  • Per drupal's coding standards, lines should not exceed 80 characters. Several lines in banklink.module exceed this limit.
  • Several of your functions have parameters but no documentation of what those parameters should be. I would suggest adding this to help developers use the functions.
klausi’s picture

Status: Needs work » Needs review

Those 2 points alone are not application blockers since it is not a hard requirement that code lines are under 80 characters. Parameter documentation is nice and recommended but also no hard requirement. Anything else that you found or should this be RTBC instead?

Noe_’s picture

Status: Needs review » Reviewed & tested by the community

Looks RTBC to me.

kscheirer’s picture

Issue tags: +PAreview: security

Security question:

  1. Your menu items have allow open access, this makes your module vulnerable to DDoS attacks. Especially for banklink/payment there should be a permission checked to make sure the user is allowed (by the site admin) to take this action.

Non-blocking issues:

  • You should add more detail to the project page - it looks like this module provides an API for other developers to use, but does not do anything on its own.
  • In banklink_get_banks(), you can refactor that if statement to (!$active || variable_get("banklink_{$bank}_active")). There's no need to check $active twice, and PHP will only evaluate the second (more expensive) statement if the first is false.
  • In banklink_get_settings() there is a lot of bank-specific coding, consider abstracting the banks settings

Assigning to klausi for security question, otherwise this is ready to go from me.

kscheirer’s picture

Assigned: Unassigned » klausi
klausi’s picture

Assigned: klausi » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: -PAreview: security

The open menu callbacks don't look vulnerable on their own since the incoming requests are verified with banklink_verify_signature(). I think the page callback execution should stop immediately if the verification fails, but at least the verified status is passed on the hook that is invoked in the end. So it is absolutely critical that every module implementing the hook must check the verified property before doing anything, which seems very error prone to me and is a potential security risk.

@picco: can you fix that to not invoke the hook when the verification fails?

picco’s picture

Status: Needs work » Needs review

Thank you for your comments.

I still support the initial design of response processing. Since this is an API module, it should not act on any error states by itself, but should forward the adequate information to the module controlling the user facing flow.

If I were to block further execution, I would not be able to control what do I want to do if error state occurs. Mostly I need to give the end user some kind of error message, but the method, how this should be implemented can only be defined in the module implementing the banklink_return() hook.

I could add a separate set of access permissions to the return menu callbacks if you feel that this can still be considered as a blocker?

chandrasekhar539’s picture

Automated Review

http://pareview.sh/ -- has some errors please check the link(http://pareview.sh/pareview/httpgitdrupalorgsandboxpicco2176677git)

Manual Review
.inc file must present in includes folder

chandrasekhar539’s picture

Status: Needs review » Needs work

Automated Review

http://pareview.sh/ -- has some errors please check the link(http://pareview.sh/pareview/httpgitdrupalorgsandboxpicco2176677git)

Manual Review
.inc file must present in includes folder

klausi’s picture

Status: Needs work » Needs review

Those minor formatting issue are surely not application blockers, anything else that you found or should this be RTBC instead?

chandrasekhar539’s picture

Hi klausi,
Except this no thers issue in this module

SoumyaDas’s picture

Issue tags: +Typo error found on module file

Typo error found on module file

SoumyaDas’s picture

Assigned: Unassigned » SoumyaDas
SoumyaDas’s picture

SoumyaDas’s picture

Assigned: SoumyaDas » Unassigned
Issue tags: -Typo error found on module file +PAreview: review bonus
klausi’s picture

Issue tags: -PAreview: review bonus

@SoumyDas: did you add the review bonus tag by accident here? No reviews of other projects listed in the issue summary here? See https://www.drupal.org/node/1975228

SoumyaDas’s picture

@klausi Yeah that is a mistake. Thanks!

kattekrab’s picture

First applied for approval more than 2 years ago, waiting for a review since October last year.

Bumping to critical as per Review process for Full Project Applications: What to Expect

Critical: Applications with a status of needs review that have been awaiting response from a reviewer for 4+ weeks.
This looks like it's been pretty thoroughly reviewed, with many rounds of revision, so hopefully it's actually close to RTBC now

@picco - if you could review some other projects to pick up the PAReview bonus while we get more reviewer eyeballs on this - hopefully we can get this approved quickly for you.

Info on the PAReview bonus project is here: https://www.drupal.org/node/1975228

kattekrab’s picture

Priority: Normal » Critical
kattekrab’s picture

Setting this back to RTBC.

It was set back to needs work for a typo.

@picco - please, if you can, do some reviews of other projects so we can get this approved ASAP.

We are trying to change the system, but at the moment, the only people reviewing and approving modules will only look at those that have the PARreview bonus.

On the other hand, if you have given up (and I would understand if you have) then you can close the issue.

In the meantime, I will try to find someone who might be willing to do a final review of your module. I wish I could do it myself!

Sorry it's taken so long, and thank you for your patience.

kattekrab’s picture

Status: Needs review » Reviewed & tested by the community
mlncn’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for your contribution! Congratulations, you are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to a full project.

Here are some recommended readings to help with excellent maintainership:

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 projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

gisle’s picture

While this one somehow got approved, it still fails on one of the main points on the review checklist.

The directory http://cgit.drupalcode.org/sandbox-picco-2176677/tree/images/88x31 in the project repo contains of 3rd party company logos that is both copyrighted and trademarked. Don't even dream about licensing this intellectual property belonging to various third parties under the GPL.

3rd party assets/code
No: Does not follow the guidelines for 3rd party assets/code.

Our policy on 3rd party materials in the repo is described in the 3rd party libraries and content on Drupal.org. It also appears in the Drupal Git Repository Usage policy you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

gisle’s picture

Status: Fixed » Needs work

Changing status.

klausi’s picture

Status: Needs work » Fixed

@gisle: This project application is closed. Please open issues in the module issue queue to removed third party content now.

Can you go back to all the fixed applications that you set to "needs work", set them to "fixed" again and open issues in the module issue queues instead?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.