Handle different files with the same file name [#1719256]

The imagecache module allows defining image presets such as Small or Large which creates thumbnails or versions of the original image that is being uploaded.

When using imagecache module together with insert module to upload an image and insert it to the wysiwyg, it is possible to insert to the wysiwyg an image with the pre-defined preset so that an img element is created with a source attribute such as:
<img src="/system/files/imagecache/Large/test_image_1.png" />
or
<img src="/system/files/imagecache/Small/test_image_1.png" />

When mimemail parses the HTML to grab all these links and turn them into attachments for the email notification it is counting on the file name to create the attachments (in this case: test_image_1.png) instead of the actual file URL.
This results in a bug if the same file is created in the same node's body with different image presets (and generally this introduces a bug where-as two files with the same file name (basename) will always just use the first occurrence only).

Comment	File	Size	Author
#16	mimemail-1719256-16-d7.patch	1.6 KB	sgabe
#16
#16	mimemail-1719256-16-d6.patch	1.67 KB	sgabe
#16
#14	imemail_dont_handle_duplicate_filenames-1719256-12.patch	911 bytes	lirantal
#14
#3	mimemail_dont_handle_duplicate_filenames-1719256-2.patch	893 bytes	lirantal
#3
#1	mimemail_dont_handle_duplicate_filenames-1719256-1.patch	639 bytes	lirantal
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

lirantal CreditAttribution: lirantal commented 8 August 2012 at 14:04

Status:

Needs work

» Needs review

File	Size
mimemail_dont_handle_duplicate_filenames-1719256-1.patch	639 bytes

Patch for fixing this and relying on the filename for the static array as well as the md5.

Comment #2

sgabe CreditAttribution: sgabe commented 8 August 2012 at 14:14

Status:

Needs review

» Needs work

You forgot to modify the $filenames array.

  if (isset($file) && (@is_file($file) || $content)) {

    if (!$name) $name = (@is_file($file)) ? basename($file) : 'attachment.dat';
    if (!$type) $type = ($name) ? file_get_mimetype($name) : file_get_mimetype($file);

    // Prevent duplicate items.
    if (isset($filenames[$file])) return 'cid:' . $filenames[$file];
    $content_id = md5($file) . '@' . $_SERVER['HTTP_HOST'];

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

    $files[] = $new_file;
    $filenames[$file] = $content_id; // This needs to be changed too!

    return 'cid:' . $content_id;

But the most important is that we should only use $file if it is not the actual file content.

Comment #3

lirantal CreditAttribution: lirantal commented 9 August 2012 at 06:45

Status:

Needs work

» Needs review

File	Size
mimemail_dont_handle_duplicate_filenames-1719256-2.patch	893 bytes

You're right, thanks.
Here's an updated version.

Regards,
Liran Tal.

Comment #4

sgabe CreditAttribution: sgabe commented 9 August 2012 at 09:38

Priority:	Major	» Normal
Status:	Needs review	» Needs work

Now you forgot the most important part. :) We need to check if we are dealing with the file content, in that case we can't use $file and we have to use the $name.

Furthermore as a response to the name of your patch, we have to handle duplicate file names, see #358439: Images are only in the first message.

Comment #5

lirantal CreditAttribution: lirantal commented 9 August 2012 at 10:27

The patch handles the case of using duplicate file names by working on the full file path instead of just the basename.
Can you please explain what is the issue with checking file content?

Comment #6

sgabe CreditAttribution: sgabe commented 9 August 2012 at 10:32

If you take a better look at the code you will notice that sometimes $file will be the content:

  }
  // We have the actual content.
  elseif ($content) {
    $file = $content;
  }

  if (isset($file) && (@is_file($file) || $content)) {

Comment #7

lirantal CreditAttribution: lirantal commented 9 August 2012 at 10:32

Just to add, I've personally tested duplicate file names and it works just fine.
I've used 2 different images with the same file name (put in different folders locally), then created a new node, used the imagefield to upload and insert both of them into the node's body.

End result is that I get the 2 distinct images in the email notification as well as they both appear just fine in the node view.

Comment #8

sgabe CreditAttribution: sgabe commented 9 August 2012 at 10:56

We have posted at the same time, see #6.

Comment #9

lirantal CreditAttribution: lirantal commented 9 August 2012 at 13:32

Right, but looking at the $new_file array it seems that anyway because if $content is passed, then $file = $content (earlier) then $new_file['file'] = $content which I think is somewhat bad, isn't it?

Anyway, I think you are referring to the problem where if $content is passed then it's being set in it's entirety into the $filesnames array, right?

Comment #10

sgabe CreditAttribution: sgabe commented 9 August 2012 at 13:38

Right, but looking at the $new_file array it seems that anyway because if $content is passed, then $file = $content (earlier) then $new_file['file'] = $content which I think is somewhat bad, isn't it?

No, mimemail_multipart_body takes care of the rest.

      if (isset($part['file'])) {
        $file = (is_file($part['file'])) ? file_get_contents($part['file']) : $part['file'];
        $part_body = chunk_split(base64_encode($file), 76, variable_get('mimemail_crlf', "\n"));
      }

Anyway, I think you are referring to the problem where if $content is passed then it's being set in it's entirety into the $filesnames array, right?

Yes!

Needs work

» Needs review

File	Size
imemail_dont_handle_duplicate_filenames-1719256-12.patch	911 bytes

Updated patch with use of $content_id for files array.
Can you please review and comment?

Comment #15

sgabe CreditAttribution: sgabe commented 16 September 2012 at 10:05

Status:

Needs review

» Needs work

Just a minor thing, since we do not use the actual file names, we should rename $filenames to $ids which would be more accurate. I would also change $content_id to just $id.