This is an application for permission to create full projects, and specifically for this project, Bounce.

1) Project overview

The Bounce module collects non-delivery reports generated by remote mail servers in response to emails sent from your Drupal site. It parses these non-delivery reports for response codes, scores those codes, and blocks further emails from being sent to email addresses that have passed a score threshold. This helps prevent your site from looking like a spam source by being a good internet citizen and respecting non-delivery responses, such as a notice that a recipient email account no longer exists.

This module is a rewrite of non-Drupal code that itself resulted from some years of experience running a few-thousand-subscriber mailing list covering the sane end of a topic that, unfortunately, tends to trigger spam filters - because there is a large and obnoxious spam-generating industry over at the other, less sane end of the pool. The default settings in Bounce are geared towards helping to maintain good long-term deliverability of mail under that sort of circumstance.

2) Project page

http://drupal.org/sandbox/exratione/1484526

3) Git repository

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/exratione/1484526.git bounce

4) Drupal version

Drupal 7 only.

5) Other comments

So far as I can see, Drupal doesn't have much in the way of general stand-alone modules with full admin interfaces to manage email list gardening through monitoring of non-delivery reports. Those that do exist are partially featured only, old, and by the look of it not moving much. Given the number of modules focused on mass mailing, that seems like a curious omission. So there's:

a) Bounced Email, a small module that just assembles data from a mailserver rather than acting on it, and doesn't have a D7 version (not that it would be hard to make one):

http://drupal.org/project/bounced_email

b) Simplenews bounce is a similar set of features to Bounced Email, but for Simplenews only, again no D7 version.

http://drupal.org/project/simplenews_bounce

c) Bounce handler is somewhat better, but doesn't seem very user friendly, and not for D7 either.

http://drupal.org/project/bounce_handler

So, the rudiments are there but that's about it. Unlike all of the above, Bounce has fairly comprehensive test code and a robust modular design to let people plug in components via hook implementations to perform non-delivery report processing their own way. It's a foundation for further development with some useful defaults rather than a unit.

6) Manual Reviews of Other Modules for the Review Bonus (Batch 1)

eztexting_gateway
http://drupal.org/node/1394212#comment-6073902
http://drupal.org/node/1394212#comment-6073914

happycaptcha
http://drupal.org/node/1536250#comment-6075142

swiftmailer
http://drupal.org/node/1174572#comment-5782242
http://drupal.org/node/1174572#comment-5793156

7) Manual Reviews of Other Modules for the Review Bonus (Batch 2)

extlink_overlay
http://drupal.org/node/1554554#comment-6100002

nodequeue_context
http://drupal.org/node/1489538#comment-6099204

webmaster_menu
http://drupal.org/node/1420358#comment-6099242

CommentFileSizeAuthor
#22 drupalcs-result.txt12.04 KBklausi

Comments

nmudgal’s picture

nmudgal commented
Status: Needs review » Needs work

Hey,
Welcome & thanks for contribution.

While someone does in-depth review please take some time out to resolve the automated review results
http://ventral.org/pareview/httpgitdrupalorgsandboxexratione1484526git

Thanks.

exratione’s picture

exratione commented
Status: Needs work » Needs review

Automated review results are resolved, barring the one wherein the code is following the guidelines for form callback docs and the review doesn't like that.

sbrattla’s picture

sbrattla commented

Hi,

Trying to get the module via GIT, but I'm having trouble getting it. 

user@computer:~/Documents/Private/Projects$ git clone http://git.drupal.org/sandbox/exratione/1484526.git bounce
Initialized empty Git repository in /home/stian/Documents/Privat/Projects/bounce/.git/
remote: Counting objects: 151, done.
remote: Compressing objects: 100% (143/143), done.
remote: Total 151 (delta 99), reused 0 (delta 0)
Receiving objects: 100% (151/151), 93.92 KiB | 99 KiB/s, done.
Resolving deltas: 100% (99/99), done.

The 'bounce' directory is empty even though GIT says it's done.

I then try 'git pull origin 7.x-1.x', which gives me the following output : 

user@computer:~/Documents/Private/Projects/bounce$ git pull origin 7.x-1.x
From http://git.drupal.org/sandbox/exratione/1484526
 * branch            7.x-1.x    -> FETCH_HEAD
Auto-merging README.txt
CONFLICT (content): Merge conflict in README.txt
CONFLICT (delete/modify): bounce.admin.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.admin.inc left in tree.
CONFLICT (delete/modify): bounce.analysis.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.analysis.inc left in tree.
CONFLICT (delete/modify): bounce.api.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.api.inc left in tree.
CONFLICT (delete/modify): bounce.blocker.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.blocker.inc left in tree.
CONFLICT (delete/modify): bounce.connector.class.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.connector.class.inc left in tree.
CONFLICT (delete/modify): bounce.connector.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.connector.inc left in tree.
CONFLICT (delete/modify): bounce.connector.test.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.connector.test.inc left in tree.
CONFLICT (delete/modify): bounce.cron.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.cron.inc left in tree.
CONFLICT (delete/modify): bounce.features.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.features.inc left in tree.
CONFLICT (delete/modify): bounce.info deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.info left in tree.
CONFLICT (delete/modify): bounce.install deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.install left in tree.
CONFLICT (delete/modify): bounce.mail.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.mail.inc left in tree.
CONFLICT (delete/modify): bounce.module deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.module left in tree.
CONFLICT (delete/modify): bounce.test deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.test left in tree.
CONFLICT (delete/modify): bounce.user.inc deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of bounce.user.inc left in tree.
CONFLICT (delete/modify): css/bounce.admin.css deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of css/bounce.admin.css left in tree.
CONFLICT (delete/modify): js/bounce.connector.js deleted in HEAD and modified in e9fa4701095cff24bdfd4aac956f75b3368d6040. Version e9fa4701095cff24bdfd4aac956f75b3368d6040 of js/bounce.connector.js left in tree.
Automatic merge failed; fix conflicts and then commit the result.

Not sure what's causing the problem, but I'm able to retrieve other modules via GIT so I don't think the issue is rooted in my machine?

sbrattla’s picture

sbrattla commented
Issue summary: View changes

Editing for git branch.

exratione’s picture

exratione commented

Try:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/exratione/1484526.git bounce

rather than

git clone http://git.drupal.org/sandbox/exratione/1484526.git bounce

With the --branch option, that should get you the right branch immediately. If you omit it you only get the empty master locally, and you'd then have to switch to/checkout the remote 7.x-1.x branch.

I think that your issue there with the errors is a result of trying to merge the remote 7.x-1.x branch with the local master (which would be what pull does).

sbrattla’s picture

sbrattla commented

Thanks,

Managed to get it (not that well versed with GIT).

I did install it and things worked well at first sight. I set it up with a test account which bounced e-mails would return to, and entered the test e-mail account in the "Non-delivery report recipient" field. With regards to this, I've got one comment.

Under the "Connector" tab, If I enter connection details which for some reason does not work, you get an error message. However, the password field gets blanked out, and if you press save again you don't get an error message (you just get the usual green message saying that the changes was saved). I'd say it would make sense to attempt to validate the details by connecting to the account every time the user press save.

Now, i tried to send an e-mail to check if the Return-Path header was added to sent e-mails. It appears that it didn't. I then added some gibberish code to hook_mail_alter in 'bounce.mail.inc' which would make the execution fail, but nothing failed. Somehow, the hook doesn't get invoked on my Drupal. I'm testing on a 7.9 installation.

Can't really say why this happens...but could it have something to do with the hook being in an external file, and Drupal not invoking it since it is specified through bounce_hook_info?

I'll be glad to test things further if you manage to figure out what the root of this issue might be?

sbrattla’s picture

sbrattla commented
Status: Needs review » Needs work
exratione’s picture

exratione commented

I am developing on 7.12. I guess I'll have to take a look at 7.9 - but it shouldn't make any difference to hook_hook_info().

Item 1: The Password Field in the Connector
--------------------------------------------------------

This is a copy of the functionality in SMTP Authentication Support for entering a password.

EDIT: I misunderstood your description of the bug - I see now that I've tried it again myself. I have committed a fix that ensures that the connection details are rechecked in the case where a password is already set.

Item 2: Return Path
-------------------------

Are you using an SMTP module to send mail? e.g. SMTP Authentication Support

http://drupal.org/project/smtp

If you send mail through the default Drupal mechanism, as explained in the project README, then yes on most setups the Return-Path header will be stripped.

I debated making SMTP Authentication Support required, but there are other modules for sending mail through servers rather than the local mail() function, and I don't want to limit people. So the alternative to that is making it clear in the docs that you MUST use something other than the default in order to send.

sbrattla’s picture

sbrattla commented

Hi,

I'm sending mails using the SwiftMailer module (which I am waiting to get approved in the applications queue too). However, it appears that your implementation of hook_mail_alter() isn't invoked at all. As far as I understand, this is where the Return-Path is added.

It is drupal_mail() which invokes the various implementations of hook_mail_alter(), and it puzzles me that it doesn't invoke yours. I've added logging statements inside your implementation of the hook, but nothing gets logged. In other words, the hook is never invoked when I attempt to send e-mails.

My module (SwiftMailer) which sends the e-mails can't really be responsible for messing up things. What it does is simply to install itself as the default mail system. Drupal, with its drupal_mail() runs all implementations of hook_mail_alter() before the mail system is loaded (which is where my module comes in)...so something odd must be going on in with Drupal not invoking the hook?

exratione’s picture

exratione commented

That is odd.

Can you tell me whether the simpletests work for you for Bounce?

One of the test methods - runTestSentMailToDatabase() - will fail if hook_mail_alter() cannot be invoked.

It all works for me, needless to say, so some digging will be done here on my part to see if I can get it to fail.

exratione’s picture

exratione commented

On checking the release notes for 7.12, I see that I require one of the recent features to block sending:

http://drupal.org/node/800434

I suppose that means I should back that out in favor of one of the older methodologies to allow any 7.x version to work. But that doesn't explain the issue you're seeing.

Separately - you probably should be working in 7.12 since it's a security update!

----------

I will work on these issues. No sense in looking at this further until I've figured it out.

exratione’s picture

exratione commented
Status: Needs work » Needs review

I have committed the following changes:

- updated README.txt to make clear the need for mail modules to support setting Return-Path
- moved hook_mail_alter() to the main module file; apparently that hook completely ignores hook_hook_info() directives.
- fixed the issue with the connector settings form and the password

What I need to do separately from further review is:

- write myself a patch for SMTP Authentication Support, as it apparently doesn't support setting a Return-Path. Crazy.
- go carry out a review of mail sending modules to see which of them do support setting a Return-Path.
- then write some more information into the docs to help site owners pick a working solution.

I could address this issue by turning Bounce into a module that also contains an SMTP implementation, but that seems frankly silly. That function should be left to other, dedicated modules, and the real problem here is that there are SMTP implementing modules that don't allow you to set the Return-Path header.

So I'll give Swift Mailer a run through as the next action item, I think, and see if the Return-Path thing works now that I've fixed things.

EDIT: I tried out Swift Mailer and it works for me; Return-Path is correctly set by Bounce, and showing up in the recipient in-box.

sbrattla’s picture

sbrattla commented

In reply to your latest post...

1. The hook_mail_alter() now works in 7.9 after you moved it to the 'bounce.module' file. It is a pity though, because the way you had it before was cleaner.

2. If you depend on functionality introduced in 7.12, why not just require at least version 7.12. I believe this should be possible according to http://drupal.org/node/211747.

3. I realise that the behaviour for the password field is just the same in my module! Could it be an idea to add a piece of text below saying something in the lines of "A password has already been set...hidden for security reasons"...or a line of javascript which adds the text as a greyed out in the actual textbox?

4. The selected protocol isn't preserved when I save settings. I chose POP3, but it went to POP3S after i saved?

5. During cron, I get:

Notice: Undefined index: data in BounceMailServerConnector->mailDecodePart() (line 557 of /var/www/playground/public_html/sites/all/modules/bounce/bounce.connector.class.inc).

In general, I find it somehow strange that other SMTP modules doesn't support the headers which the Drupal core supports. For instance, the SwiftMailer module just takes whatever headers it finds in $message and adds them to the e-mail. The SwiftMailer library does add a few things itself, but it shouldn't really do any magic on existing headers.

sbrattla’s picture

sbrattla commented

Follow up on #12:

Even though i get an error during cron, bounced e-mails are successfully retrieved from the bounce account. In other words, things seems to be working as the Return-Path header gets added successfully and Bounce retrieves bounced e-mails successfully. Seems to be a few warnings that needs to be corrected though. I'll be happy to test it again when you've had the time to look into it.

exratione’s picture

exratione commented

I have fixed:

- the warnings you were seeing on cron run, which would have shown up for mails with base 64 encoded or quoted-printable parts
- the issue in the connector settings where the protocol selector was not correctly defaulted

Per the official instructions for .info files it's not possible to specify a minimum core version - you can only specific a minimum module version. So what I've done is to put in:

dependencies[] = system (>=7.12)

Which seems devious, but works. If you're still on 7.9 that won't let you install unless you change the .info file to >=7.9.

I'll think about what to do with the password field; your suggestion is a good one.

----

Feel free to give the module a further run-through.

sbrattla’s picture

sbrattla commented

I haven't tested the latest change, but I've tested your module a little more and it seems good and works well. It is indeed a really useful module, at least for sites that want to make sure that the data it has on users are up to date!

exratione’s picture

exratione commented

I have amended the readme file with info on which SMTP-capable mail modules I've tested and found able to set the Return-Path correctly - only about half of those I tried, sadly. I filed some bugs, for what it's worth. If time allows, I'll look into submitting patches.

I've changed the text under the password field to make it more clear that a password is set when one is set.

---

At this point, unless there are further issues uncovered, I think that this module should probably be set to Reviewed & tested by the community - though I would like to see at least one other person look it over and kick the tires.

jahsh’s picture

jahsh commented

Do you have a module for drupal 6 by chance? This would be very helpful.

exratione’s picture

exratione commented

No D6 version at this time, as it happens; I was very happy to ditch the whole issue of having to know what other SMTP modules are running in D6. See notes here for some of that, under "Prevent Mail Going Out to Marked Addresses" near the end of the post:

http://www.exratione.com/2011/02/how-to-write-an-email-bounce-processing...

Backporting Bounce to D6 shouldn't be hard - the tough part from the point of view of needing to know things is described in the post above. Well, that and patching the D6 versions of SMTP Authentication Support and PHPMailer for Return-Path support, which I haven't done yet. All of the other issues have been worked out for this D7 version.

But first things first - this module needs to get approved.

sbrattla’s picture

sbrattla commented

I've tested the module with Drupal 7.14, and it works just as well as it did before. The error messages i used to receive when cron ran is gone, and the module works in general just as expected.

If it was up to me, i'd set the status to 'reviewed & tested by the community'. However, it might be an idea to get one more person to review it before doing that?

exratione’s picture

exratione commented

Yes, I'd like to see another reviewer look at this. More eyes can't hurt.

exratione’s picture

exratione commented
Issue summary: View changes

Git line.

exratione’s picture

exratione commented
Issue tags: +PAreview: review bonus

Adding PAReview: review bonus tag.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus
StatusFileSize
new drupalcs-result.txt12.04 KB

Review of the 7.x-1.x branch:

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. bounce_hook_info(): I think you got hook_hook_info() wrong. Its purpose is to define dedicated files for other modules where they can put hooks your module provides. Example: If you provide a hook_bounce_message_alter() and specify the group "bounce" I can implemented that hook as mymodule_bounce_message_alter() in mymodule.bounce.inc. So you will have to move all of your hook implementations to the module file or require_once those files globally from your module file.
  2. bounce_include(): no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once dirname(__FILE__) . '/mymodule.inc';
  3. bounce_preprocess_page(): as bounce_notify_user_message is user provided input you need to sanitize it before passing it to drupal_set_message(). See http://drupal.org/node/28984
  4. "module_invoke_all('bounce_mails_unblocked', $mails);": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php . I think bounce.api.inc might be that, so just rename it and fix the function names (should start with "hook_").
  5. bounce_get_mailserver_connector(): no need to call bounce_include(), Drupal will autoload the class for you.
  6. bounce.connector.js: please use Drupal.behaviors, see http://drupal.org/node/756722

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

exratione’s picture

exratione commented

Appreciate the care taken there. I will dig in to the details in a few days.

Regarding point (1), it's not clear to me that I am doing something terribly wrong - the module that declares an implementation of hook_hook_info() is a member of the set of all available modules, and the hook acts on all available modules. Nothing in the hook documentation suggests that it's only to be used to organize files in modules other than the declaring module.

http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

OTOH, hook_hook_info() doesn't work for all hooks, which makes it not a great way to organize code - but I was a fair way into using it by the time I figured that out, so decided to forge ahead rather than back out.

Perhaps this is a documentation issue for that hook, if the intent is as you suggest?

exratione’s picture

exratione commented
Status: Needs work » Needs review

Changes from #22 made, back to needs review status.

exratione’s picture

exratione commented
Issue summary: View changes

Adding manual review notes.

exratione’s picture

exratione commented
Issue tags: +PAreview: review bonus

Adding PAReview: review bonus tag again, round 2.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

I'm pretty sure hook_hook_info() should only be used for your own hooks that you want to be loaded from special inc files. Please file a Drupal core documentation issue if you think this is not clear.

Looks RTBC to me now! Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

exratione’s picture

exratione commented

Yes, agreed, I asked around and looked at the example of Features use of hook_hook_info(). It's one of those things where the docs are clear when you know the intent, but not so much beforehand.

misc’s picture

misc commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, exratione! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Review bonus links again.