Hi,
If use one image more than once, mime module process all image, and attach to mail.
I modify the function _mimemail_file and solve this problem, but need more testing.
The modified function:

function _mimemail_file($url = NULL, $name = '', $type = '', $disposition = 'related') {
  static $files = array();
  static $filenames = array(); // this modifycation, store all filenames with full path
  if ($url) {
    $url = _mimemail_url($url, 'TRUE');

    // If the $url is absolute, we're done here.
    if (strpos($url, '://') || preg_match('!mailto:!', $url)) {
      return $url;
    }
    else {
    // The $url is a relative file path, continue processing.
      $file = $url;
    }
  }

  if ($file && file_exists($file)) {
    if (isset($filenames[$file])) return 'cid:'. $filenames[$file]; // this modifycation, if key exists, return old value

    $content_id = md5($file) .'@'. $_SERVER['HTTP_HOST'];

    if (!$name) $name = substr($file, strrpos($file, '/') + 1);

    $new_file = array(
      'name' => $name,
      'file' => $file,
      'Content-ID' => $content_id,
      'Content-Disposition' => $disposition,
    );
    $new_file['Content-Type'] = _mimemail_mimetype($file, $type);

    $files[] = $new_file;
    $filenames[$file] = $content_id; // this modifycation, store filename to key, content_id to value
    return 'cid:'. $content_id;
  }

  $ret = $files;
  $files = array();
  return $ret;
}
CommentFileSizeAuthor
#5 mimemail-358439-5.patch254 bytesfolkertdv
#2 mimemail-358439-2.patch959 bytesfolkertdv
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jerdavis’s picture

jerdavis
Location Minneapolis, MN
CreditAttribution: jerdavis commented
Status: Active » Needs work

Thanks for submitting code, but please read http://drupal.org/patch/create and provide a patch, it's much much easier for us to work with and saves everyone time.

Thanks!

Jer

folkertdv’s picture

folkertdv CreditAttribution: folkertdv commented
FileSize
mimemail-358439-2.patch959 bytes

Came across this bug months ago as well and fixed it but somehow never submitted a patch.
The suggested patch looks clean and I've created a patch file for submission. Great work DTB :)

jerdavis’s picture

jerdavis
Location Minneapolis, MN
CreditAttribution: jerdavis commented
Status: Needs work » Fixed

Committed!

This should show up in the next development snapshot. Thanks for creating the patch folkertdv and thanks for submitting the code DTB.

Jer

Status: Fixed » Closed (fixed)

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

folkertdv’s picture

folkertdv CreditAttribution: folkertdv commented
Status: Closed (fixed) » Needs review
FileSize
mimemail-358439-5.patch254 bytes

Reopening this issue because the patch above introduces new problems (an additional patch file is provided).

Problem:
If the mimemail function is called, certain files are embedded and their cid's are stored in the filenames array (to prevent duplicates).
Subsequent calls of the mimemail function will not embed files that have been embedded in previous mails.

Cause:
The static filenames array isn't emptied at the end of the _mimemail_file function like the static files array.

Adding the single line in the attached patch file has fixed the problem for me.

Cheers!

folkertdv’s picture

folkertdv CreditAttribution: folkertdv commented

Bumping this issue before it closes, this (minor) patch still needs reviewing :)

eikes’s picture

eikes CreditAttribution: eikes commented

@folkertdv patch in #5 works

thanks a lot!

i was wondering why only the latest subscriber got the attachments...

makr’s picture

makr CreditAttribution: makr commented

Thanks, this also resolves issue http://drupal.org/node/453334, where only the first recipient receives the email-newsletter containing pictures (simplenews + mimemail).

askibinski’s picture

askibinski CreditAttribution: askibinski commented

Has this already been commited to the latest dev?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

subscribing & reviewing

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Needs review » Reviewed & tested by the community

Works just perfect. Please commit.

zoo33’s picture

zoo33 CreditAttribution: zoo33 commented
Status: Reviewed & tested by the community » Needs work

The fix suggested in #5 would just undo the static nature of the $filenames variable, wouldn't it? In that case, a better solution would be to just remove the "static" keyword. Or rather, revert the original patch.

By the way, the $files variable has the same problem, it shouldn't be static if that means it has to be reset each time the function executes. Static variables are for data that should be preserved between function calls.

Frans’s picture

Frans CreditAttribution: Frans commented

subscribe

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
CreditAttribution: hanoii commented
Status: Needs work » Reviewed & tested by the community

The patch on #5 does work and fixes the issue and I agree with #12, although I rather have this one reviewed and tested so it might get committed soon rather than holding it.

If I have some time, I'll look more into this and submit a patch with the static removed or something, will look at this a little bit deeper, but otherwise, having it static, even if it's not really used as static, does not harm that much, but this bug does.

folkertdv’s picture

folkertdv CreditAttribution: folkertdv commented

Actually, the static prefix wasn't introducted by the patch. It was there already for the files variable and duplicated for the filenames variable.
I suggest the code is carefully analyzed, since the static variable serves a purpose and is there for a reason (notice the function can return before the static variable is reset at the bottom).

NicP’s picture

NicP CreditAttribution: NicP commented

When you see code like this you understand why objects are better.

The design seems to rely on the static variable being static for the duration of an email. The desired result of the function is delivered when the function is finally called with no parameters, at that time the statics can be cleared.

So #5 is correct, I wish I had found this issue before I fixed it myself.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented

Duplicated by #506182: Attachment only sent to one email from list email subscribe and #453334: simplenews and mimemail combination, pictures only got send to first registered email adres.
jerdavis, Allie Micka, what is needed to get this patch committed?

sbogner@insightcp.com’s picture

sbogner@insightcp.com CreditAttribution: sbogner@insightcp.com commented

Just came across this issue too - looks like patches have been made & reviewed; what is needed to commit this?

folkertdv’s picture

folkertdv CreditAttribution: folkertdv commented

A more active maintainer by the looks of it. I provided the patch almost a year ago!
Since this module affects a lot of other modules (eg. simplenews), I consider this a serious bug.. pity it's been open for so long.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

I have not tested alpha2 yet, but the added line is in.
I'm not sure why jerdavis has not mentioned his February 23, 2009 commit in this issue.

http://drupal.org/cvs?commit=174768

sgabe’s picture

sgabe CreditAttribution: sgabe commented
Priority: Normal » Critical

The fix for this is in #5 which is not committed so far.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

EDIT: I stand corrected. The commit message references this issue, but the committed line of code has a small but very important difference. Sigh.

The fix for this is in #5 which is not committed so far.

Please follow the link in #20 to the commit message and click on the link to the 1.40 diff. You'll see the code from the patch in #5. In fact, that line of code is there both in HEAD and DRUPAL-6--1-0-ALPHA2.

sgabe’s picture

sgabe CreditAttribution: sgabe commented
Title: duplicate image items in attachment, solved » Images are only in the first message

Changing title to a more descriptive one.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

I just sent a newsletter with images and all emails seem fine. (Mime Mail alpha2 + Newsletter 1.1 with the Hotmail patch in http://drupal.org/node/372710#comment-2796702)

EDIT: and sending 4 e-mails in one cron run (Poormanscron 6.x-2.2).

Could someone please rephrase when problems occur and exactly what problems, since I forgot how to reproduce this...

sgabe’s picture

sgabe CreditAttribution: sgabe commented

@Eric_A: If you send a newletter with an embedded image for multiple e-mail addresses, only the first message will contain the image. The rest will have the cid: in the source, but no encoded attachment in the message.

To reproduce:

  • Create a newsletter with multiple subscribers. (Use e-mail accounts which you have access to.)
  • Create a newsletter issue for this newsletter with an embedded image. (You have to use relative path!)
  • Send it and check the arrived e-mail messages.
acb’s picture

acb CreditAttribution: acb commented

I too can confirm bug in #25; it occurs with RELATIVE vs. absolute path.

jerdavis’s picture

jerdavis
Location Minneapolis, MN
CreditAttribution: jerdavis commented
Status: Reviewed & tested by the community » Fixed

This has been committed to HEAD, once a few of the other critical fixes have been tested and committed we'll be rolling a new alpha release.

Jer

Status: Fixed » Closed (fixed)

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

benone’s picture

benone CreditAttribution: benone commented

subscribe

Stijn Vanhandsaeme’s picture

Stijn Vanhandsaeme CreditAttribution: Stijn Vanhandsaeme commented

In fact the line
$filenames = array();
needs to be added on line 156 in mimemail.inc,v 1.41 2010/03/24
I have no knowhow of submitting a patch, sorry.

Stijn Vanhandsaeme’s picture

Stijn Vanhandsaeme CreditAttribution: Stijn Vanhandsaeme commented
Status: Closed (fixed) » Active

sorry, set to active again.

sgabe’s picture

sgabe CreditAttribution: sgabe commented
Status: Active » Closed (fixed)

This has been already committed to HEAD. Closing again.

roball’s picture

roball CreditAttribution: roball commented

Hurray - finally this fix is in a release for the first time (6.x-1.0-alpha3). The first release that works with Simplenews out of the box.