[D7] iDEAL advanced

Hello,

I just did this automated test and found this:

EDIT: removed long pareview.sh blob. See http://pareview.sh/pareview/httpgitdrupalorgsandboxmvdve2353243git

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

I'm a robot and this is an automated message from Project Applications Scraper.

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.

This sounds like a feature that should live in the existing iDEAL project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the iDEAL issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

I have had contact with the developer of the other ideal modules. But unfortunately without any result.

I am the maintainer of those other modules and I know nothing of such contact.

Unfortunately is this a commercial module and not available on Drupal.org.

iDEAL 7.x-3.x is indeed sold commercially, in order to sustain the development efforts and ensure code quality.

To review the module that is under discussion here:

• Namespace conflicts: the project is called ideal_advanced, but a large part of the code base does not use this namespace. This includes the submodule and CSS.
• Copyright infringement: the iDEAL logo is protected by copyright and Currence (the copyright holder) has not given permission to distribute the image under the GPL. This means it must be removed from the repository immediately.
• Testing: there are no tests to ensure code quality and that all communication between servers is indeed secure.

Please fix at least the first two items. The last item is optional, but highly recommended.

The only other thing is unrelated to drupal.org, but a point of interest: the iDEALConnection is distributed by Dutch banks. Did they give permission for redistribution on a public platform?

I helped to test this module. And it works fine on dev install currently. Code is quite neat and follow standards as todays git.

For #7 I could mention that:

1. Existing iDEAL project is outdated unless you buy a second part of it from developer (which is sort of ok, but not always).

2. If you want to use with Drupal Commerce, existing iDEAL project is relying on few other modules: Payment for Drupal Commerce, which is relying on Payment, and of course only commercially available (by Xano) iDEAL For Drupal Commerce v3.

The second point was the main one that I would choose this module to support over existing (in terms of less dependencies). So all in all it is less of segmentation but 2 different approaches really (and unfortunately unsolvable in one project, with all my dislike of segmentation).

Anyway, Xano has some right concerns for licensing.

You see, that dislike of dependencies is exactly why we created the iDEAL project in the first place five years ago: there were so many platform-specific versions and nobody wanted or could maintain them all.

You're free to release your own module, but please don't do it just because you don't like dependencies. Many more before you thought the exact same thing and their modules ended up being poorly maintained or abandoned, and nobody needs that.

Xano, I see your approach and indeed it is valid enough. But 5 years ago we did not have Drupal Commerce to say so. 2 years ago we had to rely on custom modules to work with Commerce and now we can use what we have on D.o. pretty much out of the box.

Again, just for me it would be better to rely on one module (in worst case to update it or switch to something else) that is doing what it promise right now rather then rely on 4 which of which any can be abandoned at any moment. Roughly.

This project has a core and a separate module to integrate it with commerce. Therefore the only extra abstraction that Payment offers is some code common to payment methods, to theoretically make implementing a payment method easier.

In my opinion, however, it fails in that regard. The iDEAL plugin to Payment has critical bugs that make it unfit for live environments (only used for lack of an alternative). However, after more than 2 years it's still being sold commercially, to "assure code quality". Furthermore, there is poor response to issue reports, critical issues are left unfixed, updates are distributed over a cumbersome (for that purpose) medium, if at all.

Payment itself is not without issues either. There have been plenty of rather obscure bugs and, frankly, rather poor design decisions that have hampered adoption. I consider it another link in the chain that can break, especially as it did break in the past. It also has to be said that Payment must be the only software in the history of mankind that uses floating point for money. It does seem like a good idea to abstract the general logic of payment transactions and gateways (think Omnipay), but the particular implementation is over-engineered and has other issues with its design that make it a bad option.

This module is simpler in its approach and therefore it is a better option for most sites. Where Payment shines (in theory) is on sites where multiple payment methods must be made available to multiple channels. For example iDEAL, Paypal and Molly payment methods are available to a shop built with Commerce and a donation form (without checkout logic), on the same site. That excludes 99% of all websites.

One could argue that there are more modules that need to be supported with this approach (iDEAL -> Commerce, iDEAL -> donation, Paypal -> Commerce, Paypal -> donation, Molly -> Commerce, Molly -> donation, instead of just iDEAL -> Payment, Paypal -> Payment, Molly -> Payment, Payment -> Commerce, Payment -> donation). That's a valid point, but until there is a real incentive to choose Payment (or similar) over the simpler approach, people aren't going to use it, so there won't be much to maintain in the first place.

@a.ross: Please stick to the project application this issue is about. Complaints about Payment can be filed in its issue queue.

It was merely my 5 cents response to Klausi:

This sounds like a feature that should live in the existing iDEAL project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the iDEAL issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward.

I apparently forgot to set this RTBC... I reviewed the code once more and it's good to go.

Checked the code again too, and I could not spot any issues. Also worked just fine at my dev.

Automated Review

Review of the 7.x-1.x branch (commit 02dc949):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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.

FILE: ...ts/Drupal/PAR/pareview_temp/classes/idealAdvancedConnectorWrapper.inc
---------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------
 338 | ERROR | [x] Doc comment long description must end with a full stop
 408 | ERROR | [x] Doc comment long description must end with a full stop
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: ...l_advanced_commerce/classes/idealAdvancedCommerceConnectorWrapper.inc
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 79 | ERROR | [x] Line indented incorrectly; expected at least 6 spaces,
    |       |     found 4
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

Time: 1.13 secs; Memory: 10.75Mb

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation. Addressed.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Followsthe guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) Are logfiles used? Where are they stored? I am blocking on this pending an answer, as storage somewhere other than the private:// filesystem is an access problem.

(*) There should be no way to store the certs anywhere in the public filesystem, or anywhere else webaccessible. Personally, I would force them the the private filesystem. You could make these uploads, but I think more than one upload file in a single settings form is still broken.

Coding style & Drupal API usage

Permissions normally use spaces instead of underscores in them.

Why is ideal_advanced_icon() getting the image from the active theme? Comment needed.

Use REQUEST_TIME instead of time().

Should other IDs in your scheme have indexes on the, too? Or tuple indexes?

variable_get() normally has a default value for the second argument.

Instead of exceptions in IdealAdvancedConfigHandler::configValidate(), you may want to consider a hook_requirements(). Throwing and catchingin the same funciton is also overkill (and tends to be slow).

Why the @codingStandardsIgnoreStart / @codingStandardsIgnoreEnd? Perfect pareview is not necessary for project approval.

static:: is typically preferred over self:: Google "late static binding."

I don't see why you need to check $_POST in ideal_advanced_settings_form.

Kinda weird using your own config mechanism when system_settings_form() works pretty well.

Avoid directly using $_GET['q']; current_path() is likely better.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

The code has minor issue related with coding standards

Ex:

FILE: ...idea_advacned/ideal_advanced.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 224 | ERROR | Return comment indentation must be 2 additional spaces
--------------------------------------------------------------------------------

A few minor coding standard probems are never a reason to go back to Needs Review. The only reason that would warrant it is pretty blatant disregard for them, or code that is totally unreadable.

The private filesystem issue has been addressed adequately.

Tests page looks broken on my local admin/config/services/ideal/ideal_tests when the library is not in place, it will be great to have a warning message on this page as it has /admin/config/services/ideal

Regards

mvdve, great, It works fine on latest test I am moving this back to RTBC

manual review:

1. ideal_advanced_admin_form_tests(): doc block is wrong, this is not hook:form(). See https://www.drupal.org/coding-standards/docs#forms on how to document forms. Same for you validate/submit handlers.
2. ideal_advanced_admin_form_tests(): the check_plain() is wrong here since you are not printing the value to HTML. Make sure to read https://www.drupal.org/node/28984 again.
3. idealAdvancedConfigHandler.inc function configValidate(): throwing and catching your own exceptions in one function does not really make sense, just set a variable in case of errors and then check that? And the check_plain() is wrong here again, since you are using appropriate placholders with t() already and double escaping is bad.
4. idealAdvancedConnectorWrapper.inc: using float as price data type is problematic because of precision issues. Usually you just use cent integers as Drupal Commerce does for example.

But that are not critical application blockers, so ...

Thanks for your contribution, mvdve!

I updated your account so you can 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 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.