Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#34 | typo_error_found_on_module_file-32.patch | 382 bytes | SoumyaDas |
Comments
Comment #1
PA robot CreditAttribution: PA robot commentedThere 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.
Comment #2
picco CreditAttribution: picco commentedThe issues detected by PA robot have been resolved:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpicco2176677git
Comment #3
picco CreditAttribution: picco commentedComment #4
pachabhaiya CreditAttribution: pachabhaiya commentedDear 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
Comment #5
picco CreditAttribution: picco commentedThank 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.
Comment #6
picco CreditAttribution: picco commentedI also ran PAReview again to make sure everything stays up to standard:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpicco2176677git
Comment #7
picco CreditAttribution: picco commentedComment #8
picco CreditAttribution: picco commentedUpdate:
1) I added .install file that implements hook_install() and cleans up the variables table.
I meant hook_uninstall() :)
Comment #9
mister.koz CreditAttribution: mister.koz commentedHey 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.
Comment #10
SchnWalter CreditAttribution: SchnWalter commentedHello 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.
Comment #11
picco CreditAttribution: picco commentedThank 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.
Comment #12
picco CreditAttribution: picco commentedCould i ask for another review :)
Comment #13
ragnarkurm CreditAttribution: ragnarkurm commentedTere Ivo :)
Great job collecting all these pieces of code.
Browsed through the code and few things got my attention.
(Can you do me a review as well?)
Comment #14
ragnarkurm CreditAttribution: ragnarkurm commentedComment #15
kaido.toomingas CreditAttribution: kaido.toomingas commentedComment #16
picco CreditAttribution: picco commentedRagnar & 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.
Comment #17
picco CreditAttribution: picco commentedComment #18
klausson CreditAttribution: klausson commentedPAReview.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.
Comment #19
picco CreditAttribution: picco commentedThank 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.
Comment #20
picco CreditAttribution: picco commentedComment #21
robbertv CreditAttribution: robbertv commentedI reviewed the code. I see a couple of minor issues.
Comment #22
klausiThose 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?
Comment #23
Noe_ CreditAttribution: Noe_ commentedLooks RTBC to me.
Comment #24
kscheirerSecurity question:
Non-blocking issues:
(!$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.Assigning to klausi for security question, otherwise this is ready to go from me.
Comment #25
kscheirerComment #26
klausiThe 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?
Comment #27
picco CreditAttribution: picco commentedThank 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?
Comment #28
chandrasekhar539 CreditAttribution: chandrasekhar539 commentedAutomated 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
Comment #29
chandrasekhar539 CreditAttribution: chandrasekhar539 commentedAutomated 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
Comment #30
klausiThose minor formatting issue are surely not application blockers, anything else that you found or should this be RTBC instead?
Comment #31
chandrasekhar539 CreditAttribution: chandrasekhar539 commentedHi klausi,
Except this no thers issue in this module
Comment #32
SoumyaDas CreditAttribution: SoumyaDas as a volunteer commentedTypo error found on module file
Comment #33
SoumyaDas CreditAttribution: SoumyaDas as a volunteer commentedComment #34
SoumyaDas CreditAttribution: SoumyaDas as a volunteer commentedComment #35
SoumyaDas CreditAttribution: SoumyaDas as a volunteer commentedAdded https://www.drupal.org/files/issues/typo_error_found_on_module_file-32_0...
Comment #36
klausi@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
Comment #37
SoumyaDas CreditAttribution: SoumyaDas as a volunteer commented@klausi Yeah that is a mistake. Thanks!
Comment #38
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedFirst 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
@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
Comment #39
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedComment #40
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedSetting 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.
Comment #41
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedComment #42
mlncn CreditAttribution: mlncn at Agaric commentedThanks 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.
Comment #43
gisleWhile 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.
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.
Comment #44
gisleChanging status.
Comment #45
klausi@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?